Possible correctness issue in neon_read_at_lsnv() that could cause incorrect pages on Hot Standby replicas

[mvdholst]

In the function neon_read_at_lsnv(), there is the following code:

for (int i = 0; i < nblocks; i++)
{
	void	   *buffer = buffers[i];
	BlockNumber blockno = base_blockno + i;
	neon_request_lsns *reqlsns = &request_lsns[i];
	TimestampTz		start_ts, end_ts;

	if (PointerIsValid(mask) && !BITMAP_ISSET(mask, i))
		continue;

	start_ts = GetCurrentTimestamp();

	if (RecoveryInProgress() && MyBackendType != B_STARTUP)
		XLogWaitForReplayOf(reqlsns[0].request_lsn);

I think the last line here is incorrect, and should be

       XLogWaitForReplayOf(reqlsns[i].request_lsn)

Note that 0 is replaced with i.

The current code, as is, might return pages from the page server that have LSNs that are higher or equal to the current WAL replay LSN. In case of a multi-page WAL record, this might cause problems if the WAL record is partly replayed, and some pages were missing in the buffer pool and skipped by redo.

[kelvich]

Thank you!

@knizhnik can you please take a look

[knizhnik]

It is not a bug, just need to look some line above where variable reqlsns is initialized:

		neon_request_lsns *reqlsns = &request_lsns[i];
		TimestampTz		start_ts, end_ts;

		if (PointerIsValid(mask) && !BITMAP_ISSET(mask, i))
			continue;

		start_ts = GetCurrentTimestamp();

		if (RecoveryInProgress() && MyBackendType != B_STARTUP)
			XLogWaitForReplayOf(reqlsns[0].request_lsn);

So, reqlsns[0].request_lsn is strange but correct way to write reqlsns->equest_lsn

[mvdholst]

Yup, my mistake.

I still suggest to simplify the code a little bit. The last line becomes much clearer when you write:

XLogWaitForReplayOf(reqlsns->request_lsn);

i.e. get rid of the [0] stuff, which is weird anyway, as reqlsns is not an array, but just a pointer to some struct.