Hello, Andres.
> Fails to apply at the moment: http://cfbot.cputube.org/patch_37_2947.log
Thanks for notifying me. BTW, some kind of automatic email in case of
status change could be very helpful.
> Marked as waiting for author.
New version is attached, build is passing
(https://cirrus-ci.com/build/5599876384817152), so, moving it back to
"ready for committer" .
This may be a naive comment but I'm curious: The entire new second paragraph of the README scares me:
+There are restrictions on settings LP_DEAD bits by the standby related to
+minRecoveryPoint value. In case of crash recovery standby will start to process
+queries after replaying WAL to minRecoveryPoint position (some kind of rewind to
+the previous state). A the same time setting of LP_DEAD bits are not protected
+by WAL in any way. So, to mark tuple as dead we must be sure it was "killed"
+before minRecoveryPoint (comparing the LSN of commit record). Another valid
+option is to compare "killer" LSN with index page LSN because minRecoveryPoint
+would be moved forward when the index page flushed. Also, in some cases xid of
+"killer" is unknown - for example, tuples were cleared by XLOG_HEAP2_PRUNE.
+In that case, we compare the LSN of the heap page to index page LSN.
In terms of having room for bugs this description seems like a lot of logic to have to get correct.
Could we just do this first pass as:
Enable recovery mode LP_DEAD hint bit updates after the first streamed CHECKPOINT record comes over from the primary.
?
Now, maybe there aren't any real concerns here but even then breaking up the patches into enabling the general feature in a limited way and then ensuring that it behaves sanely during the standby crash recovery window would likely increase the appeal and ease the burden on the potential committer.
The proposed theory here seems sound to my inexperienced ears. I have no idea whether there are other bits, and/or assumptions, lurking around that interfere with this though.
David J.
