Hash index AM fake LSN oversight
I noticed a minor oversight in my commit e5836f7b: one remaining hash
index WAL record type lacks a corresponding "PageSetLSN(buf,
XLogGetFakeLSN(rel))" for the unlogged relation case.
This is completely harmless, but it should be fixed on general
principle. Attached patch fixes it.
--
Peter Geoghegan
Attachments:
v1-0001-Advance-fake-LSN-consistently-in-hash-index-AM.patchapplication/octet-stream; name=v1-0001-Advance-fake-LSN-consistently-in-hash-index-AM.patchDownload+2-1
On Wed, Jul 15, 2026 at 6:48 AM Peter Geoghegan <pg@bowt.ie> wrote:
I noticed a minor oversight in my commit e5836f7b: one remaining hash
index WAL record type lacks a corresponding "PageSetLSN(buf,
XLogGetFakeLSN(rel))" for the unlogged relation case.
Confirmed -- log_split_page() is the only data-page site e5836f7b left
without a fake-LSN branch (the _hash_init meta/bitmap paths set the LSN
under WAL only, but those aren't read concurrently, so that's expected).
The fix is correct.
One optional cosmetic point: the other fake-LSN sites, including the two
functions e5836f7b just reworked in this file, hoist recptr out of the
if and call PageSetLSN once afterwards, rather than once per branch.
Matching that form would keep log_split_page consistent with them:
log_split_page(Relation rel, Buffer buf)
{
XLogRecPtr recptr;
if (RelationNeedsWAL(rel))
{
XLogBeginInsert();
XLogRegisterBuffer(0, buf, REGBUF_FORCE_IMAGE | REGBUF_STANDARD);
recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_SPLIT_PAGE);
}
else
recptr = XLogGetFakeLSN(rel);
PageSetLSN(BufferGetPage(buf), recptr);
}
But your version is functionally identical with a smaller backpatch
diff, so this is entirely your call -- +1 either way.
This is completely harmless, but it should be fixed on general
principle. Attached patch fixes it.--
Peter Geoghegan
--
Regards,
Ewan Young
On Wed, Jul 15, 2026 at 2:47 AM Ewan Young <kdbase.hack@gmail.com> wrote:
Confirmed -- log_split_page() is the only data-page site e5836f7b left
without a fake-LSN branch (the _hash_init meta/bitmap paths set the LSN
under WAL only, but those aren't read concurrently, so that's expected).
The fix is correct.
Pushed. Thanks for the review!
One optional cosmetic point: the other fake-LSN sites, including the two
functions e5836f7b just reworked in this file, hoist recptr out of the
if and call PageSetLSN once afterwards, rather than once per branch.
I agree that it works slightly better that way; that's what I did in
the committed version.
--
Peter Geoghegan