Horton
Menu

Re: Buffer locking is special (hints, checksums, AIO writes)

Antonin Houska <ah@cybertec.at>

View thread
From: Antonin Houska <ah@cybertec.at>
To: Andres Freund <andres@anarazel.de>
Cc: Kirill Reshke <reshkekirill@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Melanie Plageman <melanieplageman@gmail.com>, Matthias van de Meent <boekewurm+postgres@gmail.com>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Noah Misch <noah@leadboat.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael.paquier@gmail.com>
Date: 2026-02-10T07:46:27Z
Lists: pgsql-hackers 
Andres Freund <andres@anarazel.de> wrote:

> > While troubleshooting REPACK issue [1], I realized that
> > HeapTupleSatisfiesMVCCBatch() can also be called during logical decoding - in
> > that case we need to use a historic MVCC snapshot.
> 
> Huh. Indeed. That's unintentional - the path should never have been reached,
> we are checking that an MVCC snapshot is used. Unfortunately, somebody
> (i.e. probably me) at some point defined the relevant macro as
> 
> /* This macro encodes the knowledge of which snapshots are MVCC-safe */
> #define IsMVCCSnapshot(snapshot)  \
> 	((snapshot)->snapshot_type == SNAPSHOT_MVCC || \
> 	 (snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC)
> 
> Which makes sense for some places, but not for plenty others.
> 
> The reason this didn't cause more widespread issues is that during logical
> decoding we mostly don't use sequential scans etc that are affected by the
> these paths.

> > My proposal to fix the problem is attached.
> 
> That's imo not at all the right fix - it'd make visibility during seqscans
> checking noticeably slower.

ok

> I think we ought to instead restrict the page-at-a-time scans to only happen
> with "real" mvcc snapshots. I.e. this:
> 
> 	/*
> 	 * Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
> 	 */
> 	if (!(snapshot && IsMVCCSnapshot(snapshot)))
> 		scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;
>
> should trigger for historic snapshots as well.

I suppose you mean changing it to

	if (!(snapshot && IsMVCCSnapshot(snapshot) &&
		  !IsHistoricMVCCSnapshot(snapshot)))
		scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;

> Does that fix the issue for you?

Yes, with this change, I don't hit the problem anymore.

> What's your reproducer?

Check out this branch

https://github.com/michail-nikolaev/postgres/tree/repack_concurrently_repro_22

and run t/008_repack_concurrently.pl in contrib/amcheck. The error we saw in
most cases was "ERROR: cache lookup failed for relation". I noticed that the
related pg_class entries had hint bits set incorrectly, so I added the
following to see when exactly it happens (actually I used lower elevel first,
to find out that the decoding worker is responsible for the problem):

diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 75ae268d753..ebf38460873 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -73,6 +73,7 @@
 #include "access/transam.h"
 #include "access/xact.h"
 #include "access/xlog.h"
+#include "commands/cluster.h"
 #include "storage/bufmgr.h"
 #include "storage/procarray.h"
 #include "utils/builtins.h"
@@ -938,6 +939,8 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
                                                HeapTupleHeaderGetRawXmin(tuple));
                else
                {
+                       if (am_decoding_for_repack())
+                               elog(PANIC, "HEAP_XMIN_INVALID set");
                        /* it must have aborted or crashed */
                        SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
                                                InvalidTransactionId);

The backtrace looked like:

    #3  0x0000000000c367ad errfinish (postgres + 0x8367ad)
    #4  0x0000000000517ee1 HeapTupleSatisfiesMVCC (postgres + 0x117ee1)
    #5  0x0000000000518e14 HeapTupleSatisfiesMVCCBatch (postgres + 0x118e14)
    #6  0x000000000050483e page_collect_tuples (postgres + 0x10483e)
    #7  0x0000000000504a8e heap_prepare_pagescan (postgres + 0x104a8e)
    #8  0x0000000000505344 heapgettup_pagemode (postgres + 0x105344)
    #9  0x0000000000505cff heap_getnextslot (postgres + 0x105cff)
    #10 0x000000000052ca08 table_scan_getnextslot (postgres + 0x12ca08)
    #11 0x000000000052d4ed systable_getnext (postgres + 0x12d4ed)
    #12 0x0000000000c2098e ScanPgRelation (postgres + 0x82098e)
    #13 0x0000000000c23aa2 RelationReloadIndexInfo (postgres + 0x823aa2)
    #14 0x0000000000c24376 RelationRebuildRelation (postgres + 0x824376)
    #15 0x0000000000c23784 RelationIdGetRelation (postgres + 0x823784)
    #16 0x00000000004b558f relation_open (postgres + 0xb558f)
    #17 0x000000000052e073 index_open (postgres + 0x12e073)
    #18 0x000000000052d0c5 systable_beginscan (postgres + 0x12d0c5)
    #19 0x0000000000c2097e ScanPgRelation (postgres + 0x82097e)
    #20 0x0000000000c23dde RelationReloadNailed (postgres + 0x823dde)
    #21 0x0000000000c24399 RelationRebuildRelation (postgres + 0x824399)
    #22 0x0000000000c23784 RelationIdGetRelation (postgres + 0x823784)
    #23 0x00000000004b558f relation_open (postgres + 0xb558f)
    #24 0x0000000000573ab1 table_open (postgres + 0x173ab1)
    #25 0x0000000000c20919 ScanPgRelation (postgres + 0x820919)
    #26 0x0000000000c21c98 RelationBuildDesc (postgres + 0x821c98)
    #27 0x0000000000c237d6 RelationIdGetRelation (postgres + 0x8237d6)
    #28 0x00000000009a028a ReorderBufferProcessTXN (postgres + 0x5a028a)
    #29 0x00000000009a10ed ReorderBufferReplay (postgres + 0x5a10ed)
    #30 0x00000000009a116b ReorderBufferCommit (postgres + 0x5a116b)
    #31 0x000000000098c7fe DecodeCommit (postgres + 0x58c7fe)
    #32 0x000000000098ba67 xact_decode (postgres + 0x58ba67)
    #33 0x000000000098b685 LogicalDecodingProcessRecord (postgres + 0x58b685)
    #34 0x00000000006a3e4b decode_concurrent_changes (postgres + 0x2a3e4b)
    #35 0x00000000006a5ea3 repack_worker_internal (postgres + 0x2a5ea3)
    #36 0x00000000006a5d5e RepackWorkerMain (postgres + 0x2a5d5e)
    #37 0x0000000000961b2b BackgroundWorkerMain (postgres + 0x561b2b)
    #38 0x0000000000964a61 postmaster_child_launch (postgres + 0x564a61)
    #39 0x000000000096b58d StartBackgroundWorker (postgres + 0x56b58d)
    #40 0x000000000096b80a maybe_start_bgworkers (postgres + 0x56b80a)
    #41 0x000000000096a5d9 LaunchMissingBackgroundProcesses (postgres + 0x56a5d9)
    #42 0x00000000009683fc ServerLoop (postgres + 0x5683fc)
    #43 0x0000000000967d37 PostmasterMain (postgres + 0x567d37)
    #44 0x0000000000813421 main (postgres + 0x413421)

Thanks.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. bufmgr: Fix ordering of checks in PinBuffer()

  2. Use UnlockReleaseBuffer() in more places

  3. bufmgr: Make UnlockReleaseBuffer() more efficient

  4. bufmgr: Don't copy pages while writing out

  5. Fix use of wrong variable in _hash_kill_items()

  6. Fix bug due to confusion about what IsMVCCSnapshot means

  7. bufmgr: Switch to standard order in MarkBufferDirtyHint()

  8. bufmgr: Remove the, now obsolete, BM_JUST_DIRTIED

  9. Require share-exclusive lock to set hint bits and to flush

  10. heapam: Don't mimic MarkBufferDirtyHint() in inplace updates

  11. bufmgr: Allow conditionally locking of already locked buffer

  12. bufmgr: Avoid spurious compiler warning after fcb9c977aa5

  13. lwlock: Remove ForEachLWLockHeldByMe

  14. lwlock: Remove support for disowned lwlwocks

  15. bufmgr: Implement buffer content locks independently of lwlocks

  16. bufmgr: Change BufferDesc.state to be a 64-bit atomic

  17. lwlock: Improve local variable name

  18. lwlock: Invert meaning of LW_FLAG_RELEASE_OK

  19. bufmgr: Make definitions related to buffer descriptor easier to modify

  20. heapam: Add batch mode mvcc check and use it in page mode

  21. freespace: Don't modify page without any lock

  22. heapam: Move logic to handle HEAP_MOVED into a helper function

  23. bufmgr: Optimize & harmonize LockBufHdr(), LWLockWaitListLock()

  24. bufmgr: Add one-entry cache for private refcount

  25. bufmgr: Separate keys for private refcount infrastructure

  26. Add pg_atomic_unlocked_write_u64

  27. Rename BUFFERPIN wait event class to BUFFER

  28. bufmgr: Turn BUFFER_LOCK_* into an enum

  29. lwlock: Fix, currently harmless, bug in LWLockWakeup()

  30. bufmgr: Use atomic sub for unpinning buffers

  31. bufmgr: Allow some buffer state modifications while holding header lock

  32. bufmgr: Fix valgrind checking for buffers pinned in StrategyGetBuffer()

  33. bufmgr: Don't lock buffer header in StrategyGetBuffer()

  34. bufmgr: fewer calls to BufferDescriptorGetContentLock

  35. bufmgr: Fix signedness of mask variable in BufferSync()

  36. bufmgr: Introduce FlushUnlockedBuffer

  37. Improve ReadRecentBuffer() scalability