Thread

1. Re: [PATCH] pg_surgery: Fix WAL corruption from concurrent heap_force_kill

   Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2026-05-05T15:11:54Z

   On Mon, Apr 20, 2026 at 5:20 PM Fabrízio de Royes Mello <
   fabriziomello@gmail.com> wrote:
   >
   > Hi hackers,
   >
   > I found a bug in pg_surgery's heap_force_kill that can produce WAL
   records with incorrect CRC checksums when multiple sessions operate on heap
   pages that share the same visibility map page.
   >
   > When heap_force_kill kills a tuple on an all-visible page, it calls
   visibilitymap_clear() to clear the VM bits, then later calls
   log_newpage_buffer(vmbuf) to write a full-page image of the VM page to WAL.
   However, visibilitymap_clear() releases the exclusive lock on the VM buffer
   before returning -- that's its normal API contract. The subsequent
   log_newpage_buffer(vmbuf) then writes the FPI without holding any content
   lock on the VM buffer.
   >
   > XLogInsert reads the page content twice via the XLogRecData chain pointer
   to shared memory: once to compute the CRC in XLogRecordAssemble, and once
   to copy the data to WAL buffers in CopyXLogRecordToWAL. If a concurrent
   backend modifies the VM page between those two reads (e.g., another
   heap_force_kill session clearing a different bit on the same VM page), the
   CRC will not match the written bytes.
   >
   > Fix it by re-acquiring the VM buffer exclusive lock immediately after
   visibilitymap_clear() returns, and release it after log_newpage_buffer()
   completes. The lock is only acquired when RelationNeedsWAL() is true, since
   unlogged relations skip the FPI write entirely.
   >
   > The patch includes a TAP test that uses injection points to
   deterministically reproduce the race condition. Three injection points are
   used:
   > * "heap-force-kill-vm-pin" in heap_surgery.c (outside the critical
   section) to initialize DSM shared memory for the wait machinery.
   > * "heap-force-kill-before-vm-wal" in heap_surgery.c (inside the critical
   section, between the heap FPI and VM FPI writes) as a synchronization
   barrier.
   > * "wal-insert-after-crc" in xloginsert.c (inside XLogInsert, between
   XLogRecordAssemble and XLogInsertRecord) to pause after CRC computation but
   before the data copy.
   >
   > The test pauses session 1 inside XLogInsert after the VM FPI's CRC is
   computed, runs a concurrent heap_force_kill from session 2 on the same VM
   page, then wakes session 1. Without the fix, session 2 modifies the VM page
   between CRC and copy, and pg_walinspect reports "incorrect resource manager
   data checksum". With the fix, session 2 blocks on the VM buffer lock and no
   corruption occurs.
   >
   > The "wal-insert-after-crc" injection point in xloginsert.c uses
   INJECTION_POINT_CACHED (pre-loaded by the caller before the critical
   section), so it only fires when explicitly loaded and adds no overhead to
   the normal WAL write path.
   >
   
   Rebased version.
   
   --
   Fabrízio de Royes Mello
   

2. Re: [PATCH] pg_surgery: Fix WAL corruption from concurrent heap_force_kill

   Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-05T20:42:43Z

   Hello!
   
   I verified both the patch, and that the test case catches the bug
   without the patch, the overall change seems good to me.
   
   + if (did_modify_vm)
   + INJECTION_POINT_CACHED("heap-force-kill-before-vm-wal", NULL);
   +
     if (did_modify_vm && RelationNeedsWAL(rel))
   + {
     log_newpage_buffer(vmbuf, false);
   + LockBuffer(vmbuf, BUFFER_LOCK_UNLOCK);
   + }
   
   Is the additional if intentional here? Based on the name it seems like
   it could be simply part of the next if, currently it also fires for
   unlogged tables.
   
   +# Give session 2 time to reach the VM buffer lock (or complete if
   +# unfixed).  We cannot reliably detect blocking from Perl, so just
   +# sleep briefly.
   +use Time::HiRes qw(usleep);
   +usleep(500_000);
   
   Wouldn't this be possibly unstable on CI?
   
   The following seems to work for me, both for detecting the issue in
   the unpatched version and to result in a quicker continuation in the
   patched version: (~1.55sec total execution time compared to ~2sec
   original)
   
   (with the patch, s2 wait for the loc, without the patch it finishes
   and becomes idle)
   
   my $s2 = $node->background_psql('postgres');
   my $s2_pid = $s2->query_safe(q{SELECT pg_backend_pid()});
   chomp $s2_pid;
   
   $s2->query_until(qr/starting_s2/,
       q(\echo starting_s2
   SELECT heap_force_kill('test_vm'::regclass, ARRAY['(1,1)']::tid[]);
   \echo s2_done
   ));
   
   use Time::HiRes qw(usleep);
   my $observed = '';
   my $deadline = time() + 10;
   while (time() < $deadline) {
       $observed = $node->safe_psql('postgres', qq{
           SELECT format('%s/%s/%s',
                         coalesce(wait_event_type, 'NULL'),
                         coalesce(wait_event, 'NULL'),
                         coalesce(state, 'NULL'))
           FROM pg_stat_activity WHERE pid = $s2_pid
       });
       last if $observed =~ m{^Buffer/BufferExclusive/}
            || $observed =~ m{/idle$};
       usleep(50_000);
   }
   
   
   
   

3. Re: [PATCH] pg_surgery: Fix WAL corruption from concurrent heap_force_kill

   Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2026-05-06T20:27:25Z

   On Tue, May 5, 2026 at 5:42 PM Zsolt Parragi <zsolt.parragi@percona.com>
   wrote:
   >
   > Hello!
   >
   > I verified both the patch, and that the test case catches the bug
   > without the patch, the overall change seems good to me.
   >
   
   Thanks for your review.
   
   > + if (did_modify_vm)
   > + INJECTION_POINT_CACHED("heap-force-kill-before-vm-wal", NULL);
   > +
   >   if (did_modify_vm && RelationNeedsWAL(rel))
   > + {
   >   log_newpage_buffer(vmbuf, false);
   > + LockBuffer(vmbuf, BUFFER_LOCK_UNLOCK);
   > + }
   >
   > Is the additional if intentional here? Based on the name it seems like
   > it could be simply part of the next if, currently it also fires for
   > unlogged tables.
   >
   
   My bad it is a leftover. Fixed.
   
   > +# Give session 2 time to reach the VM buffer lock (or complete if
   > +# unfixed).  We cannot reliably detect blocking from Perl, so just
   > +# sleep briefly.
   > +use Time::HiRes qw(usleep);
   > +usleep(500_000);
   >
   > Wouldn't this be possibly unstable on CI?
   >
   
   Yes it should be somewhat flaky. TBH, I'm not sure if the test will be
   pushed with the fix, but anyway let's improve it.
   
   > The following seems to work for me, both for detecting the issue in
   > the unpatched version and to result in a quicker continuation in the
   > patched version: (~1.55sec total execution time compared to ~2sec
   > original)
   >
   > (with the patch, s2 wait for the loc, without the patch it finishes
   > and becomes idle)
   >
   > my $s2 = $node->background_psql('postgres');
   > my $s2_pid = $s2->query_safe(q{SELECT pg_backend_pid()});
   > chomp $s2_pid;
   >
   > $s2->query_until(qr/starting_s2/,
   >     q(\echo starting_s2
   > SELECT heap_force_kill('test_vm'::regclass, ARRAY['(1,1)']::tid[]);
   > \echo s2_done
   > ));
   >
   > use Time::HiRes qw(usleep);
   > my $observed = '';
   > my $deadline = time() + 10;
   > while (time() < $deadline) {
   >     $observed = $node->safe_psql('postgres', qq{
   >         SELECT format('%s/%s/%s',
   >                       coalesce(wait_event_type, 'NULL'),
   >                       coalesce(wait_event, 'NULL'),
   >                       coalesce(state, 'NULL'))
   >         FROM pg_stat_activity WHERE pid = $s2_pid
   >     });
   >     last if $observed =~ m{^Buffer/BufferExclusive/}
   >          || $observed =~ m{/idle$};
   >     usleep(50_000);
   > }
   
   Thanks, see the newly attached version (v2) with proposed changes.
   
   Regards,
   
   -- 
   Fabrízio de Royes Mello
   On behalf of PlanetScale
   

4. Re: [PATCH] pg_surgery: Fix WAL corruption from concurrent heap_force_kill

   Melanie Plageman <melanieplageman@gmail.com> — 2026-05-06T23:54:36Z

   On Wed, May 6, 2026 at 4:27 PM Fabrízio de Royes Mello
   <fabriziomello@gmail.com> wrote:
   >
   > On Tue, May 5, 2026 at 5:42 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
   > >
   > > I verified both the patch, and that the test case catches the bug
   > > without the patch, the overall change seems good to me.
   
   I haven't reviewed the test, but I think heap_force_common() has
   bigger problems than what your fix fixes. Definitely the lock on the
   vmbuffer needs to be held not just while clearing the VM but also
   while emitting WAL for those changes. But, actually, even more than
   that, the changes to the heap and the accompanying changes to the VM
   page should be logged in the same WAL record. It is forbidden for
   PD_ALL_VISIBLE to be clear while the VM is set -- which is the state
   we are in after replaying the WAL record clearing PD_ALL_VISIBLE
   before replaying the one clearing the VM bits. You need all the
   changes to happen atomically in one record.
   
   This is briefly discussed as part of a larger bug fix I am working on
   to correctly WAL log VM clears [1]. I propose a fix there that emits a
   single WAL record for the heap and VM changes while holding a lock on
   both pages.
   
   - Melanie
   
   [1] https://www.postgresql.org/message-id/flat/CAAKRu_Z_KAAZAHtuorifR2MRc_MkEcHf-C_t4b9HZaHpa3nriw%40mail.gmail.com#61d104a35fc95fa67708c060ec61d59b