Re: Lack of PageSetLSN in heap_xlog_visible

On Fri, 2022-11-11 at 12:43 +0200, Konstantin Knizhnik wrote:
> Yes, you are right: my original concerns that it may cause problems
> with 
> recovery at replica are not correct.

Great, thank you for following up.

> 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.

It could cause a theoretical problem: during recovery, the page could
be flushed out before minRecoveryPoint is updated, and while that's
happening, a crash could tear it. Then, a subsequent partial recovery
(e.g. PITR) could recover without fixing the torn page.

That would not be a practical problem, because it would require a tear
between two fields of the page header, which I don't think is possible.

> 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.

Agreed, thank you for the report!

Committed d6a3dbe14f and backpatched through version 11.

Also committed an unrelated cleanup patch in the same area (3eb8eeccbe)
and a README patch (97c61f70d1) to master. The README patch doesn't
guarantee that things won't change in the future, but the behavior it
describes has been true for quite a while now.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS