Re: Lack of PageSetLSN in heap_xlog_visible

On 11.11.2022 03:20, Jeff Davis wrote:
> On Thu, 2022-10-13 at 12:49 -0700, Jeff Davis wrote:
>
>> It may violate our torn page protections for checksums, as well...
> I could not reproduce a problem here, but I believe one exists when
> checksums are enabled, because it bypasses the protections of
> UpdateMinRecoveryPoint(). By not updating the page's LSN, it would
> allow the page to be flushed (and torn) without updating the minimum
> recovery point. That would allow the administrator to subsequently do a
> PITR or standby promotion while there's a torn page on disk.
>
> I'm considering this a theoretical risk because for a page tear to be
> consequential in this case, it would need to happen between the
> checksum and the flags; and I doubt that's a real possibility. But
> until we formalize that declaration, then this problem should be fixed.
>
> Patch attached. I also fixed:
>
> * The comment in heap_xlog_visible() says that not updating the page
> LSN avoids full page writes; but the causation is backwards: avoiding
> full page images requires that we don't update the page's LSN.
> * Also in heap_xlog_visible(), there are comments and a branch
> leftover from before commit f8f4227976 which don't seem to be necessary
> any more.
> * In visibilitymap_set(), I clarified that we *must* not set the page
> LSN of the heap page if no full page image was taken.
>
>
>>> But it still not clear for me that not bumping LSN in this place is
>>> correct if wal_log_hints is set.
>>> In this case we will have VM page with larger LSN than heap page,
>>> because visibilitymap_set
>>> bumps LSN of VM page. It means that in theory after recovery we may
>>> have
>>> page marked as all-visible in VM,
>>> but not having PD_ALL_VISIBLE  in page header. And it violates VM
>>> constraint:
>> I'm not quite following this scenario. If the heap page has a lower
>> LSN
>> than the VM page, how could we recover to a point where the VM bit is
>> set but the heap flag isn't?
> I still don't understand this problem scenario.
>
>

I am sorry that I have reported the issue and then didn't reply.
Yes, you are right: my original concerns that it may cause problems with
recovery at replica are not correct.
Not updating page LSN may just force it's reconstruction.
I also not sure that it can cause problems with checksums - page is
marked as dirty in any case.
Yes, content and checksum of the page will be different at master and
replica. It may be a problem for recovery pages from replica.

When it really be critical - is incremental backup (like pg_probackup)
whch looks at page LSN to determine moment of page modification.

And definitely it is critical for Neon, because LSN of page
reconstructed by pageserver is different from one expected by compute node.

May be I missing something, but I do not see any penalty from setting
page LSN here.
So even if there is not obvious scenario of failure, I still thing that
it should be fixed. Breaking invariants is always bad thing
and there are should be very strong arguments for doing it. And I do not
see them here.

So it will be great if your patch can be committed.