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
