Horton

Thread

  1. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-04T13:24:49Z

    Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> On Mon, Apr 27, 2026 at 6:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Alvaro, others, what is your take on this?
> 
> I agree with you here - we should AT LEAST make that an ERROR instead
> of an assert and also check it during cache access (not only during
> the scan because of cache misses).
> But I think it will still be fragile in case of some extensions installed.
> 
> Anyway... We also have an issue with correctness right now.
> 
> I took the old stress test from [0] (the first two) and it fails now,
> even with the fix from [1] ("Possible premature SNAPBUILD_CONSISTENT
> with DB-specific running_xacts").
> 
> It looks like [1] fixes 008_repack_concurrently.pl, but
> 007_repack_concurrently.pl fails anyway, including
> 
>      pgbench: error: client 1 script 0 aborted in command 10 query 0:
> ERROR:  could not create unique index "tbl_pkey_repacknew"
>      # DETAIL:  Key (i)=(383) is duplicated.
> and
>      'pgbench: error: pgbench:client 23 script 0 aborted in command 31
> query 0: ERROR:  division by zero
> 
> Last one is not MVCC-related; you can see from the logs that it
> performs something like SELECT (509063) / 0 when the table sum
> changes.
> 
> Setting need_shared_catalogs = true make them pass, so something is
> wrong with its correctness.

Thanks for testing again. Whether we keep the "database specific slots" or
not, it'd be good to know what exactly the reason of these errors is. I wonder
if the feature just exposes a problem that remains shadowed otherwise, due to
the contention on replication slot. I'm going to investigate.

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

  2. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-05T12:47:46Z

    Antonin Houska <ah@cybertec.at> wrote:

> Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:
> 
> > On Mon, Apr 27, 2026 at 6:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Alvaro, others, what is your take on this?
> > 
> > I agree with you here - we should AT LEAST make that an ERROR instead
> > of an assert and also check it during cache access (not only during
> > the scan because of cache misses).
> > But I think it will still be fragile in case of some extensions installed.
> > 
> > Anyway... We also have an issue with correctness right now.
> > 
> > I took the old stress test from [0] (the first two) and it fails now,
> > even with the fix from [1] ("Possible premature SNAPBUILD_CONSISTENT
> > with DB-specific running_xacts").
> > 
> > It looks like [1] fixes 008_repack_concurrently.pl, but
> > 007_repack_concurrently.pl fails anyway, including
> > 
> >      pgbench: error: client 1 script 0 aborted in command 10 query 0:
> > ERROR:  could not create unique index "tbl_pkey_repacknew"
> >      # DETAIL:  Key (i)=(383) is duplicated.
> > and
> >      'pgbench: error: pgbench:client 23 script 0 aborted in command 31
> > query 0: ERROR:  division by zero
> > 
> > Last one is not MVCC-related; you can see from the logs that it
> > performs something like SELECT (509063) / 0 when the table sum
> > changes.
> > 
> > Setting need_shared_catalogs = true make them pass, so something is
> > wrong with its correctness.
> 
> Thanks for testing again. Whether we keep the "database specific slots" or
> not, it'd be good to know what exactly the reason of these errors is. I wonder
> if the feature just exposes a problem that remains shadowed otherwise, due to
> the contention on replication slot. I'm going to investigate.

I think the problem is that with database-specific snapshot,
SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin

	/*
	 * Database specific transaction info may exist to reach CONSISTENT state
	 * faster, however the code below makes no use of it. Moreover, such
	 * record might cause problems because the following normal (cluster-wide)
	 * record can have lower value of oldestRunningXid. In that case, let's
	 * wait with the cleanup for the next regular cluster-wide record.
	 */
	if (OidIsValid(running->dbid))
		return;

and thus some transactions whose XID is below running->oldestRunningXid may
continue to be incorrectly considered running.

I originally thought that this should not happen because such transactions
will be added to the builder's array of committed transactions by
SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
a transaction listed in the xl_running_xacts WAL record is not guaranteed to
follow the xl_running_xacts record in WAL. In other words, even if
xl_running_xacts is created before a COMMIT record of the contained
transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
might not take place.

I've got no good idea how to fix that. Not sure I'm able to pursue the
"database-specific snapshots" feature now.

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

  3. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-05T15:02:39Z

    On 2026-May-05, Antonin Houska wrote:

> However, I failed to notice that COMMIT record of
> a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> follow the xl_running_xacts record in WAL. In other words, even if
> xl_running_xacts is created before a COMMIT record of the contained
> transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> might not take place.

That's pretty bad news.

> I've got no good idea how to fix that. Not sure I'm able to pursue the
> "database-specific snapshots" feature now.

It appears that the only reasonable course of action at this point is to
revert 0d3dba38c777.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  4. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-05T15:04:42Z

    On 2026-Apr-30, Mihail Nikalayeu wrote:

> P.S.
> I think it is good idea to add these stress tests to the source tree,
> perhaps with some kind PG_TEST_EXTRA=stress (as done in [1]).

Yeah, agreed.  I'll see to that shortly.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
              https://postgr.es/m/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

  5. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-06T08:25:44Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2026-May-05, Antonin Houska wrote:
> 
> > However, I failed to notice that COMMIT record of
> > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > follow the xl_running_xacts record in WAL. In other words, even if
> > xl_running_xacts is created before a COMMIT record of the contained
> > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > might not take place.
> 
> That's pretty bad news.
> 
> > I've got no good idea how to fix that.

One idea occurred to me yet, effectively it's just a cleanup. Part of it was
already proposed [1].

[1] https://www.postgresql.org/message-id/flat/CAHg%2BQDcQak4jx_6X2_Ws98rzG%3DxBARLjqm_%3D56wTRUtNsY4DZQ%40mail.gmail.com

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

  6. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-08T12:25:12Z

    On Wed, May 6, 2026 at 1:55 PM Antonin Houska <ah@cybertec.at> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > On 2026-May-05, Antonin Houska wrote:
> >
> > > However, I failed to notice that COMMIT record of
> > > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > > follow the xl_running_xacts record in WAL. In other words, even if
> > > xl_running_xacts is created before a COMMIT record of the contained
> > > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > > might not take place.
> >
> > That's pretty bad news.
> >
> > > I've got no good idea how to fix that.
>
> One idea occurred to me yet, effectively it's just a cleanup. Part of it was
> already proposed [1].
>

Some issues/inefficiencies regarding this fix and base code related to
db-specific snapshots built during decoding:
*
After fix, whenever a db-specific decoder sees a cluster-wide
xl_running_xacts record, it unconditionally calls
LogStandbySnapshot(MyDatabaseId) and returns. This triggers for every
cluster-wide record the decoder encounters (including post snapbuild's
CONSISTENT state) , for the entire duration of the decoding session.
LogStandbySnapshot acquires ProcArrayLock + XidGenLock, calls
GetRunningTransactionData, and writes WAL. With N active db-specific
decoding sessions, each cluster-wide record now triggers N additional
WAL writes.

Additionally, LogStandbySnapshot also logs AccessExclusiveLocks before
the running_xacts record. Physical standbys skip db-specific
XLOG_RUNNING_XACTS records via standby_redo(), but they do process the
preceding XLOG_STANDBY_LOCK records. The same locks may already have
been logged in the most recent cluster-wide snapshot. Physical
standbys could end up processing these lock records twice which may
not be harmful because I think we avoid re-acquiring the lock but
still it is a new overhead in the system.

*
When a cluster-wide running_xacts record arrives:
SnapBuildProcessRunningXacts calls LogStandbySnapshot and returns
early. ReorderBufferAbortOld is called, but with the cluster-wide
oldestRunningXid, which could lag far behind the db-specific value
(due to a long-running transaction in another database).
When a db-specific record arrives: SnapBuildProcessRunningXacts
processes it and advances builder->xmin with the db-specific (more
current) oldestRunningXid. But ReorderBufferAbortOld is NOT called for
db-specific records. This means the reorder buffer is cleaned up using
a conservative, potentially very old, cluster-wide oldestRunningXid,
even though builder->xmin has already advanced much further. The
reorder buffer holds stale entries longer than necessary, increasing
memory pressure.

*
I also see a design level problem with plugins that have
need_shared_catalogs=false and use failover slots. IIUC, the
db-specific optimization was designed around a live decoding session
on the primary which can emit and immediately read its own db-specific
records in the WAL stream to reach consistent state. The
LogicalSlotAdvanceAndCheckSnapState path used by slotsync has a
bounded WAL window and cannot exploit the feedback loop, making the
two mechanisms fundamentally incompatible. I know the slot created by
pgrepack doesn't enable failover option but we have not even added any
guards or thought about db-specific snapbuilds with other parts of the
system that rely on cluster-wide running_xact records, so there could
be more problems which we don't see with the current set of tests.

-- 
With Regards,
Amit Kapila.

  7. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-08T13:58:18Z

    On 2026-May-08, Amit Kapila wrote:

> On Wed, May 6, 2026 at 1:55 PM Antonin Houska <ah@cybertec.at> wrote:

> > One idea occurred to me yet, effectively it's just a cleanup. Part of it was
> > already proposed [1].

Hmm, I think this cleanup makes sense.  If I apply the test patches
(0001 and 0002 here), they fail almost immediately; but after applying
0003 all is again well.  I think these tests are a good thing to have in
the tree, even if we end up reverting db-specific snapshots later.

After some back and forth, I modified the tests slightly so that
the search PG_TEST_EXTRA for the string "stress_concurrently=N".  The N
can be changed so that the tests run for longer; if not given, it's
taken as 1, and the tests run for around 6 seconds (so N=10 means runs
for a minute).  I think this is a convenient gadget for other tests of
this kind on CONCURRENTLY commands, such as the one proposed for CIC
elsewhere.

As written, these tests would run nowhere until we add that string in
some buildfarm animal.  I debated with myself whether to assume N=1 when
the string is not given.  I still think that's a good idea but perhaps
we should have something to prevent it from running by default when
under Valgrind or other slow things like that.  In normal conditions,
the total runtime is not affected when they are run with N=1 as part of
the whole test suite.

> Some issues/inefficiencies regarding this fix and base code related to
> db-specific snapshots built during decoding: [...]

Thanks for spending time reviewing this code.  I think none of these
problems are fundamental in nature, but they are obviously worth
addressing for 19.  If we hit some roadblock, we can still revert only
db-specific snapshots.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

  8. Re: Adding REPACK [concurrently]

    Masahiko Sawada <sawada.mshk@gmail.com> — 2026-05-08T23:22:15Z

    On Fri, May 8, 2026 at 6:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2026-May-08, Amit Kapila wrote:
>
> > On Wed, May 6, 2026 at 1:55 PM Antonin Houska <ah@cybertec.at> wrote:
>
> > Some issues/inefficiencies regarding this fix and base code related to
> > db-specific snapshots built during decoding: [...]
>
> Thanks for spending time reviewing this code.  I think none of these
> problems are fundamental in nature, but they are obviously worth
> addressing for 19.  If we hit some roadblock, we can still revert only
> db-specific snapshots.
>

I'm still studying REPACK (CONCURRENTLY), and I have a question about
db-specific decoder; as far as I read the codes, any decoding plugin
can use db-specific decoder by setting need_shared_catalog=false but
should we prevent such slots from being created on standbys? I'm a bit
concerned that plugin developers inadvertently set
need_shared_catalog=false in the startup callback and users would
create such slots on standbys.  For instance, if we create a logical
slot with pgrepack plugin on the standby, we would get an assertion
failure as the db-specific decoder tries to call LogStandbySnapshot()
via SnapBuildProcessRunningXacts(). Even if we add
!RecoveryInProgress() check there, the db-specific decoder on standbys
requires for the primary server to run a REPACK (CONCURRENTLY).

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  9. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-10T11:24:29Z

    On Fri, May 8, 2026 at 7:28 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2026-May-08, Amit Kapila wrote:
>
> > Some issues/inefficiencies regarding this fix and base code related to
> > db-specific snapshots built during decoding: [...]
>
> Thanks for spending time reviewing this code.  I think none of these
> problems are fundamental in nature, but they are obviously worth
> addressing for 19.  If we hit some roadblock, we can still revert only
> db-specific snapshots.
>

IIUC, the emails by Andres [1][2] on db-specific snapshots sound like
concerns which are fundamental in nature. Apart from that as well, I
think the first point mentioned in my email [3] should be at least
addressed as that causes additional WAL even after reaching
consistent_state for each runing_xact record for a db-specific
decoder.

[1] - https://www.postgresql.org/message-id/cdgw4sbbfcgk6du3iv54r2dgiy4tfywoklbotlmj4irxavdcr3%40glxfw5jj277q
[2] - https://www.postgresql.org/message-id/pveffyxhnuurhb44uzqlwo3rkyzorkfh2rot7uwzlf2axhfvbp%407nrs2omysxkc
[3] - https://www.postgresql.org/message-id/CAA4eK1LygCDP3FiFzXY9iVNFcHxhf7TT_DFf7tryTu2oipmfpA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  10. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-10T11:31:04Z

    On Tue, May 5, 2026 at 6:17 PM Antonin Houska <ah@cybertec.at> wrote:
>
> Antonin Houska <ah@cybertec.at> wrote:
>
> I think the problem is that with database-specific snapshot,
> SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin
>
>         /*
>          * Database specific transaction info may exist to reach CONSISTENT state
>          * faster, however the code below makes no use of it. Moreover, such
>          * record might cause problems because the following normal (cluster-wide)
>          * record can have lower value of oldestRunningXid. In that case, let's
>          * wait with the cleanup for the next regular cluster-wide record.
>          */
>         if (OidIsValid(running->dbid))
>                 return;
>
> and thus some transactions whose XID is below running->oldestRunningXid may
> continue to be incorrectly considered running.
>
> I originally thought that this should not happen because such transactions
> will be added to the builder's array of committed transactions by
> SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
> a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> follow the xl_running_xacts record in WAL. In other words, even if
> xl_running_xacts is created before a COMMIT record of the contained
> transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> might not take place.
>

BTW, is it possible to write a test by using injection_points or via
manual steps (by using debugger, etc) so that we can more clearly
understand this problem and proposed fix?

-- 
With Regards,
Amit Kapila.

  11. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-11T15:17:52Z

    Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Tue, May 5, 2026 at 6:17 PM Antonin Houska <ah@cybertec.at> wrote:
> >
> > Antonin Houska <ah@cybertec.at> wrote:
> >
> > I think the problem is that with database-specific snapshot,
> > SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin
> >
> >         /*
> >          * Database specific transaction info may exist to reach CONSISTENT state
> >          * faster, however the code below makes no use of it. Moreover, such
> >          * record might cause problems because the following normal (cluster-wide)
> >          * record can have lower value of oldestRunningXid. In that case, let's
> >          * wait with the cleanup for the next regular cluster-wide record.
> >          */
> >         if (OidIsValid(running->dbid))
> >                 return;
> >
> > and thus some transactions whose XID is below running->oldestRunningXid may
> > continue to be incorrectly considered running.
> >
> > I originally thought that this should not happen because such transactions
> > will be added to the builder's array of committed transactions by
> > SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
> > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > follow the xl_running_xacts record in WAL. In other words, even if
> > xl_running_xacts is created before a COMMIT record of the contained
> > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > might not take place.
> >
> 
> BTW, is it possible to write a test by using injection_points or via
> manual steps (by using debugger, etc) so that we can more clearly
> understand this problem and proposed fix?

So far I could observe the situation in WAL, but have no idea how it can
happen. For example, transaction 49242 gets committed here

rmgr: Transaction len (rec/tot): 46/ 46, tx: 49242, lsn: 0/18BC28C8, prev
0/18BC2890, desc: COMMIT 2026-05-11 16:38:16.603265 CEST

and then it appears in the 'xids' list of RUNNING_XACTS:

rmgr: Standby     len (rec/tot):    106/   106, tx:          0, lsn:
0/18BC3140, prev 0/18BC3100, desc: RUNNING_XACTS nextXid 49255
latestCompletedXid 49241 oldestRunningXid 49242; 13 xacts: 49248 49249 49246
49243 49252 49251 49244 49245 49242 49250 49253 49254 49247; dbid:5


I thought the situation is quite common (and therefore nothing of
SnapBuildProcessRunningXacts() should be skipped), but when trying to
reproduce the problem, I noticed that LogStandbySnapshot() shouldn't allow
that ordering issue when logical decoding is enabled:

	/*
	 * GetRunningTransactionData() acquired ProcArrayLock, we must release it.
	 * For Hot Standby this can be done before inserting the WAL record
	 * because ProcArrayApplyRecoveryInfo() rechecks the commit status using
	 * the clog. For logical decoding, though, the lock can't be released
	 * early because the clog might be "in the future" from the POV of the
	 * historic snapshot. This would allow for situations where we're waiting
	 * for the end of a transaction listed in the xl_running_xacts record
	 * which, according to the WAL, has committed before the xl_running_xacts
	 * record. Fortunately this routine isn't executed frequently, and it's
	 * only a shared lock.
	 */
	if (!logical_decoding_enabled)
		LWLockRelease(ProcArrayLock);

So I don't have the answer right now.

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

  12. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-11T19:30:01Z

    Antonin Houska <ah@cybertec.at> wrote:

> Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> > On Tue, May 5, 2026 at 6:17 PM Antonin Houska <ah@cybertec.at> wrote:
> > >
> > > Antonin Houska <ah@cybertec.at> wrote:
> > >
> > > I think the problem is that with database-specific snapshot,
> > > SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin
> > >
> > >         /*
> > >          * Database specific transaction info may exist to reach CONSISTENT state
> > >          * faster, however the code below makes no use of it. Moreover, such
> > >          * record might cause problems because the following normal (cluster-wide)
> > >          * record can have lower value of oldestRunningXid. In that case, let's
> > >          * wait with the cleanup for the next regular cluster-wide record.
> > >          */
> > >         if (OidIsValid(running->dbid))
> > >                 return;
> > >
> > > and thus some transactions whose XID is below running->oldestRunningXid may
> > > continue to be incorrectly considered running.
> > >
> > > I originally thought that this should not happen because such transactions
> > > will be added to the builder's array of committed transactions by
> > > SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
> > > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > > follow the xl_running_xacts record in WAL. In other words, even if
> > > xl_running_xacts is created before a COMMIT record of the contained
> > > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > > might not take place.
> > >
> > 
> > BTW, is it possible to write a test by using injection_points or via
> > manual steps (by using debugger, etc) so that we can more clearly
> > understand this problem and proposed fix?
> 
> So far I could observe the situation in WAL, but have no idea how it can
> happen. For example, transaction 49242 gets committed here
> 
> rmgr: Transaction len (rec/tot): 46/ 46, tx: 49242, lsn: 0/18BC28C8, prev
> 0/18BC2890, desc: COMMIT 2026-05-11 16:38:16.603265 CEST
> 
> and then it appears in the 'xids' list of RUNNING_XACTS:
> 
> rmgr: Standby     len (rec/tot):    106/   106, tx:          0, lsn:
> 0/18BC3140, prev 0/18BC3100, desc: RUNNING_XACTS nextXid 49255
> latestCompletedXid 49241 oldestRunningXid 49242; 13 xacts: 49248 49249 49246
> 49243 49252 49251 49244 49245 49242 49250 49253 49254 49247; dbid:5
> 
> 
> I thought the situation is quite common (and therefore nothing of
> SnapBuildProcessRunningXacts() should be skipped), but when trying to
> reproduce the problem, I noticed that LogStandbySnapshot() shouldn't allow
> that ordering issue when logical decoding is enabled:
> 
> 	/*
> 	 * GetRunningTransactionData() acquired ProcArrayLock, we must release it.
> 	 * For Hot Standby this can be done before inserting the WAL record
> 	 * because ProcArrayApplyRecoveryInfo() rechecks the commit status using
> 	 * the clog. For logical decoding, though, the lock can't be released
> 	 * early because the clog might be "in the future" from the POV of the
> 	 * historic snapshot. This would allow for situations where we're waiting
> 	 * for the end of a transaction listed in the xl_running_xacts record
> 	 * which, according to the WAL, has committed before the xl_running_xacts
> 	 * record. Fortunately this routine isn't executed frequently, and it's
> 	 * only a shared lock.
> 	 */
> 	if (!logical_decoding_enabled)
> 		LWLockRelease(ProcArrayLock);
> 
> So I don't have the answer right now.

I think now that "waiting for the end of a transaction listed in the
xl_running_xacts record" in the comment is about transaction removal from
procarray. However, the COMMIT record can still be ahead of xl_running_xacts
because RecordTransactionCommit() is called before
ProcArrayEndTransaction(). I'll think again if the whole problem can be
reproduced with injection points.

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

  13. RE: Adding REPACK [concurrently]

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-05-12T02:25:47Z

    Dear Antonin,

FYI, I have also been spending time to reproduce the failure but I have not done yet.

> So far I could observe the situation in WAL, but have no idea how it can
> happen.

Not sure it matches with your situation, but I could reproduce the situation by
using gdb.

0. initialized an instance with wal_level=logical and defined a table.
1. established a connection
2. attached the backend via gdb
3. added a breakpoint in ProcArrayEndTransaction
4. committed a transaction, and it would stop at the breakpoint
5. established another connection
6. ran REPACK CONCURRENTLY
7. detached from the first backend.
8. all commands would finish.

This could allow that COMMIT record exists ahead the RUNNING_XACTS record.
When the backend reaches CommitTransaction()->ProcArrayEndTransaction(), the commit
record has already been serialized, but the transaction is still marked as active
on the proc array. Above workload allowed that repack worker could check in-between.

[1]:
```
rmgr: Transaction len (rec/tot):     46/    46, tx:        695, lsn: 0/018B89C0, prev 0/018B8980, desc: COMMIT 2026-05-12 11:11:31.588061 JST
rmgr: Standby     len (rec/tot):     58/    58, tx:          0, lsn: 0/018B89F0, prev 0/018B89C0, desc: RUNNING_XACTS nextXid 696 latestCompletedXid 694 oldestRunningXid 695; 1 xacts: 695; dbid: 0
rmgr: Standby     len (rec/tot):     58/    58, tx:          0, lsn: 0/018B8A30, prev 0/018B89F0, desc: RUNNING_XACTS nextXid 696 latestCompletedXid 694 oldestRunningXid 695; 1 xacts: 695; dbid: 5
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  14. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-12T07:57:29Z

    On Tue, May 12, 2026 at 1:00 AM Antonin Houska <ah@cybertec.at> wrote:
>
> Antonin Houska <ah@cybertec.at> wrote:
>
> > Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > On Tue, May 5, 2026 at 6:17 PM Antonin Houska <ah@cybertec.at> wrote:
> > > >
> > > > Antonin Houska <ah@cybertec.at> wrote:
> > > >
> > > > I think the problem is that with database-specific snapshot,
> > > > SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin
> > > >
> > > >         /*
> > > >          * Database specific transaction info may exist to reach CONSISTENT state
> > > >          * faster, however the code below makes no use of it. Moreover, such
> > > >          * record might cause problems because the following normal (cluster-wide)
> > > >          * record can have lower value of oldestRunningXid. In that case, let's
> > > >          * wait with the cleanup for the next regular cluster-wide record.
> > > >          */
> > > >         if (OidIsValid(running->dbid))
> > > >                 return;
> > > >
> > > > and thus some transactions whose XID is below running->oldestRunningXid may
> > > > continue to be incorrectly considered running.
> > > >
> > > > I originally thought that this should not happen because such transactions
> > > > will be added to the builder's array of committed transactions by
> > > > SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
> > > > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > > > follow the xl_running_xacts record in WAL. In other words, even if
> > > > xl_running_xacts is created before a COMMIT record of the contained
> > > > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > > > might not take place.
> > > >
> > >
> > > BTW, is it possible to write a test by using injection_points or via
> > > manual steps (by using debugger, etc) so that we can more clearly
> > > understand this problem and proposed fix?
> >
> > So far I could observe the situation in WAL, but have no idea how it can
> > happen. For example, transaction 49242 gets committed here
> >
> > rmgr: Transaction len (rec/tot): 46/ 46, tx: 49242, lsn: 0/18BC28C8, prev
> > 0/18BC2890, desc: COMMIT 2026-05-11 16:38:16.603265 CEST
> >
> > and then it appears in the 'xids' list of RUNNING_XACTS:
> >
> > rmgr: Standby     len (rec/tot):    106/   106, tx:          0, lsn:
> > 0/18BC3140, prev 0/18BC3100, desc: RUNNING_XACTS nextXid 49255
> > latestCompletedXid 49241 oldestRunningXid 49242; 13 xacts: 49248 49249 49246
> > 49243 49252 49251 49244 49245 49242 49250 49253 49254 49247; dbid:5
> >
> >
> > I thought the situation is quite common (and therefore nothing of
> > SnapBuildProcessRunningXacts() should be skipped), but when trying to
> > reproduce the problem, I noticed that LogStandbySnapshot() shouldn't allow
> > that ordering issue when logical decoding is enabled:
> >
> >       /*
> >        * GetRunningTransactionData() acquired ProcArrayLock, we must release it.
> >        * For Hot Standby this can be done before inserting the WAL record
> >        * because ProcArrayApplyRecoveryInfo() rechecks the commit status using
> >        * the clog. For logical decoding, though, the lock can't be released
> >        * early because the clog might be "in the future" from the POV of the
> >        * historic snapshot. This would allow for situations where we're waiting
> >        * for the end of a transaction listed in the xl_running_xacts record
> >        * which, according to the WAL, has committed before the xl_running_xacts
> >        * record. Fortunately this routine isn't executed frequently, and it's
> >        * only a shared lock.
> >        */
> >       if (!logical_decoding_enabled)
> >               LWLockRelease(ProcArrayLock);
> >
> > So I don't have the answer right now.
>
> I think now that "waiting for the end of a transaction listed in the
> xl_running_xacts record" in the comment is about transaction removal from
> procarray. However, the COMMIT record can still be ahead of xl_running_xacts
> because RecordTransactionCommit() is called before
> ProcArrayEndTransaction().
>

I see your point. Due to this, once the xmin regresses based on
cluster-wide running_xact, some transaction could appear to be running
when it should have appeared as committed. However, still it is not
clear how it could lead to one update in the transaction as
successfully decoded and another one to be skipped. One theory could
be that before the second update, somehow invalidation happens and
when decoding tries to reload the catalog to decode second update, the
relation is not visible because xmin has regressed and the update is
somehow skipped. I can't see how it can happen in code but something
like that is happening. Assuming, the problematic case is something
like what I described, even than the fix of skipping cluster-wide
running xacts and instead LOG db-specific running_xacts to help
updating builder's xmin sounds inelegant and probably inefficient. For
example, I think such a dependency means we can never enable
db-specific snapshots on standby.

-- 
With Regards,
Amit Kapila.

  15. Re: Adding REPACK [concurrently]

    Antonin Houska <ah@cybertec.at> — 2026-05-12T11:08:20Z

    Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Tue, May 12, 2026 at 1:00 AM Antonin Houska <ah@cybertec.at> wrote:
> >
> > Antonin Houska <ah@cybertec.at> wrote:
> >
> > > Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > On Tue, May 5, 2026 at 6:17 PM Antonin Houska <ah@cybertec.at> wrote:
> > > > >
> > > > > Antonin Houska <ah@cybertec.at> wrote:
> > > > >
> > > > > I think the problem is that with database-specific snapshot,
> > > > > SnapBuildProcessRunningXacts() returns early, w/o adjusting builder->xmin
> > > > >
> > > > >         /*
> > > > >          * Database specific transaction info may exist to reach CONSISTENT state
> > > > >          * faster, however the code below makes no use of it. Moreover, such
> > > > >          * record might cause problems because the following normal (cluster-wide)
> > > > >          * record can have lower value of oldestRunningXid. In that case, let's
> > > > >          * wait with the cleanup for the next regular cluster-wide record.
> > > > >          */
> > > > >         if (OidIsValid(running->dbid))
> > > > >                 return;
> > > > >
> > > > > and thus some transactions whose XID is below running->oldestRunningXid may
> > > > > continue to be incorrectly considered running.
> > > > >
> > > > > I originally thought that this should not happen because such transactions
> > > > > will be added to the builder's array of committed transactions by
> > > > > SnapBuildCommitTxn() anyway. However, I failed to notice that COMMIT record of
> > > > > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > > > > follow the xl_running_xacts record in WAL. In other words, even if
> > > > > xl_running_xacts is created before a COMMIT record of the contained
> > > > > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > > > > might not take place.
> > > > >
> > > >
> > > > BTW, is it possible to write a test by using injection_points or via
> > > > manual steps (by using debugger, etc) so that we can more clearly
> > > > understand this problem and proposed fix?
> > >
> > > So far I could observe the situation in WAL, but have no idea how it can
> > > happen. For example, transaction 49242 gets committed here
> > >
> > > rmgr: Transaction len (rec/tot): 46/ 46, tx: 49242, lsn: 0/18BC28C8, prev
> > > 0/18BC2890, desc: COMMIT 2026-05-11 16:38:16.603265 CEST
> > >
> > > and then it appears in the 'xids' list of RUNNING_XACTS:
> > >
> > > rmgr: Standby     len (rec/tot):    106/   106, tx:          0, lsn:
> > > 0/18BC3140, prev 0/18BC3100, desc: RUNNING_XACTS nextXid 49255
> > > latestCompletedXid 49241 oldestRunningXid 49242; 13 xacts: 49248 49249 49246
> > > 49243 49252 49251 49244 49245 49242 49250 49253 49254 49247; dbid:5
> > >
> > >
> > > I thought the situation is quite common (and therefore nothing of
> > > SnapBuildProcessRunningXacts() should be skipped), but when trying to
> > > reproduce the problem, I noticed that LogStandbySnapshot() shouldn't allow
> > > that ordering issue when logical decoding is enabled:
> > >
> > >       /*
> > >        * GetRunningTransactionData() acquired ProcArrayLock, we must release it.
> > >        * For Hot Standby this can be done before inserting the WAL record
> > >        * because ProcArrayApplyRecoveryInfo() rechecks the commit status using
> > >        * the clog. For logical decoding, though, the lock can't be released
> > >        * early because the clog might be "in the future" from the POV of the
> > >        * historic snapshot. This would allow for situations where we're waiting
> > >        * for the end of a transaction listed in the xl_running_xacts record
> > >        * which, according to the WAL, has committed before the xl_running_xacts
> > >        * record. Fortunately this routine isn't executed frequently, and it's
> > >        * only a shared lock.
> > >        */
> > >       if (!logical_decoding_enabled)
> > >               LWLockRelease(ProcArrayLock);
> > >
> > > So I don't have the answer right now.
> >
> > I think now that "waiting for the end of a transaction listed in the
> > xl_running_xacts record" in the comment is about transaction removal from
> > procarray. However, the COMMIT record can still be ahead of xl_running_xacts
> > because RecordTransactionCommit() is called before
> > ProcArrayEndTransaction().
> >
> 
> I see your point. Due to this, once the xmin regresses based on
> cluster-wide running_xact, some transaction could appear to be running
> when it should have appeared as committed.

The problem is that xmin does not advance when it should. Attached is a test
that reproduces the problem (it includes [1], to handle injection points in
background worker), I hope the comments in the specification file are helpful.

It's actually not exactly the problem reported in the stress test, but IMO the
core issue is the same: effects of some transactions are lost. In the stress
test, tuple deletion was lost, so the error was "could not create unique
index". Here I only demonstrate lost INSERT.

> Assuming, the problematic case is something
> like what I described, even than the fix of skipping cluster-wide
> running xacts and instead LOG db-specific running_xacts to help
> updating builder's xmin sounds inelegant and probably inefficient. For
> example, I think such a dependency means we can never enable
> db-specific snapshots on standby.

ok

[1] https://www.postgresql.org/message-id/4703.1774250534%40localhost

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

  16. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-13T12:34:17Z

    On Tue, May 12, 2026 at 4:38 PM Antonin Houska <ah@cybertec.at> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> >
> > I see your point. Due to this, once the xmin regresses based on
> > cluster-wide running_xact, some transaction could appear to be running
> > when it should have appeared as committed.
>
> The problem is that xmin does not advance when it should. Attached is a test
> that reproduces the problem (it includes [1], to handle injection points in
> background worker), I hope the comments in the specification file are helpful.
>

Yes, the comments were helpful. IIUC, the test skipped insert into
repack_test because the transaction doing that insert happened before
the snapbuild reached FULL_SNAPSHOT/CONSISTENT state, so its commit is
not decoded. Then we also didn't update builder->xmin after reaching
CONSISTENT state in the last running_xact record for MyDatabase. So,
the insert is neither covered in initial copy, nor decoded, so after
repack (concurrently) is finished the table is empty.

I think the patch proposed will fix this specific issue but apart from
other points raised for this patch few more are: (a) Post CONSISTENT
state, for cleanup of db_specific snapshots, we need to separately
again LOG db-specific running xacts whenever we encounter another
running_xacts, (b) Other point is that because
repack-concurrently always use full-snapshot, the serialization of
snapshot is useless because it can't be used by others.

> It's actually not exactly the problem reported in the stress test, but IMO the
> core issue is the same: effects of some transactions are lost. In the stress
> test, tuple deletion was lost, so the error was "could not create unique
> index". Here I only demonstrate lost INSERT.
>
> > Assuming, the problematic case is something
> > like what I described, even than the fix of skipping cluster-wide
> > running xacts and instead LOG db-specific running_xacts to help
> > updating builder's xmin sounds inelegant and probably inefficient. For
> > example, I think such a dependency means we can never enable
> > db-specific snapshots on standby.
>
> ok
>

So now the question is where do we go from here. I am not confident
that the current code to achieve db-specific snapshots in logical
decoding is the best possible solution both because of the drawbacks
(like we won't be able to enable this on standby) and inefficiencies
pointed out by me in this and previous emails in this work.

-- 
With Regards,
Amit Kapila.

  17. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-13T16:58:14Z

    Hello Amit,

On 2026-May-13, Amit Kapila wrote:

> So now the question is where do we go from here. I am not confident
> that the current code to achieve db-specific snapshots in logical
> decoding is the best possible solution both because of the drawbacks
> (like we won't be able to enable this on standby) and inefficiencies
> pointed out by me in this and previous emails in this work.

This is a fair question.  I don't think we have time to go much further
on this aspect before beta 1, so we either accept this patch, fix the
inefficiencies you pointed out and keep db-specific snapshots, or we
revert db-specific snapshots and go back to the standard snapshot-taking
technique for REPACK in 19 and see what we can improve for 20.

Now, the worst consequence of reverting db-specific snapshots is that
you will only be able to run REPACK in a single database at a time
(because any subsequent REPACK will have to wait until the first one
finishes before being able to get its snapshot).  In most normal cases
this is probably not a big deal.  But if you have a multitenant system,
and you want your users to be able to run REPACK on their tables, you
may be a bit screwed.  So I hesitate to just go and revert it without
offering those people any alternative.

(It's also possible that being unable to run more than one REPACK at a
time is not so big a deal.  After all, it's supposed to be an infrequent
operation.  And users probably don't or shouldn't have multi-terabyte
tables in multitenant databases anyway.)

I'm not sure I understand the point of the standby.  I mean, you can't
run REPACK on the standby anyway, so I don't see this as a very
problematic restriction.  Do you have other reasons for wanting a
db-specific snapshot in a standby?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  18. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-14T07:02:25Z

    On Wed, May 13, 2026 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello Amit,
>
> On 2026-May-13, Amit Kapila wrote:
>
> > So now the question is where do we go from here. I am not confident
> > that the current code to achieve db-specific snapshots in logical
> > decoding is the best possible solution both because of the drawbacks
> > (like we won't be able to enable this on standby) and inefficiencies
> > pointed out by me in this and previous emails in this work.
>
> This is a fair question.  I don't think we have time to go much further
> on this aspect before beta 1, so we either accept this patch, fix the
> inefficiencies you pointed out and keep db-specific snapshots,
>

I don't think it would be easy to address these inefficiencies before
beta 1. The root of those inefficiencies is that the patch reuses the
cluster-wide running_xact WAL infrastructure to log db-specific
running transactions, and then tries to feed that into the existing
snapbuild machinery to reach a consistent state.

As another example of this mismatch that occurred to me today: in
SnapBuildCommitTxn, we are tracking the committed_xip array for all
cluster-wide XIDs, even when using a db-specific snapshot. A
db-specific snapshot shouldn't need to care about XIDs from other
databases. We only try to take care of it in one part of the system
where process running_xacts record. I admit that I don't know at this
stage what exactly we should do about it but all such things deserve a
discussion and careful thought.

The broader issue is that the entire logical decoding mechanism is
designed to process cluster-wide transactions. This patch tries to
bypass that foundational assumption, but only during the initial
snapshot construction while processing running_xacts record.

To be clear, I am not against the idea of db-specific snapshots to
enable concurrent repacks. My concern is simply the time required to
get the architecture right. In its current state, we need more time to
carefully consider how this db-specific concept interacts with the
rest of the logical decoding machinery, which is built for
cluster-wide records.

> or we
> revert db-specific snapshots and go back to the standard snapshot-taking
> technique for REPACK in 19 and see what we can improve for 20.
>
> Now, the worst consequence of reverting db-specific snapshots is that
> you will only be able to run REPACK in a single database at a time
> (because any subsequent REPACK will have to wait until the first one
> finishes before being able to get its snapshot).  In most normal cases
> this is probably not a big deal.  But if you have a multitenant system,
> and you want your users to be able to run REPACK on their tables, you
> may be a bit screwed.  So I hesitate to just go and revert it without
> offering those people any alternative.
>

I understand your point but I feel we can extend the current feature
in future versions to address such cases (allow REPACK CONCURRENTLY on
tables in multiple-databases simultaneously). For now, they may need
to rely on REPACK without CONCURRENTLY option, if they want to use it
for multiple databases simultaneously.

> (It's also possible that being unable to run more than one REPACK at a
> time is not so big a deal.  After all, it's supposed to be an infrequent
> operation.  And users probably don't or shouldn't have multi-terabyte
> tables in multitenant databases anyway.)
>
> I'm not sure I understand the point of the standby.  I mean, you can't
> run REPACK on the standby anyway, so I don't see this as a very
> problematic restriction.  Do you have other reasons for wanting a
> db-specific snapshot in a standby?
>

We are exposing need_shared_catalogs as a generic plugin option,
defined as: 'it can be set to false if one is certain the plugin
functions do not access shared system catalogs.' This implies it can
be used for purposes other than REPACK.

For example, one can imagine a single-database audit plugin that only
cares about data modifications within a specific database. By setting
need_shared_catalogs = false on a standby, it could reach a CONSISTENT
state much faster, perfectly serving its needs.

While such a plugin might not exist right now, my broader point is
this: when we expose a generic facility, it can and will be used in
ways beyond our initial core use cases. We should try to ensure the
design doesn't permanently preclude such extensions. With the current
design choice, we are painting ourselves into a corner where this
feature cannot easily be extended to standbys even in the future.

-- 
With Regards,
Amit Kapila.

  19. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-19T18:52:13Z

    On 2026-May-14, Amit Kapila wrote:

> The broader issue is that the entire logical decoding mechanism is
> designed to process cluster-wide transactions. This patch tries to
> bypass that foundational assumption, but only during the initial
> snapshot construction while processing running_xacts record.
> 
> To be clear, I am not against the idea of db-specific snapshots to
> enable concurrent repacks. My concern is simply the time required to
> get the architecture right. In its current state, we need more time to
> carefully consider how this db-specific concept interacts with the
> rest of the logical decoding machinery, which is built for
> cluster-wide records.

Hmm.  So at this point I have to admit that the time I'll have before
beta 1 is going to be very scarce.  You're probably right that it's
better to revert db-specific snapshots in pg19, and try again for 20.
The revert should be a simple patch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  20. Re: Adding REPACK [concurrently]

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-23T15:29:35Z

    On Tue, May 19, 2026 at 11:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2026-May-14, Amit Kapila wrote:
>
> > The broader issue is that the entire logical decoding mechanism is
> > designed to process cluster-wide transactions. This patch tries to
> > bypass that foundational assumption, but only during the initial
> > snapshot construction while processing running_xacts record.
> >
> > To be clear, I am not against the idea of db-specific snapshots to
> > enable concurrent repacks. My concern is simply the time required to
> > get the architecture right. In its current state, we need more time to
> > carefully consider how this db-specific concept interacts with the
> > rest of the logical decoding machinery, which is built for
> > cluster-wide records.
>
> Hmm.  So at this point I have to admit that the time I'll have before
> beta 1 is going to be very scarce.  You're probably right that it's
> better to revert db-specific snapshots in pg19, and try again for 20.
>

Sounds reasonable.

> The revert should be a simple patch.
>

I also think so.

-- 
With Regards,
Amit Kapila.

  21. Re: Adding REPACK [concurrently]

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-05-24T11:29:28Z

    On 2026-May-23, Amit Kapila wrote:

> > The revert should be a simple patch.
> 
> I also think so.

Okay, pushed the revert after seeing it pass CI:
  https://cirrus-ci.com/build/5520722497372160

Thanks!

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/