Subject: Re: Adding REPACK [concurrently]

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Wed, 1 Apr 2026 20:25:54 +0530

Hello,

i was fuzz testing v48 , and found a crash when REPACK (concurrently)
itself errors out,
1) while running

create table test(id int);
REPACK (concurrently) test;

TBH i didn't knew this, sometimes it's better to not know "rules" ;)
[NOTE: maybe we should add that we can't run
REPACK (concurrently) on table without identity index or primary key into
repack.sgml]

ERROR:  cannot process relation "test"
2026-04-01 19:06:31.211 IST [2495575] HINT:  Relation "test" has no
identity index.
2026-04-01 19:06:31.211 IST [2495575] STATEMENT:  repack (concurrently)
test;
TRAP: failed Assert("proc->statusFlags ==
ProcGlobal->statusFlags[proc->pgxactoff]"), File: "procarray.c", Line: 719,
PID: 2495575
postgres: srinath postgres [local]
REPACK(ExceptionalCondition+0x98)[0xaaaaad938d84]
postgres: srinath postgres [local]
REPACK(ProcArrayEndTransaction+0x1f0)[0xaaaaad6c15fc]
postgres: srinath postgres [local] REPACK(+0x210cf0)[0xaaaaad190cf0]
postgres: srinath postgres [local] REPACK(+0x2117e4)[0xaaaaad1917e4]
postgres: srinath postgres [local]
REPACK(AbortCurrentTransaction+0x10)[0xaaaaad191740]
postgres: srinath postgres [local]
REPACK(PostgresMain+0x568)[0xaaaaad7116e4]
postgres: srinath postgres [local] REPACK(+0x786ae0)[0xaaaaad706ae0]
postgres: srinath postgres [local]
REPACK(postmaster_child_launch+0x1f0)[0xaaaaad5d719c]
postgres: srinath postgres [local] REPACK(+0x65ea98)[0xaaaaad5dea98]
postgres: srinath postgres [local] REPACK(+0x65b650)[0xaaaaad5db650]
postgres: srinath postgres [local]
REPACK(PostmasterMain+0x1564)[0xaaaaad5dae1c]
postgres: srinath postgres [local] REPACK(main+0x3dc)[0xaaaaad466348]
/lib/aarch64-linux-gnu/libc.so.6(+0x284c4)[0xffffb40d84c4]
/lib/aarch64-linux-gnu/libc.so.6(__libc_start_main+0x98)[0xffffb40d8598]
postgres: srinath postgres [local] REPACK(_start+0x30)[0xaaaaad06ddf0]
2026-04-01 19:06:31.800 IST [2494560] LOG:  client backend (PID 2495575)
was terminated by signal 6: Aborted

2) And when running REPACK (concurrently) on the same table while already a
repack was running
on the same table ,just to verify the deadlock occurs and gets errored out
that
"could not wait for concurrent REPACK" but instead got the same crash.

ERROR:  could not wait for concurrent REPACK
2026-04-01 12:55:39.481 IST [2397660] DETAIL:  Process 2397660 waits for
REPACK running on process 2397307
2026-04-01 12:55:39.481 IST [2397660] CONTEXT:  waiting for
ShareUpdateExclusiveLock on relation 16385 of database 5
2026-04-01 12:55:39.481 IST [2397660] STATEMENT:  repack (concurrently)
stress_victim ;
2026-04-01 12:55:39.497 IST [2397151] LOG:  checkpoint complete: time:
wrote 2056 buffers (12.5%), wrote 0 SLRU buffers; 0 WAL file(s) added, 0
removed, 0 recycled; write=206.804 s, sync=0.003 s, total=861.616 s; sync
files=17, longest=0.002 s, average=0.001 s; distance=318978 kB,
estimate=515341 kB; lsn=2/02810A60, redo lsn=2/02810910
TRAP: failed Assert("proc->statusFlags ==
ProcGlobal->statusFlags[proc->pgxactoff]"), File: "procarray.c", Line: 719,
PID: 2397660
postgres: srinath postgres [local]
REPACK(ExceptionalCondition+0x98)[0xaaaae7d58d84]
postgres: srinath postgres [local]
REPACK(ProcArrayEndTransaction+0x1f0)[0xaaaae7ae15fc]
postgres: srinath postgres [local] REPACK(+0x210cf0)[0xaaaae75b0cf0]
postgres: srinath postgres [local] REPACK(+0x2117e4)[0xaaaae75b17e4]
postgres: srinath postgres [local]
REPACK(AbortCurrentTransaction+0x10)[0xaaaae75b1740]
postgres: srinath postgres [local]
REPACK(PostgresMain+0x568)[0xaaaae7b316e4]
postgres: srinath postgres [local] REPACK(+0x786ae0)[0xaaaae7b26ae0]
postgres: srinath postgres [local]
REPACK(postmaster_child_launch+0x1f0)[0xaaaae79f719c]
postgres: srinath postgres [local] REPACK(+0x65ea98)[0xaaaae79fea98]
postgres: srinath postgres [local] REPACK(+0x65b650)[0xaaaae79fb650]
postgres: srinath postgres [local]
REPACK(PostmasterMain+0x1564)[0xaaaae79fae1c]
postgres: srinath postgres [local] REPACK(main+0x3dc)[0xaaaae7886348]
/lib/aarch64-linux-gnu/libc.so.6(+0x284c4)[0xffff9ec984c4]
/lib/aarch64-linux-gnu/libc.so.6(__libc_start_main+0x98)[0xffff9ec98598]
postgres: srinath postgres [local] REPACK(_start+0x30)[0xaaaae748ddf0]
2026-04-01 12:58:18.198 IST [2397147] LOG:  client backend (PID 2397660)
was terminated by signal 6: Aborted

the reason for this crash was ProcGlobal->statusFlags was not initialized
during the start of ExecRepack , earlier Abort before reaching the proper
initialization of ProcGlobal->statusFlags which was done in rebuild_relation
caused this assert failure in ProcArrayEndTransaction.

Here's the diff to solve this crash.

diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 29ba49744eb..d44092a407a 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -284,7 +284,23 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool
isTopLevel)
  * that others can conflict with.
  */
  if ((params.options & CLUOPT_CONCURRENT) != 0)
+ {
+ /*
+ * Do not let other backends wait for our completion during their
+ * setup of logical replication. Unlike logical replication publisher,
+ * we will have XID assigned, so the other backends - whether
+ * walsenders involved in logical replication or regular backends
+ * executing also REPACK (CONCURRENTLY) - would have to wait for our
+ * completion before they can build their initial snapshot. It is o.k.
+ * for any decoding backend to ignore us because we do not change
+ * tuple descriptor of any table, and the data changes we write should
+ * not be decoded by other backends.
+ */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
  MyProc->statusFlags |= PROC_IN_CONCURRENT_REPACK;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+ LWLockRelease(ProcArrayLock);
+ }

  /*
  * If a single relation is specified, process it and we're done ... unless
@@ -988,22 +1004,6 @@ rebuild_relation(Relation OldHeap, Relation index,
bool verbose,

  if (concurrent)
  {
- /*
- * Do not let other backends wait for our completion during their
- * setup of logical replication. Unlike logical replication publisher,
- * we will have XID assigned, so the other backends - whether
- * walsenders involved in logical replication or regular backends
- * executing also REPACK (CONCURRENTLY) - would have to wait for our
- * completion before they can build their initial snapshot. It is o.k.
- * for any decoding backend to ignore us because we do not change
- * tuple descriptor of any table, and the data changes we write should
- * not be decoded by other backends.
- */
- LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
- MyProc->statusFlags |= PROC_IN_CONCURRENT_REPACK;
- ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
- LWLockRelease(ProcArrayLock);
-
  /*
  * The worker needs to be member of the locking group we're the leader
  * of. We ought to become the leader before the worker starts. The

Thoughts?

-- 
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

---

From: Antonin Houska <[email-masked]>
Date: Wed, 01 Apr 2026 19:06:53 +0200

Srinath Reddy Sadipiralla <[email-masked]> wrote:

> i was fuzz testing v48 , and found a crash when REPACK (concurrently) itself errors out,
> 1) while running
> 
> REPACK (concurrently) on table without identity index or primary key into repack.sgml]
> 
> 2026-04-01 19:06:31.211 IST [2495575] HINT:  Relation "test" has no identity index.
> 2026-04-01 19:06:31.211 IST [2495575] STATEMENT:  repack (concurrently) test;
> TRAP: failed Assert("proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]"), File: "procarray.c", Line: 719, PID: 2495575

Thanks. Attached here is v48-0006 fixed.

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

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 1 Apr 2026 23:52:12 +0200

> Thanks. Attached here is v48-0006 fixed.

Ah thanks, I incorporated this in the series and rebased, and here's
also a quick and simple additional patch that adds a GUC
max_repack_replication_slots which are especially there to support
REPACK.

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

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Thu, 2 Apr 2026 05:49:42 +0530

Hi Antonin,

>   * of. We ought to become the leader before the worker starts. The

i think as i did earlier in the diff, shouldn't we remove the duplicate code
from rebuild_relation, am i missing something?

---

From: Antonin Houska <[email-masked]>
Date: Fri, 03 Apr 2026 14:08:38 +0200

> max_repack_replication_slots which are especially there to support

This is an alternative implementation of 0006, allowing one backend running
REPACK (CONCURRENTLY) in a database, instead of one backend in a cluster.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 3 Apr 2026 16:59:09 +0200

> REPACK (CONCURRENTLY) in a database, instead of one backend in a cluster.

Thanks!  so I'm removing the previous one and taking this one.  Here's a
v50:

- In testing, I noticed that we could sometimes request a Flush for a
  WAL position that hasn't been written yet.  This is due to my
  replacing the original code that wrote a dummy xlog record that we
  could flush, with a call to XLogGetInsertRecPtr().  So we'd get an
  error like

LOG:  request to flush past end of generated WAL; request 0/15CF0018, current position 0/15CF000
  Antonin promptly noticed that this is because XLogGetInsertRecPtr()
  gets the LSN past the segment header, which is 18 bytes wrong.  So the
  fix here is to use XLogGetInsertEndRecPtr() instead.
  
- My testing also uncovered a problem with exclusion constraints; tables
  with them would fail to repack like
  ERROR:  exclusion constraint record missing for rel temporal_fk_mltrng2mltrng_pk_repacknew
  Antonin sent a patch to create copies of the constraints on the
  transient index, which seems like it fixes the problem.  Those
  constraints are obviously discarded together with the transient index.

- I polished the patch to reserve replication slots for REPACK.  Given
  the new implementation of 0006 that was submitted implies that we can
  now run multiple repacks concurrently, I changed the default of 1 to 5.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Here's a general engineering tip: if the non-fun part is too complex for you
to figure out, that might indicate the fun part is too ambitious." (John Naylor)
https://postgr.es/m/CAFBsxsG4OWHBbSDM%3DsSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g%40mail.gmail.com

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 3 Apr 2026 19:24:30 +0200

On 2026-Apr-03, Antonin Houska wrote:

> diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
> index 00b21ede481..c25dbeadff3 100644
> --- a/src/backend/commands/repack_worker.c
> +++ b/src/backend/commands/repack_worker.c

> @@ -233,6 +234,13 @@ repack_setup_logical_decoding(Oid relid)
>  
>  	EnsureLogicalDecodingEnabled();
>  
> +	/*
> +	 * By declaring that our output plugin does not need shared catalogs, we
> +	 * avoid waiting for completion of transactions running in other databases
> +	 * than the one we're connected to.
> +	 */
> +	accessSharedCatalogsInDecoding = false;
> +
>  	/*
>  	 * Neither prepare_write nor do_write callback nor update_progress is
>  	 * useful for us.

I find this reliance on a global variable for this a bit icky.  Would it
work to instead change the CreateInitDecodingContext() signature, so
that instead of "bool need_full_snapshot" it has a three-valued boolean
to distinguish the two cases from the original plus this new one?  I
think the value could be stored in LogicalDecodingContext, from where
standby_decode() could obtain it.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 3 Apr 2026 21:31:48 +0200

>   now run multiple repacks concurrently, I changed the default of 1 to 5.

Srinath let me know that this new part was causing CI failures on
Windows.  This version v51 should be okay (or, at least, it passes for
me on CI).  Additional changes worth mentioning:

- I think it's nicer for the index_create() API to get a bit to indicate
  suppression of progress reporting; so existing callers don't need to
  do anything.  I guess this is mostly a matter of taste.

- I incorporated Srinath's fix for the PreventInTransactionBlock block.

- When CheckSlotRequirements() is to complain about
  "max_replication_slots or max_repack_replication_slots", it seems
  actually nicer to say exactly which one of these is the cause of the
  problem.  This is easy to change; patch 0010 does it; it requires
  passing down a "repack" flag all the way from
  CheckLogicalDecodingRequirements() and it needs to add an argument to
  CreateInitDecodingContext(), which is perhaps not so great.  On the
  whole I'm inclined to do it anyway, but I'm about equally happy to
  leave it alone.  This is what would change:

original:
        ereport(ERROR,
-               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                errmsg("replication slots can only be used if \"%s\" > 0 or \"%s\" > 0",
-                       "max_replication_slots", "max_repack_replication_slots")));

patched:
        ereport(ERROR,
+               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+               errmsg("replication slots can only be used if \"%s\" > 0",
+                      repack ? "max_repack_replication_slots" : "max_replication_slots"));

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"This is a foot just waiting to be shot"                (Andrew Dunstan)

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 3 Apr 2026 21:37:57 +0200

Hi,

Sorry -- there's a doc build failure also in CI.  Fixed here.

Regards

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Sat, 4 Apr 2026 06:20:35 +0000

Dear Álvaro,

While testing REPACK CONCURRENTLY command with xid_wraparound module, noticed that
wraparound-autovac worker was terminated with an error like below.

``
ERROR:  could not wait for concurrent REPACK
DETAIL:  Process 41512 waits for REPACK running on process 41027
CONTEXT:  waiting for ShareUpdateExclusiveLock on relation 16384 of database 5
        automatic vacuum of table "postgres.public.test"
```

The behavior is different from other commands, like LOCK and maybe normal REPACK.
In these cases the autovac worker would wait till the command fails.

Is it an intentional behavior? If so, what is the advantage that we terminate the
failsafe vacuum?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Antonin Houska <[email-masked]>
Date: Sat, 04 Apr 2026 10:50:14 +0200

> standby_decode() could obtain it.

I agree that the global variable is not handy, but instead of modifying
CreateInitDecodingContext(), how about adding a boolean returning callback to
OutputPluginCallbacks? The point is that whether shared catalogs are needed
during the decoding or not is actually property of the plugin.

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 4 Apr 2026 11:48:58 +0200

> during the decoding or not is actually property of the plugin.

Oh, yeah, that sounds good to me.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

---

From: 'Alvaro Herrera' <[email-masked]>
Date: Sat, 4 Apr 2026 12:19:10 +0200

Hello,

> Is it an intentional behavior? If so, what is the advantage that we terminate the

Hmm, this is intentional; see here:
https://postgr.es/m/[email-masked]
Note that in order for this to happen, the autovacuum must be starting
when the repack is already underway.  The theory behind this behavior is
that the autovacuum run is not useful anyway: its purpose is to advance
the freeze xmin/multixact, but the repack is also going to do that.
Once repack is done, autovacuum can assess again whether an emergency
vacuum is needed, and launch a worker in that case.

Do you think it would be better if we allowed autovacuum to continue?
I'm not 100% myself of this new behavior, and it would be very good to 
give it some more thought.

I suppose it's unfortunate that autovacuum launcher is going to try
again and again to get workers to process that table, and they are going
to be killed over and over.  Maybe it would be better to have autovac
ignore those tables.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Para tener más hay que desear menos"

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Sat, 4 Apr 2026 18:46:58 +0530

Hi Alvaro,

On Sat, Apr 4, 2026 at 1:01 AM Alvaro Herrera <[email-masked]>
wrote:

> me on CI).

yes indeed , TotalMaxReplicationSlots was the culprit , as the window
after getting repack worked was forked , i think this was set back to 0.
Thanks for fixing this with "repack" flag , initially i thought why can't we
go with a macro
#define TotalMaxReplicationSlots (max_replication_slots +
max_repack_replication_slots)
but i thought maybe in future we might need this "repack" flag to add
more slot requirements.

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 4 Apr 2026 15:55:48 +0200

> but i thought maybe in future we might need this "repack" flag to add

Yeah, the other option would have been to add the variable to
restore_backend_variables() so that it's restored after the fork+exec,
but that didn't seem better.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Niemand ist mehr Sklave, als der sich für frei hält, ohne es zu sein."
       Nadie está tan esclavizado como el que se cree libre no siéndolo
                                           (Johann Wolfgang von Goethe)

---

From: Antonin Houska <[email-masked]>
Date: Sat, 04 Apr 2026 17:29:19 +0200

> Oh, yeah, that sounds good to me.

This is it. New callback was actually not needed, I just added a new flag to
the OutputPluginOptions structure.

---

From: Alvaro Herrera <[email-masked]>
Date: Sun, 5 Apr 2026 01:53:09 +0200

> the OutputPluginOptions structure.

Thank you, I removed the previous one and picked up this one (it's 0001
here.)  The only potentially troublesome thing I see with it is this change:

    /*
     * Update range of interesting xids based on the running xacts
     * information. We don't increase ->xmax using it, because once we are in
     * a consistent state we can do that ourselves and much more efficiently
     * so, because we only need to do it for catalog transactions since we
     * only ever look at those.
     *
     * NB: We only increase xmax when a catalog modifying transaction commits
     * (see SnapBuildCommitTxn).  Because of this, xmax can be lower than
     * xmin, which looks odd but is correct and actually more efficient, since
     * we hit fast paths in heapam_visibility.c.
+    *
+    * If database specific transaction info was used during startup, the info
+    * for the whole cluster can make xmin go backwards. That would be bad
+    * because we might no longer have older XIDs in ->committed.
     */
-   builder->xmin = running->oldestRunningXid;
+   if (NormalTransactionIdFollows(running->oldestRunningXid, builder->xmin))
+       builder->xmin = running->oldestRunningXid;

I can't see any problem with advancing the ->xmin only when it goes
forward, but I wonder if it's possible to introduce any bugs this way.

This bit looks funny though:

    /*
     * Advance the xmin limit for the current replication slot, to allow
     * vacuum to clean up the tuples this slot has been protecting.
     *
     * The reorderbuffer might have an xmin among the currently running
     * snapshots; use it if so.  If not, we need only consider the snapshots
     * we'll produce later, which can't be less than the oldest running xid in
     * the record we're reading now.
     */
    xmin = ReorderBufferGetOldestXmin(builder->reorder);
-   if (xmin == InvalidTransactionId)
+   /*
+    * Like above, do not let slot xmin go backwards.
+    */
+   if (xmin == InvalidTransactionId && !db_specific)
        xmin = running->oldestRunningXid;

I probably need some sleep, but this doesn't make sense to me.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

---

From: Antonin Houska <[email-masked]>
Date: Sun, 05 Apr 2026 09:54:49 +0200

> I probably need some sleep, but this doesn't make sense to me.

ok, maybe just skip the whole cleanup in that special case.

---

From: Alvaro Herrera <[email-masked]>
Date: Sun, 5 Apr 2026 13:50:12 +0200

> ok, maybe just skip the whole cleanup in that special case.

Hmm, should we make this test only in the db_specific case?  Doing it
unconditionally makes me a bit nervous (maybe because I don't fully
understand historic snapshot building).

Anyway I just pushed the addition of a progress-suppression bit to
index_create() interface.  Here's the rest of the series.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

---

From: Antonin Houska <[email-masked]>
Date: Sun, 05 Apr 2026 18:30:41 +0200

> understand historic snapshot building).

I thought about adding Assert(db_specific) in front of the new return
statement. So what you suggest makes sense to me.

As far as I understand, the xl_running_xacts record is not directly involved
in the snapshot build. Rather, the list of XIDs for snapshots is created and
updated by processing COMMIT and ABORT records.

---

From: Alvaro Herrera <[email-masked]>
Date: Sun, 5 Apr 2026 20:56:16 +0200

Hi,

So I've been trying to understand the "Introduce an option to make
logical replication database specific." patch and I have to confess I
just cannot.

As far as I can read, the point is that if we reach
SnapBuildProcessRunningXacts() when db_specific is true (which means
standby_decode is called in an output plugin that has set
need_shared_catalogs to false), _and_ we've not reached consistent state
yet, then we'll call LogStandbySnapshot with our DB oid to emit a new
xl_running_xacts message.

So the WAL-decoding process emits WAL.  I don't know if in normal
conditions logical decoding processes emit WAL.  If this is exceptional,
I think we should add a comment.

Now, this additional WAL message will be processed by all other
processes decoding WAL.  Perhaps it will ignored by most of them.  But
most importantly, it will also reach back to ourselves, at which point
we can hopefully use it to see that we might have reached consistent
state within our database.  Then we know our snapshot is ready to be
used.

Is this correct?

I think the reason it's safe to skip a lot of the processing caused by
this additional process, is that xl_running_xacts messages are also
emitted in other places in a non-database specific manner.  So all the
other placecs that are emitting that message continue to exist and
cause logical-decoders operate in the same way as before.

I think we should sprinkle lots of comments in several places about
this.  For example, I propose that standby_redo() should have something
like

  * If 'dbid' is valid, only gather transactions running in that database.
+ * Such records should not be the only ones emitted, because this has
+ * potentially dangerous side-effects which makes some places ignore them:
+ *
+ * 1. SnapBuildProcessRunningXacts will skip computing the xmin and restart
+ * point from its input record if the record's xmin is older that the
+ * snapbuilder's current xmin; this should normally be fine because that
+ * information will be updated from other xl_running_xacts records.
+ * 2. standby_redo will likewise skip processing such a record
  *
(are there other things that should be mentioned?)

Also, LogStandbySnapshot() should have a comment explaining that passing
a valid dboid is a weird corner case which is to be used with care, and
that functions X Y and Z are going to ignore snapshots carrying a valid
dbid.

Why do we call SnapBuildFindSnapshot() to do this, instead of doing it
directly in SnapBuildProcessRunningXacts?  Seems like it would be more
straightforward.

---

From: Alvaro Herrera <[email-masked]>
Date: Sun, 5 Apr 2026 22:41:50 +0200

Hi,

So here's a v55 version of the base REPACK patches that I'm feeling
comfortable calling very close to committable.  I'm going to give an
additional read tomorrow and maybe make cosmetic adjustments, but there
should be nothing substantial.  Of course, the subsequent additions in
the other patches of v54 are still in the cards, and they are most
likely essential.

Changes compared to v54:
- changed reform_tuple() to not deform the tuple if no attributes are
  going to be touched.  We can simply make a copy instead, which I
  suspect is considerably cheaper (but I didn't measure).

- cleaned up worker shmem shutdown callback.  I think it's how it is
  because it copied parallel worker code, but that has a weird structure
  for --as far as I can see-- no good reason (we oughta change it too)

- renamed the worker from pgoutput_repack to pgrepack.
  (Note that this is an internal name that users don't face.)

- reverted some unnecessary changes to master

Thanks

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Apr 2026 10:54:23 +0530

> vacuum is needed, and launch a worker in that case.

But won't it delay in update of datfrozenxid/datminmxid? For example,
say repack errored out due to some reason, won't it further delay the
update to datfrozenxid/datminmxid which in turn can delay truncate of
clog. Is it possible that after repack errored out the launcher delays
in picking the same table again which further add to such a delay?

> Do you think it would be better if we allowed autovacuum to continue?

IIUC, the disadvantage of letting it continue is that if repack is
successful then we have unnecessarily occupied the worker which won't
do anything useful. If we want to not let autovacuum continue then we
should somehow deal with boundary cases which shouldn't lead to delay
in making progress to update frozen xids.

> to be killed over and over.  Maybe it would be better to have autovac

I feel that would be better at least when we know that the repack
concurrently command is already in progress. It can help avoid
launching workers again and again, especially when repack concurrently
command is going to take a long time.

-- 
With Regards,
Amit Kapila.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 06 Apr 2026 11:03:44 +0200

> xl_running_xacts message.

Right.

> I think we should add a comment.

Emitting WAL in logical decoding is not exceptional: SnapBuildWaitSnapshot()
already calls LogStandbySnapshot(), in order to get to the next phase.

> processes decoding WAL.  Perhaps it will ignored by most of them.

Right. That's one thing that I realized late yesterday, after having posted
the latest version of the patch. In SnapBuildProcessRunningXacts(), we need

	if (OidIsValid(running->dbid))
		return;

rather than

	if (db_specific)
		return;

because other backends can also generate their database-specific records.

> But

Yes.

> cause logical-decoders operate in the same way as before.

Yes. I'm still thinking though if, after having used the database-specific
record to reach CONSITENT, we sould enforce one cluster-wide record, so that
the cleanup (in "our backend") takes place sooner rather than later. Not sure
about that.

> (are there other things that should be mentioned?)

I added something like that, but - due to the reference to
SnapBuildProcessRunningXacts() - less verbose about snapbuild.c.

> that functions X Y and Z are going to ignore snapshots carrying a valid

One more check added in standby_decode() (and mentioned in that function in
the comment).

> directly in SnapBuildProcessRunningXacts?  Seems like it would be more

Yes, fixed.

One more problem I found when testing w/o background worker
(contrib/test_decoding) was that accessSharedCatalogsInDecoding was not set
back to true. Both AllocateSnapshotBuilder() FreeSnapshotBuilder() do that
now.

---

From: vignesh C <[email-masked]>
Date: Mon, 6 Apr 2026 14:45:31 +0530

> the other patches of v54 are still in the cards, and they are most

Few comments:
1) Can we add a comment why we should error out here, as repack
concurrently requires this whereas repack does not require this check.
Even if it is required for decoding can't it be handled by replica
identity full:
+       /*
+        * If the identity index is not set due to replica identity being, PK
+        * might exist.
+        */
+       ident_idx = RelationGetReplicaIndex(rel);
+       if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))
+               ident_idx = rel->rd_pkindex;
+       if (!OidIsValid(ident_idx))
+               ereport(ERROR,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                               errmsg("cannot process relation \"%s\"",
+                                          RelationGetRelationName(rel)),
+                               errhint("Relation \"%s\" has no
identity index.",
+                                               RelationGetRelationName(rel)));

2) Do you think it will be good to add a test to simulate a case where
one of the swap_replation_files is successful and a failure after
that. We can verify that the oid should still point to old oids:
+       /*
+        * Even ShareUpdateExclusiveLock should have prevented others from
+        * creating / dropping indexes (even using the CONCURRENTLY
option), so we
+        * do not need to check whether the lists match.
+        */
+       forboth(lc, ind_oids_old, lc2, ind_oids_new)
+       {
+               Oid                     ind_old = lfirst_oid(lc);
+               Oid                     ind_new = lfirst_oid(lc2);
+               Oid                     mapped_tables[4] = {0};
+
+               swap_relation_files(ind_old, ind_new,
+                                                       (old_table_oid
== RelationRelationId),
+                                                       false,  /*
swap_toast_by_content */
+                                                       true,
+                                                       InvalidTransactionId,
+                                                       InvalidMultiXactId,
+                                                       mapped_tables);

3) I'm not sure if this change should be part of this patch:
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl
b/src/backend/storage/lmgr/generate-lwlocknames.pl
index b49007167b0..2e7f1054e62 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -162,7 +162,7 @@ while (<$lwlocklist>)

 die
   "$wait_event_lwlocks[$lwlock_count] defined in wait_event_names.txt but "
-  . " missing from lwlocklist.h"
+  . "missing from lwlocklist.h"
   if $lwlock_count < scalar @wait_event_lwlocks;

4) Can we add an example for concurrently in documentation

5) Typos
5.a) "jsut" should be "just":
+ * operation to avoid any lock-upgrade hazards.  In the concurrent case, we
+ * grab ShareUpdateExclusiveLock (jsut like VACUUM) for most of the

5.b In commit message "intial" should be "initial"
While the "concurrent data" changes are applied at specific stages (we cannot
do that until the intial copy is finished and indexes are built), a background

6) This includes are not required in repack.c, for me it could compile
without it:
+#include "access/detoast.h"
+#include "access/xloginsert.h"
+#include "catalog/pg_control.h"

7) Can you check if the copyright year mentioned for the new files are
correct, as different files mention different years like:
/*-------------------------------------------------------------------------
 *
 * pgrepack.c
 * Logical Replication output plugin for REPACK command
 *
 * Copyright (c) 2012-2026, PostgreSQL Global Development Group
 *
 * IDENTIFICATION
 *   src/backend/replication/pgrepack/pgrepack.c
 *
 *-------------------------------------------------------------------------
 */

 /*-------------------------------------------------------------------------
 *
 * repack_worker.c
 *    Implementation of the background worker for ad-hoc logical decoding
 *    during REPACK (CONCURRENTLY).
 *
 *
 * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994-5, Regents of the University of California
 *
 *
 * IDENTIFICATION
 *   src/backend/commands/repack_worker.c

 Meson.build file:
 # Copyright (c) 2022-2026, PostgreSQL Global Development Group

Regards,
Vignesh

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 6 Apr 2026 11:38:20 +0200

> Few comments:

Thanks for reviewing this patch!

> +                                               RelationGetRelationName(rel)));

Ah, I was just rewriting that comment moments ago.  I think we could
make it work with replica identity full in theory, but I'm guessing it
would be useless.  You really need an index that lets you locate the
affected tuples; otherwise the replay would take forever.  If the table
is big, that would make the whole thing unworkable.  If the table isn't
big, then you can probably just use straight repack without too much
disruption.

	/*
	 * Obtain the replica identity index -- either one that has been set
	 * explicitly, or the primary key.  If none of these cases apply, the
	 * table cannot be repacked concurrently.  It might be possible to have
	 * repack work with a FULL replica identity; however that requires more
	 * work and is not implemented yet.
	 */
 
Now, it's possible to have a non-unique index on some columns (not good
enough to be replica identity) which gives you a list of candidate
tuples, and then the implementation chooses one based on the other
columns which are present in the FULL replica identity.  (This seems a
bit dangerous, because there might be multiple matching tuples; and how
do you choose?)  Now let's further suppose you can narrow down to a
single tuple; in that case, using FULL would work.  However, as I said,
this requires more implementation effort.  We could entertain a patch
for this during the pg20 cycle, though I'm doubtful that it would really
be worth your while.

> that. We can verify that the oid should still point to old oids:

Hmm, it's not clear to me in which cases this can happen.  Are you
thinking that the first swap_replation_files call dies because of
out-of-memory?

Note that the really weird cases, like pg_class or mapped relations, are
directly rejected.  So we don't get into the branch with
!RelFileNumberIsValid, and so on.

I mean -- I'm not opposed to adding a test case for it.  But I suspect
it's going to be somewhat annoying to write.

>    if $lwlock_count < scalar @wait_event_lwlocks;

Yeah, will remove.

> 4) Can we add an example for concurrently in documentation

Sure.

> correct, as different files mention different years like:

I'll look into these, thanks for pointing it out.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)

---

From: vignesh C <[email-masked]>
Date: Mon, 6 Apr 2026 15:31:27 +0530

>          * work and is not implemented yet.

Should this be mentioned in XXX comment:
It might be possible to have repack work with a FULL replica identity;
however that requires more work and is not implemented yet.

> thinking that the first swap_replation_files call dies because of

Yes, I was thinking of that.

> it's going to be somewhat annoying to write.

I will verify this scenario through debugger.

Regards,
Vignesh

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 6 Apr 2026 12:14:57 +0200

Hello!

On Sun, Apr 5, 2026 at 10:41 PM Alvaro Herrera <[email-masked]> wrote:
> comfortable calling very close to committable.

Some comments for v55.

repack.c:2725

    if (!VARATT_IS_EXTERNAL(varlen))
          continue;

I think it should be VARATT_IS_EXTERNAL_INDIRECT - the same as in pgrepack:244.

Also,  after

natt_ext--;

I think it worth to add

Assert(natt_ext >= 0);

or Assert(natt_ext == 0); but in another place.

Or exit early with
     for (int i = 0; i < desc->natts && natt_ext > 0; i++)

----------------------------
repack.c:2587

    table_tuple_insert(rel, slot, GetCurrentCommandId(true),
       HEAP_INSERT_NO_LOGICAL, NULL);

More idiomatic to use TABLE_INSERT_NO_LOGICAL instead.

----------------------------
repack.c:2696

     ExecForceStoreHeapTuple(tup, slot, false);

AFAIU there is a memory leak here. Memory allocated above (for tuple)
is not freed in any way, because shouldFree == false.
Also, ExecClearTuple (tts_virtual_clear for virtual tuples) requires
TTS_SHOULDFREE to be set to free anything.

-------------------------
grab ShareUpdateExclusiveLock (jsut like VACUUM

typo in "just"

--------------------------

"If the identity index is not set due to replica identity being, PK"

Missing "FULL" after "being"?

-------------------------

Commit message:

"intial copy" -> "initial copy"
"backed performing REPACK" -> "backend performing REPACK"

Best regards,
Mikhail.

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 6 Apr 2026 12:48:23 +0200

Right.

>      for (int i = 0; i < desc->natts && natt_ext > 0; i++)

Hmm, how about something like this?

            natt_ext--;
            if (natt_ext < 0)
                ereport(ERROR,
                        errcode(ERRCODE_DATA_CORRUPTED),
                        errmsg("insufficient number of attributes stored separately"));

I'd like to give more details, such as the tuple's identity, but that
seems hard ...

Ah right.

Yeah but I don't want the virtual tuple to be materialized (which would
happen in tts_virtual_materialize if I set shouldFree=true).  The memory
should be freed in 
		ResetPerTupleExprContext(chgcxt->cc_estate);
anyway, right?  Maybe deserves a comment.

Right.

Ah yeah, I rewrote this.

Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 6 Apr 2026 13:21:32 +0200

Hi!

>                         errmsg("insufficient number of attributes stored separately"));
I think it is ok.

> anyway, right?  Maybe deserves a comment.

Not sure, ResetPerTupleExprContext resets just "ExecutorState".
But slots are created in another memory context.

Also, we can't reset slot->tts_mcxt itself - it will free the slot also.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 06 Apr 2026 13:23:57 +0200

> it's going to be somewhat annoying to write.

After all, I think we'd end up testing whether transaction abort works
correctly.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Apr 2026 17:26:33 +0530

On Mon, Apr 6, 2026 at 3:08 PM Alvaro Herrera <[email-masked]> wrote:
> single tuple; in that case, using FULL would work.
>

I think this is already working for apply worker where we compare
tuples if the index is non_uniuqe, see FindReplTupleInLocalRel.

  However, as I said,
> for this during the pg20 cycle, though I'm doubtful that it would really

It is fine to support it later in pg20. In general, I wonder if we
have evaluated whether some of the apply-worker infrastructure could
have been reused here?

-- 
With Regards,
Amit Kapila.

---

From: Andres Freund <[email-masked]>
Date: Mon, 6 Apr 2026 17:11:30 -0400

Hi,

I just saw this got committed and wanted to briefly play with it.  It works
nicely!

Except that at first I tried this in a debugging build, and was briefly rather
dismayed by the performance.  It was really slow.  But it's not really related
to repack / the patches here.

Turns out that it is to a good part due to
  heap_insert()
  ->CacheInvalidateHeapTuple()
  ->CacheInvalidateHeapTupleCommon()
  ->AssertCouldGetRelation()
not being cheap and running a *lot*.

Admittedly it's way worse if you build with -O0, which I tend to do to make
debugging easier.

In that config, the assert single-handled increases the time for a repack by
35% or so.

Noah, is there any reason we need to do the AssertCouldGetRelation() before
the !IsCatalogRelation(relation)? Given that the goal is to make
RelationGetRelid() safe, it doesn't seem there is?

It's totally valid to not have done so initially, this is a quite complicated
feature:

I saw this is using individual heap_insert()s during the
heapam_relation_copy_for_cluster().  Doing individual WAL logged inserts isn't
exactly cheap or efficient from a WAL volume perspective...

Is there anything other than round tuits preventing us from using
multi_insert?

That actually would also reduce the cost in the REPACK decoding worker, due to
having to parse far fewer WAL records.

Greetings,

Andres Freund

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 6 Apr 2026 23:59:59 +0200

> I just saw this got committed and wanted to briefly play with it.  It works

Yeah, I have to say that Antonin did a great job here.

> In that config, the assert single-handled increases the time for a repack by

Yeah, I saw it was kinda sluggish, but wow, I didn't see *that* much
overhead.

> having to parse far fewer WAL records.

Nope, not really ... but I don't have any :-(

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 00:22:32 +0200

Hello,

> Also, we can't reset slot->tts_mcxt itself - it will free the slot also.

So what I ended up doing, is to just not change to the slot's context in
restore_tuple.  That was just dumb (mea culpa).  So all the memory we
allocate for the slot lives in the executor context, and goes away on
the ResetPerTupleExprContext call at the bottom of the loop.  BTW I
don't understand why you say that function only resets ExecutorState --
as far as I can tell, it does this

#define ResetPerTupleExprContext(estate) \
	do { \
		if ((estate)->es_per_tuple_exprcontext) \
			ResetExprContext((estate)->es_per_tuple_exprcontext); \
	} while (0)

which in turn does

#define ResetExprContext(econtext) \
	MemoryContextReset((econtext)->ecxt_per_tuple_memory)

which AFAICT is exactly what we want.

Anyway, here's the three missing parts.  I have not yet edited the
deadlock-checker one to protect autovacuum from processing tables under
repack.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Tue, 7 Apr 2026 01:53:02 +0200

Hi!

On Tue, Apr 7, 2026 at 12:22 AM Alvaro Herrera <[email-masked]> wrote:
> restore_tuple.

Yes, it should work.

> BTW I
> don't understand why you say that function only resets ExecutorState

My bad, I confused "ExprContext" with "ExecutorState" (names passed to
AllocSetContextCreate) while tracing.

Mikhail.

---

From: Noah Misch <[email-masked]>
Date: Mon, 6 Apr 2026 18:10:56 -0700

> RelationGetRelid() safe, it doesn't seem there is?

By running AssertCouldGetRelation() during every INSERT statement, this
detects cases that would be unsafe when the target of the INSERT happens to be
a system catalog.  Little of our INSERT/UPDATE coverage targets a system
catalog.  Hence, the current position is better for detection.

I wonder if this got slower in v19.  In v14-v18, the assert's cost is
proportional to the number of held lwlocks, often 0 or 1.  In v19, it's
proportional to PrivateRefCountHash cardinality.

---

From: Andres Freund <[email-masked]>
Date: Mon, 6 Apr 2026 21:58:19 -0400

Hi,

On 2026-04-06 18:10:56 -0700, Noah Misch wrote:
> a system catalog.

I see.

> Little of our INSERT/UPDATE coverage targets a system catalog.

Sure. We do have plenty DML doing heap_insert/update however.

> Hence, the current position is better for detection.

What if we returned early in AssertBufferLocksPermitCatalogRead() if
InterruptHoldoffCount == 0?  That'd only fail if some code manually did a
RESUME_INTERRUPTS() to balance the one acquired as part of the content lock?

> proportional to PrivateRefCountHash cardinality.

Yea, plausible.  It will only scan PrivateRefCountHash if
PrivateRefCountOverflowed overflowed, but it did overflow in the case I was
testing...

Greetings,

Andres Freund

---

From: Noah Misch <[email-masked]>
Date: Mon, 6 Apr 2026 19:42:33 -0700

> RESUME_INTERRUPTS() to balance the one acquired as part of the content lock?

Sounds good.

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Apr 2026 00:44:47 -0400

Maybe you saw this already, but BF member skink is failing on
src/test/modules/injection_points/specs/repack.spec:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2026-04-06%2022%3A50%3A41

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 10:40:25 +0200

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2026-04-06%2022%3A50%3A41

Thanks, I hadn't, and I cannot reproduce it locally.  Anyway, the
valgrind report is

==1617044== VALGRINDERROR-BEGIN
==1617044== Syscall param pwrite64(buf) points to uninitialised byte(s)
==1617044==    at 0x6704003: pwrite (pwrite64.c:25)
==1617044==    by 0x44AAC72: pg_pwritev (pg_iovec.h:101)
==1617044==    by 0x44AC6B5: FileWriteV (fd.c:2280)
==1617044==    by 0x44A8EC4: FileWrite (fd.h:245)
==1617044==    by 0x44A8EC4: BufFileDumpBuffer (buffile.c:538)
==1617044==    by 0x44A9034: BufFileFlush (buffile.c:724)
==1617044==    by 0x44A9661: BufFileClose (buffile.c:418)
==1617044==    by 0x426C4DE: export_initial_snapshot (repack_worker.c:346)
==1617044==    by 0x426CA17: RepackWorkerMain (repack_worker.c:145)
==1617044==    by 0x441D3AD: BackgroundWorkerMain (bgworker.c:865)
==1617044==    by 0x44219D9: postmaster_child_launch (launch_backend.c:265)
==1617044==  Address 0x12d745e2 is 106 bytes inside a block of size 8,272 client-defined
==1617044==    at 0x4661CFE: palloc (mcxt.c:1411)
==1617044==    by 0x44A8C54: makeBufFileCommon (buffile.c:121)
==1617044==    by 0x44A933F: BufFileCreateFileSet (buffile.c:272)
==1617044==    by 0x426C4A5: export_initial_snapshot (repack_worker.c:341)
==1617044==    by 0x426CA17: RepackWorkerMain (repack_worker.c:145)
==1617044==    by 0x441D3AD: BackgroundWorkerMain (bgworker.c:865)
==1617044==    by 0x44219D9: postmaster_child_launch (launch_backend.c:265)
==1617044==    by 0x4423B6D: StartBackgroundWorker (postmaster.c:4197)
==1617044==    by 0x4423DB8: maybe_start_bgworkers (postmaster.c:4362)
==1617044==    by 0x4425119: LaunchMissingBackgroundProcesses (postmaster.c:3437)
==1617044==    by 0x4426A75: ServerLoop (postmaster.c:1737)
==1617044==    by 0x44280DC: PostmasterMain (postmaster.c:1412)
==1617044==  Uninitialised value was created by a stack allocation
==1617044==    at 0x4674D39: SerializeSnapshot (snapmgr.c:1737)
==1617044== 
==1617044== VALGRINDERROR-END

and obviously BufFileCreateFileSet() is being called by existing code
already and it doesn't fail, so I *think* the problem might be that
SerializedSnapshotData has some padding bytes that are being written.
Maybe using palloc0() is enough?  I'll try that.

If that doesn't silence skink, I guess my next step is to reproduce
skink's environment more precisely ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Tue, 7 Apr 2026 14:12:22 +0530

Hi Tom,

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2026-04-06%2022%3A50%3A41

i looked into this , it seems like valgrind catches the uninitialised
padding bytes, which
repack worker is writing using BufFileWrite, it seems this fix solved the
problem.

diff --git a/src/backend/utils/time/snapmgr.c
b/src/backend/utils/time/snapmgr.c
index 2e6197f5f35..f5682b87626 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1739,6 +1739,8 @@ SerializeSnapshot(Snapshot snapshot, char
*start_address)

  Assert(snapshot->subxcnt >= 0);

+ MemSet(&serialized_snapshot, 0, sizeof(SerializedSnapshotData));
+
  /* Copy all required fields */
  serialized_snapshot.xmin = snapshot->xmin;
  serialized_snapshot.xmax = snapshot->xmax;

thoughts?

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 10:57:46 +0200

Srinath Reddy Sadipiralla <[email-masked]> wrote:

> i looked into this , it seems like valgrind catches the uninitialised padding bytes, which
> repack worker is writing using BufFileWrite, it seems this fix solved the problem.
> 
> diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> @@ -1739,6 +1739,8 @@ SerializeSnapshot(Snapshot snapshot, char *start_address)
>  

Could you reproduce the failure in your environment?

I haven't thought of this explanation because BufFileWrite() only copies the
data to a buffer in the BufFile structure and BufFileDumpBuffer() writes the
buffer. Maybe valgrind is able to track the copying?

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 11:22:18 +0200

> Could you reproduce the failure in your environment?

Yeah, he did.

> buffer. Maybe valgrind is able to track the copying?

Yeah, apparently it keeps track of tainted bytes somehow.  Clever.

The change to palloc0() that I was proposing did not fix the problem,
because the stack allocated struct overwrote those zeroes with the
uninitialized padding bytes.

I ended up with an equivalent fix to Srinath's -- zero-initializing
the stack-allocated struct, so that the bytes that end up copied by
memcpy() are all defined.  Srinath confirmed that in his environment the
valgrind failure goes away, so I think we're good.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No tengo por qué estar de acuerdo con lo que pienso"
                             (Carlos Caszeli)

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 11:29:17 +0200

> buffer. Maybe valgrind is able to track the copying?

Given this message, you may be right:

In my environment, the 'buffer' field starts at offset 80 into the BufFile
structure. We first write 8 bytes into it

	BufFileWrite(file, &snap_size, sizeof(snap_size));

followed by the snapshot. Since sizeof(SerializedSnapshotData) is 24, the
offset 106 should be the padding following the 'takenDuringRecovery' field.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 11:35:50 +0200

Thanks!

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 11:39:12 +0200

> Yeah, I have to say that Antonin did a great job here.

Likewise, I appreciate all your improvements! And of course, all the reviews
and testing done by other developers.

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Tue, 7 Apr 2026 15:11:17 +0530

> offset 106 should be the padding following the 'takenDuringRecovery' field.

yeah , same in my environment aslo

==00:00:00:06.569 3479320== Syscall param pwrite64(buf) points to
uninitialised byte(s)
==00:00:00:06.569 3479320==    at 0x55D6D38: pwrite (pwrite64.c:25)
==00:00:00:06.569 3479320==    by 0x842EE7: pg_pwritev (pg_iovec.h:101)
==00:00:00:06.569 3479320==    by 0x845F67: FileWriteV (fd.c:2280)
==00:00:00:06.569 3479320==    by 0x840877: FileWrite (fd.h:245)
==00:00:00:06.569 3479320==    by 0x841407: BufFileDumpBuffer
(buffile.c:538)
==00:00:00:06.569 3479320==    by 0x841A67: BufFileFlush (buffile.c:724)
==00:00:00:06.569 3479320==    by 0x8410B7: BufFileClose (buffile.c:418)
==00:00:00:06.569 3479320==    by 0x4BFBBB: export_initial_snapshot
(repack_worker.c:346)
==00:00:00:06.569 3479320==    by 0x4BF703: RepackWorkerMain
(repack_worker.c:145)
==00:00:00:06.569 3479320==    by 0x765D3F: BackgroundWorkerMain
(bgworker.c:865)
==00:00:00:06.569 3479320==    by 0x76C703: postmaster_child_launch
(launch_backend.c:265)
==00:00:00:06.569 3479320==    by 0x774F87: StartBackgroundWorker
(postmaster.c:4197)
==00:00:00:06.569 3479320==  Address 0x7b055f2 is 106 bytes inside a block
of size 8,272 client-defined
==00:00:00:06.569 3479320==    at 0xB234F4: palloc (mcxt.c:1411)
==00:00:00:06.569 3479320==    by 0x8408BF: makeBufFileCommon
(buffile.c:121)
==00:00:00:06.569 3479320==    by 0x840C2F: BufFileCreateFileSet
(buffile.c:272)
==00:00:00:06.569 3479320==    by 0x4BFB7F: export_initial_snapshot
(repack_worker.c:341)
==00:00:00:06.569 3479320==    by 0x4BF703: RepackWorkerMain
(repack_worker.c:145)
==00:00:00:06.569 3479320==    by 0x765D3F: BackgroundWorkerMain
(bgworker.c:865)
==00:00:00:06.569 3479320==    by 0x76C703: postmaster_child_launch
(launch_backend.c:265)
==00:00:00:06.569 3479320==    by 0x774F87: StartBackgroundWorker
(postmaster.c:4197)
==00:00:00:06.569 3479320==    by 0x775277: maybe_start_bgworkers
(postmaster.c:4362)
==00:00:00:06.569 3479320==    by 0x7739F7:
LaunchMissingBackgroundProcesses (postmaster.c:3437)
==00:00:00:06.569 3479320==    by 0x770C2B: ServerLoop (postmaster.c:1737)
==00:00:00:06.569 3479320==    by 0x77037B: PostmasterMain
(postmaster.c:1412)
==00:00:00:06.569 3479320==  Uninitialised value was created by a stack
allocation
==00:00:00:06.569 3479320==    at 0xB43008: SerializeSnapshot
(snapmgr.c:1737)
==00:00:00:06.569 3479320==

and you are spot on here with the explanation with offsets

Size snap_size; 8 bytes;
TransactionId xmin; 4 bytes
TransactionId xmax; 4 bytes
uint32 xcnt; 4 bytes
int32 subxcnt; 4 bytes
bool suboverflowed; 1 byte
bool takenDuringRecovery; 1 byte
/* 2 bytes of padding */
CommandId curcid; 4 bytes

---

From: John Naylor <[email-masked]>
Date: Tue, 7 Apr 2026 17:08:25 +0700

Hi,

mamba is failing with

repack.c: In function 'initialize_change_context':
repack.c:2946:7: error: cast from pointer to integer of different size
[-Werror=pointer-to-int-cast]
 2946 |       (Datum) NULL);
      |       ^
cc1: all warnings being treated as errors

-- 
John Naylor
Amazon Web Services

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 12:30:07 +0200

Hi,

> cc1: all warnings being treated as errors

Hmm, yeah that should have been "(Datum) 0".  Pushed.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The Postgresql hackers have what I call a "NASA space shot" mentality.
 Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 13:19:17 +0200

Hi,

There's one more failure in thorntail
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thorntail&dt=2026-04-07%2006%3A31%3A03
because it runs with wal_level=minimal.  I think we can make it work by
using a temp-config argument to run the tests, as in the attached.

I didn't actually try to run the buildfarm client though ...

---

From: Amit Kapila <[email-masked]>
Date: Tue, 7 Apr 2026 17:45:16 +0530

> deadlock-checker one to protect autovacuum from processing tables under

I have a question based on 0001's commit message: "This patch adds a
new option to logical replication output plugin, to declare that it
does not use shared catalogs (i.e. catalogs that can be changed by
transactions running in other databases in the cluster).". In which
cases, currently plugin needs to access multi-database transactions or
transactions that need to access shared catalogs and on what basis a
plugin can decide that the changes it requires won't need any such
access.

-- 
With Regards,
Amit Kapila.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 14:17:57 +0200

> using a temp-config argument to run the tests, as in the attached.

LGTM. We had almost exactly this in earlier versions of the patch [1], before
commit 67c20979ce.

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 7 Apr 2026 12:32:32 +0000

Dear Álvaro,

Thanks for updating the patch. I have two comments/questions.

01.
```
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -394,6 +394,14 @@ systable_beginscan(Relation heapRelation,
 	SysScanDesc sysscan;
 	Relation	irel;
 
+	/*
+	 * If this backend promised that it won't access shared catalogs during
+	 * logical decoding, this it the right place to verify.
+	 */
+	Assert(!HistoricSnapshotActive() ||
+		   accessSharedCatalogsInDecoding ||
+		   !heapRelation->rd_rel->relisshared);
```

Not sure it's OK to use Assert(). elog(ERROR) might be better if we want to really
avoid the case.

02. SnapBuildProcessRunningXacts

Per my understanding, the db_specic snapshot can be also serialized. Is it
possibility tha normal logical decoding system restores the snapshot and obtain
the wrong result?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 14:33:50 +0200

> plugin can decide that the changes it requires won't need any such

I don't think any plugin needs "multi-database" access as such, but
needing access to shared catalogs is likely normal.  Repack knows it
won't access any shared catalogs, so it can set the flag at ease.

There's a cross-check added in the commit that tests for access to
shared catalogs if the flag is set to false.  I guess you could set it
to false and see what breaks :-)

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Apr 2026 10:10:34 -0400

Hi,

> to false and see what breaks :-)

I think this has a quite high chance of indirect breakages. You just need some
cache invalidation processing / building accessing shared catalogs to violate
the rule, and whether that happens very heavily depends on what cache entries
are present and whether something registers relcache callbacks or such.

This can be triggered by an output function during logical decoding or such,
so you don't really have control over it.

Greetings,

Andres Freund

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 16:19:20 +0200

> possibility tha normal logical decoding system restores the snapshot and obtain

I don't think that the database-specific xl_running_xacts WAL record affects
what SnapBuildSerialize() writes to disk: the contents of builder->committed,
etc. is updated by decoding COMMIT and ABORT records.

On the other hand, with that kind of record, there's probably no reason to
call SnapBuildSerialize(). Good catch, thanks.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 17:38:24 +0200

> so you don't really have control over it.

The REPACK plugin only deforms tuples and writes them to a file, so I think
that things like this should not happen. However, I admit that an option that
allows the plugin developer to declare "I don't need shared catalogs" may be
considered deceptive.

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Apr 2026 11:48:42 -0400

Hi,

On 2026-04-07 17:38:24 +0200, Antonin Houska wrote:
> that things like this should not happen.

You don't need to do it yourself.  It just requires a shared_preload_library
extension to register a relcache invalidation callback that accesses shared
catalog.

It's only kind of an accident that we don't have a case today that accesses
shared catcaches during a relcache build in core (I'm not even sure there's
nothing). You'd just need somebody to add e.g. relcache caching for
publications for that to change. Or look up information about a reloption in
pg_parameter_acl. Or lookup tablespace configuration.

> However, I admit that an option that allows the plugin developer to declare
> "I don't need shared catalogs" may be considered deceptive.

At the very least it would need to be a runtime check rather than just an
assert. This would much more likely to be hit in production because otherwise
it's probably hard to hit the case where shared invalidations happen in the
wrong moment.  And the consequences are corrupted caches, which could cause
all kinds of havoc.

But I think this may need more infrastructure / deeper analysis than what we
can do right now.

Greetings,

Andres Freund

---

From: Antonin Houska <[email-masked]>
Date: Tue, 07 Apr 2026 18:01:16 +0200

> But I think this may need more infrastructure / deeper analysis than what we

ok, thanks a lot for having looked at it.

---

From: Robert Treat <[email-masked]>
Date: Tue, 7 Apr 2026 15:43:50 -0400

> On 2026-Apr-06, Mihail Nikalayeu wrote:
<snip>
> deadlock-checker one to protect autovacuum from processing tables under

I have this lingering bit of paranoia that users could end up in a
situation with a large / long running repack that goes past failsafe
age which prevents the simpler fix of failsafe autovacuum from
running. While the repack finishing would resolve this issue, we can't
know ahead of time that the repack would finish in time, and
statistically speaking, failsafe autovacuum should generally run much
quicker than any repack could. I'm not sure if that means we should
let failsafe vacuum cancel repacks (that seems a bit extreme), but
maybe we want to help $operator to think about this decision, except
if we don't allow autovacuum to wait and we don't allow it to respawn,
I wonder if the end user will ever realize they are in this position.
Granted, there doesn't seem like a clean fix for this...

Robert Treat
https://xzilla.net

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Apr 2026 16:24:04 -0400

Hi,

On 2026-04-07 00:22:32 +0200, Alvaro Herrera wrote:
> From 4303eea0a72408183f9f5afcf8d2801df20f8ffe Mon Sep 17 00:00:00 2001
> From: Antonin Houska <[email-masked]>
> Date: Wed, 1 Apr 2026 17:35:47 +0200
> Subject: [PATCH v56 3/3] Error out any process that would block at REPACK
> 
> Any process waiting on REPACK to release its lock would actually cause
> it to deadlock when it tries to upgrade its lock to AEL, losing all work
> done to that point.  We avoid this by teaching the deadlock detector to
> raise an error when this condition is detected.

I'm rather doubtful that that is ok. Won't this make it basically unsafe to
use repack in way way too many situations? If sessions start erroring out,
rather than wait, because they briefly lock a relation or such it'll imo make
this rather unsafe in production.

ISTM that the proper fix here might be to allow repack to do a lock upgrade
that jumps to the front of the lock's wait queue. The lock upgrade is the
reason for the deadlock, right? And the reason it will often cause a deadlock
is that the repack will be at the tail, rather than the front of the wait
queue. So let's fix that.

Jumping the queue won't make the lock upgrade immediately, of course, but I
think it would fix the issue of the repack, rather than some other process,
getting cancelled?

Greetings,

Andres Freund

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 7 Apr 2026 23:38:11 +0200

> command is going to take a long time.

Okay.  I implemented that now, and here it is.  0001 is as before; 0002
creates shared memory for repack and has autovacuum recheck each table
there before processing it.

Having implemented it, I'm not sure the resulting behavior is all that
different from before.  I mean, the only difference is that the worker
is going to see the table listed in repack shmem and skip it; as opposed
to trying to lock it and be terminated by the deadlock detector one
second later, at which point it continues with the next table on its
list.  So it's some wasted work to set up the autovac work, but nothing
more.

However, there's also the point Andres just made in [1] which basically
kills this idea.  So I'm withdrawing this proposal.  Still, having
written it, I thought it'd be better to get it archived.

[1] https://postgr.es/m/4n4q3preb3lgyhpzstebhux7b2aojhsw7gik4ivaznyggiezrs@lrznutssxlh2

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

---

From: Amit Kapila <[email-masked]>
Date: Wed, 8 Apr 2026 09:19:04 +0530

> etc. is updated by decoding COMMIT and ABORT records.

I think the point is that the other process say a walsender could
restore such a snapshot making it take the wrong decision.

-- 
With Regards,
Amit Kapila.

---

From: Antonin Houska <[email-masked]>
Date: Wed, 08 Apr 2026 09:31:55 +0200

> Granted, there doesn't seem like a clean fix for this...

If REPACK is not going to finish in time, I think it makes little difference
whether VACUUM is allowed to wait or not: even if it waits, it will start just
too late. One reason to avoid waiting might be to allow autovacuum to work on
other tables in between.

I agree that the DBA should have some guidance to asses whether REPACK or
(failsafe) VACUUM is the appropriate action. While failsafe VACUUM is clearly
a means to avoid XID wraparound, I tend to consider REPACK primarily a command
to remove table bloat. Or is there a situation where REPACK is better even to
avoid the wraparound?

Technically, the deadlock can be avoided by not running DDLs on the table
while REPACK is running. I'm just thinking if, by mentioning this in the
REPACK documentation, we'd admit that the REPACK (CONCURRENTLY) feature is
actually incomplete. On the other hand, if we don't mention the risk of
deadlock, it's a similar situation to not mentioning it for commands like
ALTER TABLE: if ALTER TABLE performs table rewrite, deadlock can also result
in a significant amount of wasted resources. (Of course, it's not the same if
the purpose of REPACK is considered substitute for failsafe VACUUM, but I'm
not sure about that.)

---

From: Amit Kapila <[email-masked]>
Date: Wed, 8 Apr 2026 14:05:49 +0530

On Wed, Apr 8, 2026 at 1:54 AM Andres Freund <[email-masked]> wrote:
> I'm rather doubtful that that is ok.
>

Another possible idea is that after copying table_data to the new
table, we mark the old table as in_use_by_repack and release the
ShareUpdateExclusiveLock on the old table. Then the function
CheckTableNotInUse() should be updated to give an ERROR if the table
is marked as in_use_by_repack. Now, acquiring AEL by repack
(concurrently) should be safe because all concurrent DDLs should be
errored out due to flag in_use_by_repack. Can this address the problem
we are worried about the lock upgrade?

-- 
With Regards,
Amit Kapila.

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 8 Apr 2026 12:20:53 +0200

Hi,

while building on a rpi5 with a 32-bit system, I'm getting these warnings:

config.status: linking src/makefiles/Makefile.linux to src/Makefile.port
In file included from ../../../src/include/access/tupmacs.h:20,
                 from ../../../src/include/access/htup_details.h:20,
                 from ../../../src/include/access/relscan.h:17,
                 from ../../../src/include/access/heapam.h:19,
                 from repack.c:36:
In function ‘VARSIZE_ANY’,
    inlined from ‘restore_tuple’ at repack.c:2731:15:
../../../src/include/varatt.h:243:51: warning: array subscript
‘varattrib_4b[0]’ is partly outside array bounds of ‘union
<anonymous>[1]’ [-Warray-bounds=]
  243 |         ((((const varattrib_4b *) (PTR))->va_4byte.va_header >>
2) & 0x3FFFFFFF)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
../../../src/include/varatt.h:467:24: note: in expansion of macro
‘VARSIZE_4B’
  467 |                 return VARSIZE_4B(PTR);
      |                        ^~~~~~~~~~
repack.c: In function ‘restore_tuple’:
repack.c:2717:49: note: object ‘chunk_header’ of size 4
 2717 |                         }                       chunk_header;
      |                                                 ^~~~~~~~~~~~
In function ‘VARSIZE_ANY’,
    inlined from ‘restore_tuple’ at repack.c:2734:4:
../../../src/include/varatt.h:243:51: warning: array subscript
‘varattrib_4b[0]’ is partly outside array bounds of ‘union
<anonymous>[1]’ [-Warray-bounds=]
  243 |         ((((const varattrib_4b *) (PTR))->va_4byte.va_header >>
2) & 0x3FFFFFFF)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
../../../src/include/varatt.h:467:24: note: in expansion of macro
‘VARSIZE_4B’
  467 |                 return VARSIZE_4B(PTR);
      |                        ^~~~~~~~~~
repack.c: In function ‘restore_tuple’:
repack.c:2717:49: note: object ‘chunk_header’ of size 4
 2717 |                         }                       chunk_header;
      |                                                 ^~~~~~~~~~~~

I'm not sure if it's just the compiler (gcc 14.2) being pesky, or if
it's an actual issue. The repack tests seem to pass fine.

regards

-- 
Tomas Vondra

---

From: Antonin Houska <[email-masked]>
Date: Wed, 08 Apr 2026 12:46:46 +0200

> it's an actual issue. The repack tests seem to pass fine.

We already introduced this definition above in the function to suppress this
kind of warning

    union
    {
	    alignas(int32) varlena hdr;
	    char		data[sizeof(void *)];
    }			chunk_header;

The problem on a 32-bit system probably is that sizeof(void *) is 4. We need
some other constant. Maybe (sizeof(varlena) + 1) ...

Thanks.

---

From: Tom Lane <[email-masked]>
Date: Wed, 08 Apr 2026 12:47:33 -0400

Antonin Houska <[email-masked]> writes:
>> ...

Several 32-bit BF animals are showing these too (so far: dodo,
grison, and turaco).

> some other constant. Maybe (sizeof(varlena) + 1) ...

This seems unnecessarily Rube Goldberg-ish already.  Why not just

	uint64	chunk_header;

It will not hurt anything if the variable has more-than-required
alignment.  And it'd be better if it were the same size everywhere.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 8 Apr 2026 13:22:41 -0400

Hi,

> we are worried about the lock upgrade?

I don't think this is a viable path.  You need to prevent any further lock
acquisitions on the relation to be able to swap it, not just conflicting DDL.
And you need to wait for all pre-existing locks to have been released.  That
doesn't really get easier by what you propose.

I don't think CheckTableNotInUse() would work anyway - don't we already hold
locks by the point we call it? And even if that were not the case, there are
several paths to locking relations that don't ever go anywhere near
CheckTableNotInUse().

Greetings,

Andres Freund

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 9 Apr 2026 01:36:00 +0200

Hello, Andres!

> acquisitions on the relation to be able to swap it, not just conflicting DDL.

AFAIU, Amit's main idea is that we currently upgrade the lock instead
of **releasing and re-acquiring** it because we fear DDL between those
actions.
DML actions are ok for us; REPACK will wait for them while getting
AEL. Later changes will be applied by REPACK backend while holding
AEL.

One more thing we may prevent from sneaking into that hole is a
VACUUM. It will not break anything, but will be huge waste of time and
resources.

> doesn't really get easier by what you propose.

 Do you mean locks from other sessions accessing the table? Is it done
automatically while waiting for AEL?

> locks by the point we call it?

Yes, the DDL session already holds locks by the time
CheckTableNotInUse is called - but is that really the problem? They
will be released on error.

> And even if that were not the case, there are

But those aren't DDL, so they shouldn't be the problem (CREATE TRIGGER
might be - it seems to ignore CheckTableNotInUse, but perhaps it's
fine).

So, in my undeerstanding Amit's idea has two parts: "set flag and
release/re-acquire" + "use CheckTableNotInUse (or some place like
that) to check the flag and fail for DDL commands."

Reading your arguments again, they all seem valid under the assumption
that we still hold SUEL while requesting AEL.
I suspect you may have just skimmed past the 'release/re-acquire' part
- but more probably I missed something.

Best regards,
Mikhail.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Apr 2026 10:24:05 +0530

> AEL. Later changes will be applied by REPACK backend while holding

Yes, this is exactly what I had in mind.

> VACUUM. It will not break anything, but will be huge waste of time and

We can prevent other commands (if required) by checking the
rel_in_use_by_repack flag but I thought for the initial version it is
better to do what is minimally required.

> automatically while waiting for AEL?

Right and this is already the case with the code where locks
not-conflicting with ShareUpdateExclusiveLock could be present before
we try to upgrade the lock. The point was we will not let DDL execute
on the table after the new flag (rel_in_use_by_repack) is set and we
released the ShareUpdateExclusiveLock.

> will be released on error.

Yes, that is the key to solve this problem. Let me take an example to
explain this a bit more. Right now, the problem can happen in the
following kind of sequence.

Session-1:
REPACK (CONCURRENTLY) foo;

-- now say after the above command has acquired
ShareUpdateExclusiveLock and is doing the work of copying the table,
Session-2 did following actions.

Session-2:
Select * from foo; -- this is allowed
ALTER TABLE foo ADD COLUMN c2; -- this will blocked as Session-1
already has acquired ShareUpdateExclusiveLock

Session-1:
-- continues and tries to upgrade the lock to AEL. This leads to
deadlock ERROR in the current session doing REPACK (CONCURRENTLY).

Now, with the solution I proposed, because we will release
ShareUpdateExclusiveLock after setting a flag like
rel_in_use_by_repack, the ALTER TABLE in session-2 will succeed but
will error_out by CheckTableNotInUse(or a similar function that checks
rel_in_use_by_repack). Both with and without this solution, acquiring
AEL by REPACK (CONCURRENTLY) needs to wait concurrent locks like the
one for SELECT in above example that could have been acquired during
the time REPACKing had ShareUpdateExclusiveLock.

> that) to check the flag and fail for DDL commands."

True, this is the core of the idea.

-- 
With Regards,
Amit Kapila.

---

From: Antonin Houska <[email-masked]>
Date: Thu, 09 Apr 2026 08:59:00 +0200

> This seems unnecessarily Rube Goldberg-ish already.

Indeed.

> Why not just

That's certainly better. Thanks.

---

From: Antonin Houska <[email-masked]>
Date: Thu, 09 Apr 2026 10:43:14 +0200

> True, this is the core of the idea.

This approach LGTM when it comes to concurrent DDLs. However, consider REPACK
holding ShareUpdateExclusiveLock (SUEL) and VACUUM (w/o VACOPT_SKIP_LOCKED)
waiting for the same lock. Once REPACK releases its SUEL, VACUUM gets it and
processes the table, then REPACK finally gets AccessExclusiveLock (AEL) and
finishes too. Nothing went wrong from the data consistency POV, however the
VACUUM proably wasted a lot of resources, because REPACK does "more than
VACUUM". Furthermore, while REPACK was waiting for the AEL (and the VACUUM was
running), a *lot of* DMLs could have been executed on the table, and REPACK
will have to replay those while holding AEL. The point is that REPACK should
hold AEL for as short time as possible.

What Andres proposed (AFAIU) should help to avoid this problem because
REPACK's request for AEL would get in front of the VACUUM's request for SUEL
in the queue. Of course, VACUUM would eventually run too, but - if REPACK
succeeded - it'd be much cheaper because it would (supposedly) find very
little work to do.

Anti-wraparound (failsafe) VACUUM is a bit different case [1] (i.e. it should
possibly have higher priority than REPACK), but I think this prioritization
should be implemented in other way than just letting it get in the way of
REPACK (at the time REPACK is nearly finished).

[1] https://www.postgresql.org/message-id/CABV9wwMrrP4S54jPGn5D-a8AbJm%2Be5%2BWORs6ykUBgXdc-%2B%2BNtQ%40mail.gmail.com

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 9 Apr 2026 11:11:16 +0200

Hi!

On Thu, Apr 9, 2026 at 10:43 AM Antonin Houska <[email-masked]> wrote:
> finishes too.

> VACUUM. It will not break anything, but will be huge waste of time and

I thought about that too, I think we may just add some kind of
CheckTableNotInUse in VACUUM after getting the SUEL.

Mikhail.

---

From: Antonin Houska <[email-masked]>
Date: Thu, 09 Apr 2026 11:26:29 +0200

> CheckTableNotInUse in VACUUM after getting the SUEL.

Sure, it's possible, but IMO the principal question is whether REPACK should
let VACUUM and DDLs error out, or just let them wait.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 9 Apr 2026 16:06:17 +0200

Hello!

> let VACUUM and DDLs error out, or just let them wait.

One more idea: instead of ERROR in CheckTableNotInUse in case of
in_repack - just release the lock and retry a little later (by that
time, the repack operation will likely have acquired the AEL).

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Apr 2026 10:13:26 -0400

Hi,

> of **releasing and re-acquiring** it because we fear DDL between those

I got that, I just don't think it's going to work in any sort of way
reasonably well.

The fix here should be to make the system understand how to make lock upgrades
as safe as possible, not to hack around the corners.  For that you need to
teach the deadlock detector about all of this properly.

> will be released on error.

There's no guarantee they get there if it's done after the lock
acquisition. It can be part of a more complicated lock cycle.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Apr 2026 10:20:18 -0400

Hi,

On 2026-04-09 10:43:14 +0200, Antonin Houska wrote:
> in the queue.

Note that that already happens today.

This works today (without the error triggering patch):

S1: REPACK starts
S2: LOCK TABLE / VACUUM / ... starts waiting
S1: REPACK tries to get AEL
S1: REPACK's lock requests get reordered in the wait queue to be before S2 and
    just gets the lock
S1: REPACK finishes
S2: lock acquisition completes.

That's because we do already have this "jumping the wait queue" logic, which I
had forgotten about.

What does *not* work is this:

S1: REPACK starts
S2: BEGIN; SELECT 1 FROM table LIMIT 1;
S2: LOCK TABLE / VACUUM / ... starts waiting
S1: REPACK tries to get AEL
S1: lock is not granted, can't be reordered to be before S2, because S2 holds
    conflicting lock, deadlock detector triggers
S2: lock acquisition completes

But with my proposal to properly teach the deadlock detector about assuming
there's a wait edge for the eventual lock upgrade by S1, the first example
would still work, because the lock upgrade would not be considered a hard
cycle, and the second example would have S2 error out.

> REPACK (at the time REPACK is nearly finished).

Yea, it makes no sense to interrupt the long running repack, given that the
new relation will have much less stuff for vacuum to do.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Apr 2026 10:26:22 -0400

Hi,

> time, the repack operation will likely have acquired the AEL).

I continue to think this approach has no chance of working.  Even if it
theoretically could, there are lots of paths to locking relations that do not
go through CheckTableNotInUse(), and we're not going to just route them all
through CheckTableNotInUse().

What CheckTableNotInUse() is for is to prevent DDL from changing the structure
of the table when it is still being referred to.  You can't just call
CheckTableNotInUse() from a LOCK TABLE - it would trigger wrong errors *ALL
THE TIME* because a LOCK TABLE does not need to error out just because there's
also a cursor on the table.

And it'd trigger lots of bogus errors. See the first example in
https://postgr.es/m/fpr4nsmyy3mpfrm2mijspr44dgol2cjeke5tyznb4btsznxsgx%40iifdbfe2wl63

S2 would get the lock and then error out due to the proposed check. Even
though there's no need for it.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Apr 2026 10:34:52 -0400

Hi,

> though there's no need for it.

Before you protest that you could just let the non-DDL operation have its
lock, sure, but that's an utterly terrible idea, because it will lead to more
and more work accumulating that then has to happen when the AEL is actually
held.

Greetings,

Andres Freund

---

From: Alexander Lakhin <[email-masked]>
Date: Fri, 10 Apr 2026 10:00:00 +0300

Hello Alvaro,

08.04.2026 00:38, Alvaro Herrera wrote:
> Okay.  I implemented that now, and here it is.  ...

Could you please look at an assertion failure produced by the following
script, starting from 0d3dba38c:?
createdb db1
createdb db2

echo "
CREATE TABLE t0 (a text);
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
INSERT INTO t0 VALUES ('a');
SELECT pg_sleep(1);
" | psql db1 &

echo "
CREATE TABLE t1 (id int PRIMARY KEY);
CREATE TABLE t2 (id int PRIMARY KEY, a TEXT, FOREIGN KEY (id) REFERENCES t1);
SET min_parallel_table_scan_size = 1;
REPACK (CONCURRENTLY) t2;
" | psql db2
wait

It triggers for me:
TRAP: failed Assert("TransactionIdPrecedesOrEquals(TransactionXmin, RecentXmin)"), File: "procarray.c", Line: 2071, PID: 
3529520
postgres: parallel worker for PID 3529517 (ExceptionalCondition+0x69)[0x62f724b19c4c]
postgres: parallel worker for PID 3529517 (+0x522456)[0x62f72498e456]
postgres: parallel worker for PID 3529517 (GetSnapshotData+0x6b)[0x62f72498f50c]
postgres: parallel worker for PID 3529517 (GetNonHistoricCatalogSnapshot+0x4b)[0x62f724b5bd90]
postgres: parallel worker for PID 3529517 (GetCatalogSnapshot+0x20)[0x62f724b5ce7b]
postgres: parallel worker for PID 3529517 (systable_beginscan+0x10b)[0x62f7245c31e7]
postgres: parallel worker for PID 3529517 (+0x69e1e7)[0x62f724b0a1e7]
postgres: parallel worker for PID 3529517 (+0x69e5a6)[0x62f724b0a5a6]
postgres: parallel worker for PID 3529517 (+0x6a4d86)[0x62f724b10d86]
postgres: parallel worker for PID 3529517 (RelationIdGetRelation+0x83)[0x62f724b11208]
postgres: parallel worker for PID 3529517 (relation_open+0x1e)[0x62f72456c235]
...
2026-04-10 05:59:51.471 UTC [3529495] LOG:  background worker "parallel worker" (PID 3529520) was terminated by signal 
6: Aborted

Best regards,
Alexander

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 10 Apr 2026 10:53:53 +0000

Hi,

When testing REPACK concurrently, I noticed that all WALs are retained from
the moment REPACK begins copying data to the new table until the command
finishes replaying concurrent changes on the new table and stops the repack
decoding worker.

I understand the reason: the REPACK command itself starts a long-running
transaction, and logical decoding does not advance restart_lsn beyond the
oldest running transaction's start position. As a result, slot.restart_lsn
remains unchanged, preventing the checkpointer from recycling WALs.

However, since REPACK can run for a long time (hours or even days), I'd like
to confirm whether this is expected behavior or if we plan to improve it
in the future ? And additionally, IIUC, REPACK without using concurrent option
does not have this issue.

Given that we do not restart a REPACK, I think the repack decoding worker
should be able to advance restart_lsn each time after writing changes
(similar to how a physical slot behaves). To illustrate this, I've written
a patch (attached) that implements this approach, and it works fine for me.

BTW, catalog_xmin also won't advance, but that seems not a big issue as
the REPACK transaction itself also holds a snapshot that retains catalog tuples,
so advancing catalog_xmin wouldn't change the situation anyway.

Thoughts ?

Best Regards,
Hou zj

---

From: Antonin Houska <[email-masked]>
Date: Fri, 10 Apr 2026 12:57:47 +0200

Alexander Lakhin <[email-masked]> wrote:

> TRAP: failed Assert("TransactionIdPrecedesOrEquals(TransactionXmin, RecentXmin)"), File: "procarray.c", Line: 2071, PID: 3529520

Attached is a fix that works for me.

Nevertheless, REPACK (CONCURRENTLY) in your test goes ahead only due to commit
0d3dba38c7, which will probably be reverted [1]. Then REPACK will wait for the
transaction in the other database (db1) to complete before it can actually
start.

Thanks for the report!

[1] https://www.postgresql.org/message-id/cdgw4sbbfcgk6du3iv54r2dgiy4tfywoklbotlmj4irxavdcr3@glxfw5jj277q

---

From: Antonin Houska <[email-masked]>
Date: Fri, 10 Apr 2026 15:21:45 +0200

> remains unchanged, preventing the checkpointer from recycling WALs.

I think you're right, sorry for the omission.

> in the future ? And additionally,

Yes, it will be improved. I have a draft patch for it, will rebase and post it
soon. The plan is to:

1) preserve the original xmin/xmax of the tuples when we insert them into the
new heap. Thus, besides achieving MVCC safety, we won't need XID assigned for
most of the time.

2) do catalog changes in separate transactions - XID needed here, but these
transactions take very short time.

3) use a single snapshot only for limited number of tuples/pages. When more
data needs to be copied, a new snapshot is built, supposedly with higher
->xmin than the prevous one.

> IIUC, REPACK without using concurrent option does not have this issue.

It does not have the WAL recycling issue because it does not need to read
WAL. However it also runs in a long transaction. Even though it does not need
XID for the actual heap rewriting, it gets one at the moment it locks the
table using AccessExclusiveLock (which is at the very beginning).

> a patch (attached) that implements this approach, and it works fine for me.

LGTM, thanks!

> so advancing catalog_xmin wouldn't change the situation anyway.

The snapshot "resetting" (mentioned above) should fix this problem too.

---

From: Robert Treat <[email-masked]>
Date: Fri, 10 Apr 2026 12:14:59 -0400

> That's because we do already have this "jumping the wait queue" logic, which I

You know, I was wondering how this wasn't already a problem for
pg_repack/pg_squeeze, and I guess this explains it :-P

> cycle, and the second example would have S2 error out.

In the above S2 will error out if you try to run a VACUUM, but the
point still stands that calling an explicit LOCK or similar could lead
to this issue. In the current repack world, we document the need for
lock escalation at the end of the repacking and caution that doing
things like DDL or explicit LOCKing could cause trouble, so don't do
that. What you're proposing above would be an improvement though,
IMHO.

> new relation will have much less stuff for vacuum to do.

We might be talking about 2 different scenarios. In the case where we
are at the point of lock escalation, you would probably want the
repack to get priority over a waiting vacuum, even a failsafe vacuum.
But outside of that scenario, we can't know that the repack is the
better option (and statistically it probably isn't) since a repack
that is actively copying rows might still need to rebuild some large
number of indexes (or just some really expensive index) which could
take significantly longer than a failsafe vacuum would need to ensure
wraparound avoidance. I don't think we'd go as far as saying the
failsafe vacuum should cancel the repack, but I think ideally we'd
like it to not be canceled either, since that would increase
likelihood for dba/monitoring to pick up on the situation, and in the
case that REPACK fails for some reason, the failsafe vacuum could
immediately start working without having to go through any additional
hoops.

Robert Treat
https://xzilla.net

---

From: Antonin Houska <[email-masked]>
Date: Fri, 10 Apr 2026 20:56:42 +0200

> immediately start working without having to go through any additional

What about just teaching the anti-wraparound VACUUM to print out a WARNING if
it could not lock the table in "reasonable" time? The DBA would then have to
consider if the blocking command should be cancelled, whether it's REPACK or
something else.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sun, 12 Apr 2026 15:31:20 +0200

Hello!

> cycle, and the second example would have S2 error out.

Attached patch contains some (maybe naive) POC for similar approach.
It adds a 'deadlock_protected' flag, which changes how the deadlock
detector cancels backends.

Instead of cancelling the backend entered the deadlock detector - it
cancel some another (nearest hard edge) until it is possible to get the
lock (either by
reordering or directly).

Also, I added test cases with the scenarios you mentioned into the repack
spec - to ensure repack is still working while other backend are cancelled.

> new relation will have much less stuff for vacuum to do.

For now I decided to keep anti-wraparound unaffected because the
feature is may be used not only for repack but also for other potential
scenarios.
But yes, maybe it's worth canceling antiwraparound in the repack case.
Also, checking proc flags to detect antuwraparound requires
LWLockConditionalAcquire(ProcArrayLock) -  something I don't like in
deadlock detector.

Best regards,
Mikhail.

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 12 Apr 2026 17:00:00 +0300

Hello Antonin and Alvaro,

> transaction in the other database (db1) to complete before it can actually

Thank you for the fix!

I've stumbled upon one more issue with this feature:
CREATE TABLE t (i int PRIMARY KEY);
REPACK (CONCURRENTLY) t;

fails for me with sanitizers enabled and
min_dynamic_shared_memory = '1GB'
in postgresql.conf as below:
2026-04-12 13:23:02.000 UTC [2733633] LOG:  statement: REPACK (CONCURRENTLY) t;
repack.c:3373:15: runtime error: load of value 240, which is not a valid value for type '_Bool'
     #0 0x6441f7eba454 in start_repack_decoding_worker .../src/backend/commands/repack.c:3373
     #1 0x6441f7ebdaad in rebuild_relation .../src/backend/commands/repack.c:1010
     #2 0x6441f7ebe9a2 in cluster_rel .../src/backend/commands/repack.c:656
     #3 0x6441f7ebefea in process_single_relation .../src/backend/commands/repack.c:2359
     #4 0x6441f7ebf870 in ExecRepack .../src/backend/commands/repack.c:296
     #5 0x6441f886f20e in standard_ProcessUtility .../src/backend/tcop/utility.c:867
...

2026-04-12 13:23:03.620 UTC [2733620] LOG:  client backend (PID 2733633) was terminated by signal 6: Aborted
2026-04-12 13:23:03.620 UTC [2733620] DETAIL:  Failed process was running: REPACK (CONCURRENTLY) t;

Could you please have a look?

Best regards,
Alexander

---

From: Andres Freund <[email-masked]>
Date: Sun, 12 Apr 2026 10:02:59 -0400

Hi,

On 2026-04-10 12:14:59 -0400, Robert Treat wrote:
> wraparound avoidance.

I don't really understand why you consider REPACK CONCURRENTLY to be so
different from other existing long-running operations?

> I don't think we'd go as far as saying the failsafe vacuum should cancel the
> repack, but I think ideally we'd like it to not be canceled either

I don't think what I suggested could lead to (auto-)vacuum getting cancelled?
Antonin's earlier patch could, but as long as you only do cancellations if
there's an actual deadlock cycle, VACUUM should not get cancelled, since it
won't already hold a lower level lock?

> , since that would increase likelihood for dba/monitoring to pick up on the
> situation, and in the case that REPACK fails for some reason, the failsafe
> vacuum could immediately start working without having to go through any
> additional hoops.

Not sure I buy this (leaving aside the premise doesn't hold, I think). If you
have monitoring for either long running tasks or tables not getting
autovacuumed, it'd pick up on that regardless of whether autovacuum is getting
cancelled or not.  What realistic monitoring would pick up on autovacuum just
being stuck waiting for a lock for long, but would not pick up on repack
running forever or age(datfrozenxid) getting a bit long in the tooth?

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Sun, 12 Apr 2026 10:05:34 -0400

Hi,

> cancel some another (nearest hard edge) until it is possible to get the

I don't think that's as good.  The problem is that that way you're only
detecting the deadlocks once they have materialized (i.e. once repack actually
does the lock upgrade), rather than cancelling when we know that the problem
starts.  Having sessions pointlessly blocked for many hours is bad.

> spec - to ensure repack is still working while other backend are cancelled.

I think we should perhaps commit spec tests for these (I've not yet reviewed
them, but in principle), even before we fix the problem. It's good to document
the current behavior and have a comment that the wrongly cancelled case should
not trigger an error.

Greetings,

Andres Freund

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sun, 12 Apr 2026 16:58:31 +0200

Hi!

> starts.  Having sessions pointlessly blocked for many hours is bad.

O, I think I understand you now.
You propose to somehow mark SUEL as "going to become AEL, so, the
deadlock detector should treat it as such".
In that case LOCK TABLE should get some kind of "future deadlock detected".

Yep, that feels much better. I'll try to check that approach tomorrow.

Best regards,
Mikhail.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 13 Apr 2026 11:42:03 +0200

>     #5 0x6441f886f20e in standard_ProcessUtility .../src/backend/tcop/utility.c:867

I could not reproduce the problem, but noticed that the field is not
initialized correctly. Please confirm that 0001 should fix that.

While working on it, I noticed that one field can be removed from
DecodingWorkerShared - 0002 removes that.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 14 Apr 2026 15:37:56 +0200

Andres Freund <[email-masked]> wrote:

> starts.

This is my hack that tries to do that.

---

From: Andres Freund <[email-masked]>
Date: Tue, 14 Apr 2026 09:58:55 -0400

Hi,

> This is my hack that tries to do that.

I still think this needs to be in the deadlock detector.  The lock cycle just
needs to be a bit more complicated for a hack in JoinWaitQueue not to work.
There's no guarantee that the wait that triggers the deadlock is actually on
the relation being repacked.

Greetings,

Andres Freund

---

From: Mihail Nikalayeu <[email-masked]>
Date: Tue, 14 Apr 2026 18:55:17 +0200

Hello!

> the relation being repacked.

I have started prototyping a way to declare a "future" lock which the
deadlock detector treats as a hard edge.
But I currently stuck on issues related to the fact that SUE doesn't
force weak locks (fast-path) to go through
FastPathTransferRelationLocks, so the deadlock detector can't handle
the case when another backend tries to execute LOCK TABLE repack_test
IN SHARE UPDATE EXCLUSIVE MODE;
Also, VACUUM takes the same lock.

I'm not sure how to deal with this in a non-hacky way. One option is
to force SUE to transfer locks if relation it is trying to lock
relation which marked with "future lock". But I am not sure it is good
enough or covers all tricky cases (multiple backends in the loop).

Best regards,
Mikhail.

---

From: Antonin Houska <[email-masked]>
Date: Wed, 15 Apr 2026 16:50:11 +0200

> the relation being repacked.

ok, I see.

I thought of a "hypothetical graph", which would include the to-be-granted
lock, but the major issue is that it will not work correctly without the
locking the LMGR's LW locks we do in CheckDeadLock():

    for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
        LWLockAcquire(LockHashPartitionLockByIndex(i), LW_EXCLUSIVE);

And obviously, doing this each time we want to insert a lock into the queue
would be bad for performance. It's even mentioned in the storage/lmgr/README
that the current approach is optimistic, so I think that major rework would be
needed if we wanted to entirely avoid waiting that leads to deadlock.

The approach proposed by Mihail [1] seems the least problematic to me, and
something like that occurred to me when I thought about the problem the first
time. However, when we wake up the other processes in order to run the
deadlock detection, they should do that immediately. I've got no good idea
about implementation at the moment, since latch can be set for unrelated
reasons. (Besides that, I have some more questions about this patch, which I
can post separately.)

[1] https://www.postgresql.org/message-id/CADzfLwURKVNQ%2B%2BDpi7bjoGfj-8pchDQEVex3eWBx0NCYn6TbDQ%40mail.gmail.com

---

From: Andres Freund <[email-masked]>
Date: Wed, 15 Apr 2026 10:59:51 -0400

Hi,

On 2026-04-15 16:50:11 +0200, Antonin Houska wrote:
> would be bad for performance.

Hence my suggestion to do this as part of the deadlock check. Then we don't do
this unnecessary work outside of the case where we actually need it.

That does need to deal with the case of the deadlock check running first in
the backend doing repack, but that's not that hard - I think it'd be good
enough to set its deadlock timeout temporarily to a higher value. The backend
*should* still run the deadlock detector, because it could probably still get
into a deadlock (e.g. due to a pg_class access or something).

Greetings,

Andres Freund

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 15 Apr 2026 17:54:12 +0200

Hello!

> reasons. (Besides that, I have some more questions about this patch, which I

It is already possible to "deadlock" ANOTHER backend while running the
deadlock check [0].
Supported in current infra, just not used at the moment (my POC used that also).

[0]: https://github.com/postgres/postgres/blob/master/src/backend/storage/lmgr/proc.c#L1870-L1873

---

From: Antonin Houska <[email-masked]>
Date: Wed, 15 Apr 2026 20:28:24 +0200

> into a deadlock (e.g. due to a pg_class access or something).

Yes, the question is when we should run that check. I thought that it should
happen during each lock acquisition, and that made me worried about
performance. AFAIU you suggest REPACK to do something like:

1. Acquire ShareUpdateExclusiveLock

2. Perform the "enhanced check" to see if a future request for
   AccessExclusiveLock can trigger a deadlock.

3. Do major part of the work (copy the table, build indexes, ...)

4. Request AccessExclusive lock.

5. Finish the work (process the remaining concurrent changes and swap the
   table files).

This makes me concerned that if another session does

BEGIN;
TABLE t;

between steps 2 and 4, and something like

-- Get AccessExclusiveLock
ALTER TABLE t ADD COLUMN j int;

just after step 4, the two session will probably up in a deadlock anyway. In
other words, even if REPACK does the check early, it does not prevent other
sessions from getting in the way.

Maybe I'm still missing something.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 16 Apr 2026 13:18:55 +0200

Hello!

> Maybe I'm still missing something.

I was trying to solve that by using another approach: the ability to
define a "future lock" [0]. It is declared even before taking the
actual lock, so, no race is possible.

That approach works correctly except one case -
ShareUpdateExclusiveLock from another backend, I described it here [1]
a little bit.

For now, I don't know how to solve it without a performance downgrade.

[0]: https://github.com/michail-nikolaev/postgres/commit/ba0f4247dad3d96b8282cd18056b7776cd69317c
[1]: https://www.postgresql.org/message-id/flat/CADzfLwU8Qw6LXFHO7Tbjc-O7o%2BtM26jdnOJBWqYLu61rf7bO%2Bg%40mail.gmail.com#1e96f8882363afb2fc53c2f08346f527

---

From: Mihail Nikalayeu <[email-masked]>
Date: Fri, 17 Apr 2026 04:01:00 +0200

Hello!

I think I got working POC for deadlock-detector enhancements for
REPACK (and potentially other).

Each PGPROC gets one FutureWaitLock slot: a (locktag, mode) pair,
which is empty when locktag_lockmethodid == 0.
A backend calls LockDeclareFutureWait() to publish its intent. Only
REPACK (CONCURRENTLY) uses it today, declaring its future
AccessExclusiveLock.

Only the owning backend writes its slot; it does so under the
partition lock covering the declared tag. Remote readers (the deadlock
detector) must hold that partition lock to observe it.
The detector holds all partition locks during its graph walk, so it
can read any slot safely. The slot clears in LockAcquireExtended() at
the exact point the backend attaches its waitLink for the same tag and
mode.
It is also cleared on abort, backend exit, and end of transaction.

FindLockCycleRecurseFuture() treats the declared future lock as a hard
edge to (a) current holders whose mode conflicts and (b)
already-queued waiters that JoinWaitQueue() would place ahead of the
future request.

Weak relation locks are kept in per-backend fast-path arrays and
remain invisible to the detector's graph walk.
Normally LockAcquireExtended() migrates them to the main lock table
when a conflicting strong lock is requested.
This action is gated by FastPathStrongRelationLocks->count to prevent
new fast-path lock processing.

A future-wait declaration does not trigger that migration due to
performance considerations.

Instead CheckDeadLock() now snapshots currently declared
fast-path-incompatible future waits and increments
FastPathStrongRelationLocks->count for those tags (blocking new
fast-path acquisitions).
It then calls FastPathTransferRelationLocks() for each such tag to
move existing holders into the main table, making everything visible
to the deadlock walker.
Once the deadlock checker finishes, FastPathStrongRelationLocks->count
is decremented enabling fast-path processing again. Performance
degradation occurs only during deadlock processing and only if REPACK
is active.

If a deadlock contains a "future" edge it is reported as "future
deadlock detected".

Repack first declares future AE and only then proceeds to actually get SUE.

Added test cases for different scenarios, including SUE from another
backend and a 3-backend deadlock.

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 17 Apr 2026 09:44:12 -0500

I had trouble testing this at first:

postgres=# REPACK (CONCURRENTLY) users;
ERROR:  42501: permission denied to use replication slots
DETAIL:  Only roles with the REPLICATION attribute may use replication slots.
CONTEXT:  REPACK decoding worker
LOCATION:  CheckSlotPermissions, slot.c:1697

That's surprising, since it was run as a superuser.

It turns out that repack runs as the owner of the table, and the table
*owner* needs to have REPLICATION -- regardless of who runs the command.

I imagine people have been testing with one user, that both owns the
table and invokes REPACK.  Maybe this just needs to be clarified in the
documentation/error message?

-- 
Justin

---

From: Antonin Houska <[email-masked]>
Date: Sat, 18 Apr 2026 21:23:08 +0200

> REPACK (and potentially other).

That looks interesting, I'll check it.

I've also thought about the problem quite a bit this week. I tried to add a
pointer to PGPROC that, like ->waitLock, points to the lock being acquired,
but it's initialized before the actual waiting starts. I adjusted the deadlock
detector to use that pointer too, but it did not work. The problem was
probably that the lock wasn't in the queue during the check.

Finally it occurred to me that a new field can be added to the LOCK structure,
indicating that the lock is being upgraded. It enforces some extra deadlock
checks by other processes, so that the upgrading process does not have to care
about deadlock detection at all. More info in the commit message.

It should handle all the cases in your tests, however a new injection point
would be needed. (Not added yet.)

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sun, 19 Apr 2026 00:46:00 +0200

Hello, Antonin!

I've briefly looked at your patch. As I understand it, it cancels the
other process only when REPACK actually tries to upgrade the lock. In
that sense, its approach is close to my variant at [0].

AFAIU, Andres's concern is that the "victim" should be cancelled
sooner, rather than waiting until REPACK actually attempts the
upgrade. I was trying to solve it by
[1].

 -------------------------------

There are some comments on [1]:

Some details related to POC from previous letter (once I re-read that
in the morning I relized it is not so easy to understand, sorry).

So, goal of patch to:
* make sure REPACK will survive deadlock which may caused by lock upgrade
* reject some other backend with error early than deadlock really
occur - to avoid pointless waiting for other commands
* implement it at deadlock detector level to correctly handle
different 2+ backend-involved scenarious

To achive it the first idea is to add some kind of "future lock"
(FutureWaitLock). It may declare an intention to acquire a lock of a
certain level as high-priority in the future.
Once deadlock detector starts to walking graph it treat that intention
like an actual "waiting".
That way deadlock detector looks for one more step in the future -
moment of actual acquiring the "future" lock - and if it ends with
cycle - reject waiting backend ("future deadlock detected").

Looks like best place to put that FutureWaitLock is PGPROC itself.

Few moments to consider:
* it is not allowed to declare a future lock if backend already holds
some kind of lock for the same tag (this may cause a race condition).
* so, REPACK first declares future Access Excluvie and only after it
Share Update Exclusive
* declared future lock should be >= SUE

So far everything looks good.

In case of that scenario:

S1: BEGIN; SELECT * FROM t;
S2: REPACK (CONCURRENTLY) t;
S1: LOCK TABLE t in ACCESS EXCLUSIVE MODE <--- "future deadlock detected"

Deadlock detector sees:

S1 ----> S2 (waiting for AE conflicting with SUE)
S2 ----> S1 (future wait for AE conflict with current Access Share)

But there is a tricky case related to SUE:

S1: BEGIN; SELECT * FROM t;
S2: REPACK (CONCURRENTLY) t;
S1: LOCK TABLE t in SHARE UPDATE EXCLUSIVE MODE;

In that case we have almost the same deadlock scenario - but that is
not visible to deadlock detector.
It happens because SUE does not force all locks taken on 't' to be
transfered using FastPathTransferRelationLocks into the main table
(SUE is does not ConflictsWithRelationFastPath).
Because of it S2 -> S1 edge is not visible by deadlock detector
(Access Share is held using fast-path).

To deal with it we may force any relation with FutureWaitLock to
through slow-path locking - but I don't think it is acceptable.

Instead next approach is proposed:

* deadlock detector checks if any "future" locks are present in the
system (counter in shared memory)
* if so - it iterates over all PROCs to collect relations which are
"future locked"
* for each such relations - FastPathTransferRelationLocks called and
slow-path is forced (FastPathStrongRelationLocks->count)
* deadlock detector start looking for cycles
* once ready - FastPathStrongRelationLocks->count is decremented to
allow fast-path

That way performance degradation happens only during deadlock detector
processing and only if some future locks present.

Due tue LW ordering we need to use some tricks to avoid LW-level
deadlocks (using some kind of retry logic, but that is more explained
in the patch).

[0]: https://www.postgresql.org/message-id/flat/CADzfLwURKVNQ%2B%2BDpi7bjoGfj-8pchDQEVex3eWBx0NCYn6TbDQ%40mail.gmail.com#bceb18354aa20c130a94b1deedd76fb7.

[1}] postgresql.org/message-id/flat/CADzfLwU8Qw6LXFHO7Tbjc-O7o%2BtM26jdnOJBWqYLu61rf7bO%2Bg%40mail.gmail.com#1e96f8882363afb2fc53c2f08346f527

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 19 Apr 2026 14:00:01 +0300

I've noticed a wrong reference coined in commit 28d534e2a:
A comment in repack_worker.c says:
     /*
      * Override the default bgworker_die() with die() so we can use
      * CHECK_FOR_INTERRUPTS().
      */
     pqsignal(SIGTERM, die);
     BackgroundWorkerUnblockSignals();

but bgworker_die() doesn't exist since d62dca3b2 (2026-02-18).
Maybe this is not needed anymore?

Best regards,
Alexander

---

From: Antonin Houska <[email-masked]>
Date: Mon, 20 Apr 2026 09:44:45 +0200

> documentation/error message?

It was discussed earlier [1] and the concerns about possibly excessive
resource consumptions were addressed by [2]. So I think it the fix was just
forgotten. Attached here.

[1] https://www.postgresql.org/message-id/[email-masked]
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e76d8c749c3152657711ed733f0aea61c0e36a91

---

From: Antonin Houska <[email-masked]>
Date: Mon, 20 Apr 2026 09:47:36 +0200

> forgotten. Attached here.

Sorry, I attached wrong patch. This is what I meant.

---

From: Chao Li <[email-masked]>
Date: Mon, 20 Apr 2026 15:54:19 +0800

> On Apr 20, 2026, at 15:47, Antonin Houska <[email-masked]> wrote:
> 
> <0001-Do-not-check-the-REPLICATION-attribute-when-running-.patch>

In my test, I found the exact same issue and I was about to post a fix that is the same as v1. So, v1 looks good to me.

BTW, Antonin, can you please take a look at [1] that is the other issue I found with repack.

[1] https://www.postgresql.org/message-id/10DD5E13-B45D-44F1-BE08-C63E00ABCAC0%40gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Apr 2026 13:45:35 +0200

> Sorry, I attached wrong patch. This is what I meant.

Yeah, I had also written the same patch a couple of days ago.

BTW I ran into a small problem after adding some tests in cluster.sql
that would exercise this -- that test would die more or less randomly
but frequently in CI (which it never did in my laptop) because of the
size of the snapshot,

 ALTER TABLE ptnowner1 REPLICA IDENTITY USING INDEX ptnowner1_i_key;
 REPACK (CONCURRENTLY) ptnowner1;
+ERROR:  initial slot snapshot too large
+CONTEXT:  REPACK decoding worker
 RESET SESSION AUTHORIZATION;

I think the solution for this is to move cluster to a separate parallel
test.  The one where it is now is a bit too crowded.  Maybe the one for
compression is okay?  I'll test and push if I see it passing CI.

Another obvious thing after adding tests is that the LOGIN privilege is
required, which is also quite bogus IMO.  0002 here should solve that.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If you want to have good ideas, you must have many ideas.  Most of them
will be wrong, and what you have to learn is which ones to throw away."
                                                         (Linus Pauling)

---

From: Antonin Houska <[email-masked]>
Date: Mon, 20 Apr 2026 15:46:13 +0200

> compression is okay?  I'll test and push if I see it passing CI.

That shouldn't break anything, but I have no idea why this problem was not
triggered (as far as I remember) by the stress tests we ran during
development.

I thought that it might be due to less frequent calls of
SnapBuildPurgeOlderTxn(), which might in turn be due to less frequent
checkpoints (because xl_running_xacts is typically written during checkpoint),
and that checkpoints may be deliberately less frequent to make regression
tests run faster. However I don't immediately see related configuration in the
regression test setup.

> required, which is also quite bogus IMO.  0002 here should solve that.

ok

> >From b3d4158356f4914d2b0cba86eef6994c0ee50ab9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
> Date: Mon, 20 Apr 2026 11:38:48 +0200
> Subject: [PATCH 1/2] REPACK: do not require the user to have REPLICATION

> Because there are now successful concurrent repack runs in the
> regression tests, we're forced to run test_plan_advice under
> wal_level=replica.

Is this an attempt to disable REPACK (CONCURRENTLY)? That would require
wal_level=minimal (due to commit 67c20979ce). In which way does REPACK seem to
break test_plan_advice?

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Apr 2026 15:54:52 +0200

> triggered (as far as I remember) by the stress tests we ran during

I took a guess that it's because some tests use minimally configured
servers -- that is, max_connections=20 or so -- and then run 20
processes.  If we then try to construct a snapshot that's limited to
having only that many XIDs, we might not have enough room in the xip
array.  I didn't try to trace it super carefully though.

> wal_level=minimal (due to commit 67c20979ce). In which way does REPACK seem to

No, quite the contrary.  That test normally runs with wal_level=minimal,
which causes REPACK to complain that it cannot start logical
decoding.

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Apr 2026 10:42:06 -0400

> which causes REPACK to complain that it cannot start logical

So what you're saying is that the core regression tests will now fail
with wal_level=minimal?  I don't see how that can possibly be
considered acceptable from a global standpoint; we might as well
remove wal_level=minimal, because it will never again get tested.

I think you need to move these tests out into some other test suite
(or make a new one).

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Apr 2026 17:11:56 +0200

> remove wal_level=minimal, because it will never again get tested.

Hmm, you're right, this was brain fade on my part.

It surprising though that no buildfarm animal so far has complained.
But I remember there's at least one that runs with that, per
be142fa008ad.

> I think you need to move these tests out into some other test suite

I'll see what I can find.

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Apr 2026 12:01:42 -0400

> But I remember there's at least one that runs with that, per

Yeah, thorntail does, but it only runs once a day.

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Apr 2026 18:30:25 +0200

> I'll see what I can find.

I think the simplest would be to add them to
src/test/modules/injection_points.  We already have some repack tests
there (because they needed injection points), and it has an SQL suite,
so we would not be adding any extra overhead.  However, the new tests
are not related to injection points, so they would be out of place.

Another possibility could be src/test/subscription, but it doesn't have
sql tests; doesn't seem good to have them just for this.

There's also contrib/test_decoding.  It's somewhat vaguely adjacent, and
the tests aren't _really_ about the decoding part, but of all these
options, it seems the least bad one.

I don't like the idea of adding another suite.  Too much scaffolding for
so little, I think.

I don't have any other ideas ATM.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"We're here to devour each other alive"            (Hobbes)

---

From: Antonin Houska <[email-masked]>
Date: Mon, 20 Apr 2026 19:44:52 +0200

Mihail Nikalayeu <[email-masked]> wrote:

> I've briefly looked at your patch. As I understand it, it cancels the other
> process only when REPACK actually tries to upgrade the lock. ...
>
> upgrade.

I thought the point is that the deadlock should be resolved in a controlled
way, i.e. w/o relying on deadlock timeout. Once both processes sleep, the
decision which one should be kicked off is effectively random. In other words,
the actual deadlock IMO starts exactly at the moment both processes end up
sleeping. But I may be wrong.

---

From: Chao Li <[email-masked]>
Date: Tue, 21 Apr 2026 15:24:25 +0800

> On Apr 10, 2026, at 18:53, Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> 
> Hi,
> 
> <v1-0001-Allow-old-WALs-to-be-removed-during-REPACK-CONCUR.patch>

I found the same problem with LogicalConfirmReceivedLocation and posted a fix in a separate thread [1]. So I would withdraw my patch.

Looking at this patch, the change is exactly the same as what I did in [1], but I think the code comment should be updated as well. For the comment change, please see my patch in [1].

[1] https://www.postgresql.org/message-id/D8D9F770-DAA2-482C-A7E0-F87E5104C13E%40gmail.com

Best regards,

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 22 Apr 2026 11:30:26 +0200

> options, it seems the least bad one.

Here's a patch that does this.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 23 Apr 2026 13:01:57 +0200

> Here's a patch that does this.

Pushed, thanks.  Thorntail should turn green in its next run.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 23 Apr 2026 13:43:18 +0200

Hello!

On Mon, Apr 20, 2026 at 7:44 PM Antonin Houska <[email-masked]> wrote:

> When another process tries to get the lock after that, it checks this field,
> and if it's set, it checks for deadlocks even if deadlock timeout hasn't
> expired yet. If the process is already in the lock's queue and sleeping, the
> lock upgrading process wakes it up so it checks the flag immediately.

In any way there is no sense in waking up other backends just to force
them to cancel themselves.
Better to go in the [0] way - and just cancel another backend if we
are repack and found cycle in the deadlock detector.

We currently have all required infrastructure, even as the comment
described the possibility [1]:

>         * Get this process out of wait state. (Note: we could do this more
>         * efficiently by relying on lockAwaited, but use this coding to
>         * preserve the flexibility to kill some other transaction than the
>         * one detecting the deadlock.)

At the same time version [2] (with FutureWaitLock, explained in more
detail in [3]) is correct and cancels other backends without pointless
waiting, but it feels too complicated due to the complexity of
supporting the FastLock path.

Also, here's one simple idea inspired by your version.
What about adding a new field "do not try to upgrade that lock" to the
LOCK structure? If some backends try to (it has some lockmode and
tries to upgrade it) and it does not has flag 'deadlock_protected'
from [0] - just fail fast with "future deadlock detected".

That way [0] ensures correctness and "do not try to upgrade that lock"
just optimization to fail quickly for the most common scenarios.
Some 2+ backend loops might still wait a long time to be cancelled
(which is solved by [2], but complicated) - but I think this is a
super rare case we can ignore (because repack is still protected from
deadlock).

Mikhail.

[0]: https://www.postgresql.org/message-id/flat/CADzfLwURKVNQ%2B%2BDpi7bjoGfj-8pchDQEVex3eWBx0NCYn6TbDQ%40mail.gmail.com#bceb18354aa20c130a94b1deedd76fb7.
[1]: https://github.com/postgres/postgres/blob/master/src/backend/storage/lmgr/proc.c#L1870-L1873
[2]: https://www.postgresql.org/message-id/flat/CADzfLwUSnGnkfLwCWHQ%3DVVuAY1YTo%2B0Lr7pb%2BOPWUZbcYKSRUw%40mail.gmail.com#e7ba203b7402de332dfb58ce5329a73a
[3]: https://www.postgresql.org/message-id/flat/CADzfLwVf-3mjMwSTOcj9djNzGd-UjBOYbFjxgXRhtKuH_4rajA%40mail.gmail.com#b69f9c1d1d3d1cbf7f6d66be790894c4

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sun, 26 Apr 2026 15:34:00 +0200

Hello!

I think I have a good enough approach now (at least balancing
complexity and outcome).

Patch (and commit message) is quite explanatory, but in a few words:

- add 'upgradeIntent' to PROCLOCK (set by REPACK)
- check that in the deadlock detector. If the backend finds the cycle
and is part of it, but because it's upgrading an already announced
lock, it cancels another backend instead of itself.
- use that in the fast path of simple deadlock detection to avoid
pointless waiting (for the easy case involving two backends)

It doesn't cover all scenarios (explained in patch details) but for
majority of realistic scenarios - yes.
It may be extended to cover all of them, but I'm not sure it's worth
the additional complexity.

Best regards,
Mikhail.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 27 Apr 2026 09:55:30 +0530

> But I think this may need more infrastructure / deeper analysis than what we

We still have not decided on this point. The code corresponding to
this part [1] has an open bug as well [2]. Based on the discussion
here, I don't see we have a consensus with the current approach and
even if we want to go with the current approach it seems that we need
more work which needs further analysis. Alvaro, others, what is your
take on this?

[1]: commit 0d3dba38c777384a9dd7dffe924355c9683a6b71: Allow logical
replication snapshots to be database-specific
[2]: https://www.postgresql.org/message-id/CAHg%2BQDcQak4jx_6X2_Ws98rzG%3DxBARLjqm_%3D56wTRUtNsY4DZQ%40mail.gmail.com
-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 27 Apr 2026 04:46:37 +0000

Dear Hackers,

> more work which needs further analysis. Alvaro, others, what is your

FYI, the point I raised [1] [2] seemed not to be fixed yet. Not sure it's the
last point we missed.

[1]: https://www.postgresql.org/message-id/225003.1775571560%40localhost
[2]: https://www.postgresql.org/message-id/CAA4eK1KWDbBk4FgbbWdivQLrPPzR4zgvfnHK4WjWC78rbuRVbg%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 30 Apr 2026 13:24:41 +0200

Hello!

On Mon, Apr 27, 2026 at 6:25 AM Amit Kapila <[email-masked]> 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.

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]).

[0]: https://www.postgresql.org/message-id/flat/CADzfLwUitd5J17O9FUxNGrZBurOpL6n%2BtnS6dgArXi-i9DNxhg%40mail.gmail.com#c5945a539400676cdfd72eec6c101710
[1]: https://www.postgresql.org/message-id/flat/CAHg%2BQDcQak4jx_6X2_Ws98rzG%3DxBARLjqm_%3D56wTRUtNsY4DZQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/flat/CADzfLwWC%2BKxYWb-2QotWaz-q1LK8koLNVUR1Q8obAtt%2BR_sORA%40mail.gmail.com#c8435a284f8893bbc1a891c56be5e158

---

From: 'Alvaro Herrera' <[email-masked]>
Date: Fri, 1 May 2026 09:30:27 +0200

Hello,

> Not sure it's OK to use Assert(). elog(ERROR) might be better if we want to really

How about the attached?

---

From: Antonin Houska <[email-masked]>
Date: Mon, 04 May 2026 15:24:49 +0200

> 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.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 05 May 2026 14:47:46 +0200

> 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.

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 May 2026 17:02:39 +0200

On 2026-May-05, Antonin Houska wrote:

> However, I failed to notice that COMMIT record of

That's pretty bad news.

> "database-specific snapshots" feature now.

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

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 May 2026 17:04:42 +0200

> 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/[email-masked]

---

From: Antonin Houska <[email-masked]>
Date: Wed, 06 May 2026 10:25:44 +0200

Alvaro Herrera <[email-masked]> wrote:

> > 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

---

From: Amit Kapila <[email-masked]>
Date: Fri, 8 May 2026 17:55:12 +0530

> One idea occurred to me yet, effectively it's just a cleanup. Part of it was

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.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 8 May 2026 15:58:18 +0200

> > One idea occurred to me yet, effectively it's just a cleanup. Part of it was

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.

> 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"

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 8 May 2026 16:22:04 -0700

> restore such a snapshot making it take the wrong decision.

Right. I think it affects even concurrent REPACK (CONCURRENTLY)
running on other databases. They could end up restoring the snapshot
serialized by another REPACK command running on another database and
becoming the consistent state without waiting for transactions
concurrently running on the same database, which is clearly wrong.

Regards,

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

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 8 May 2026 16:22:15 -0700

> addressing for 19.  If we hit some roadblock, we can still revert only

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,

---

From: Amit Kapila <[email-masked]>
Date: Sun, 10 May 2026 16:54:29 +0530

> addressing for 19.  If we hit some roadblock, we can still revert only

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.

---

From: Amit Kapila <[email-masked]>
Date: Sun, 10 May 2026 17:01:04 +0530

> transaction, it may end up at higher LSN in WAL. So the cleanup I relied on

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.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 11 May 2026 17:17:52 +0200

> 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.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 11 May 2026 21:30:01 +0200

> 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.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 12 May 2026 02:25:47 +0000

Dear Antonin,

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

> 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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 12 May 2026 13:27:29 +0530

On Tue, May 12, 2026 at 1:00 AM Antonin Houska <[email-masked]> wrote:
> 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.

---

From: Antonin Houska <[email-masked]>
Date: Tue, 12 May 2026 13:08:20 +0200

Amit Kapila <[email-masked]> wrote:

> 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

ok

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

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 May 2026 18:04:17 +0530

> 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.

> > db-specific snapshots on standby.

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.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 13 May 2026 18:58:14 +0200

Hello Amit,

> 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?

---

From: Amit Kapila <[email-masked]>
Date: Thu, 14 May 2026 12:32:25 +0530

On Wed, May 13, 2026 at 10:28 PM Alvaro Herrera <[email-masked]> wrote:
> 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

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.

> 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.

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 19 May 2026 20:52:13 +0200

> rest of the logical decoding machinery, which is built for

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.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 23 May 2026 08:29:35 -0700

> 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.

---

From: Alvaro Herrera <[email-masked]>
Date: Sun, 24 May 2026 13:29:28 +0200

> > The revert should be a simple patch.

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

Thanks!

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 25 May 2026 06:26:52 +0000

> -----Original Message-----
> Sent: Friday, April 10, 2026 9:22 PM
> To: Hou, Zhijie/侯 志杰 <[email-masked]>
> Cc: Alvaro Herrera <[email-masked]>; Amit Kapila
> <[email-masked]>; Kuroda, Hayato/黒田 隼人
> <[email-masked]>; Srinath Reddy Sadipiralla
> <[email-masked]>; Mihail Nikalayeu <[email-masked]>;
> Matthias van de Meent <[email-masked]>; Pg Hackers
> <[email-masked]>; Robert Treat <[email-masked]>
> Subject: Re: Adding REPACK [concurrently]
> 
> > When testing REPACK concurrently, I noticed that all WALs are retained
> > from the moment REPACK begins copying data to the new table until the
> > command finishes replaying concurrent changes on the new table and
> > stops the repack decoding worker.
> >
> > I understand the reason: the REPACK command itself starts a
> > long-running transaction, and logical decoding does not advance
> > restart_lsn beyond the oldest running transaction's start position. As
> > a result, slot.restart_lsn remains unchanged, preventing the checkpointer
> from recycling WALs.
> 
> XID for the actual heap rewriting, it gets one at the moment it locks the table
> using AccessExclusiveLock (which is at the very beginning).
> 
> > Given that we do not restart a REPACK, I think the repack decoding
> > worker should be able to advance restart_lsn each time after writing
> > changes (similar to how a physical slot behaves). To illustrate this,
> > I've written a patch (attached) that implements this approach, and it works
> fine for me.
> 
> LGTM, thanks!
> 

Thanks for reviewing!

After listening to the REPACK talk at pgconf.dev this year, I understand that
WAL accumulation during REPACK CONCURRENTLY is not intended behavior. I think we
can consider fixing this in the current release. Attached is the rebased
patch, with comments adjusted based on Chao Li's comments.

Best Regards,
Hou zj

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 26 May 2026 17:31:33 +0200

> patch, with comments adjusted based on Chao Li's comments.

You're right, this is a thinko.  I'll look at your patch hoping to get
it pushed shortly.  I wonder if we should add a TAP test to verify that
WAL files are actually removed?  Sounds a bit excessive TBH, but maybe
it isn't really.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 27 May 2026 08:08:41 +0000

On Tuesday, May 26, 2026 11:32 PM Alvaro Herrera <[email-masked]> wrote:
> > After listening to the REPACK talk at pgconf.dev this year, I
> > understand that WAL accumulation during REPACK CONCURRENTLY is not
> > intended behavior. I think we can consider fixing this in the current
> > release. Attached is the rebased patch, with comments adjusted based on
> Chao Li's comments.
> 
> You're right, this is a thinko.  I'll look at your patch hoping to get it pushed
> shortly.  I wonder if we should add a TAP test to verify that WAL files are
> actually removed?  Sounds a bit excessive TBH, but maybe it isn't really.

I tried a bit, and the test complexity and speed (< 1s) appear to be within
acceptable limits. I'm attaching 0002 as a reference test.

0001 remains unchanged.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 17:31:59 -0700

> 0001 remains unchanged.

Few minor comments:
=================
*
+ * To allow old WAL files to be recycled, we manually advance the
+ * slot each time a WAL segment boundary is crossed.

This is  safe only because REPACK creates a temporary slot that is
dropped if REPACK fails — there's no scenario where this slot needs to
restart decoding from
an earlier position while still alive. I feel that is worth a mention.

*
@@ -1910,8 +1910,14 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
  SpinLockRelease(&MyReplicationSlot->mutex);

  ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
  }
+
+ /*
+ * Now the new restart_lsn is safely on disk, recompute the global WAL
+ * retention requirement.
+ */
+ if (updated_restart)
+ ReplicationSlotsComputeRequiredLSN();

This change is not related to this patch, rather we need it even
without this patch, is it worth mentioning in the commit message?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 20:34:08 -0700

> <[email-masked]> wrote:

Commit message says: "This change does not advance catalog_xmin.
REPACK already holds a snapshot that prevents catalog dead tuple
removal, so catalog_xmin handling can be addressed independently.".
Isn't it equally important to advance this, otherwise, for long
running REPACKs dead tuples will be accumulated needlessly? If so, do
we have any ideas to avoid this?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 28 May 2026 05:18:34 +0000

On Thursday, May 28, 2026 11:34 AM Amit Kapila <[email-masked]> wrote:
> REPACK already holds a snapshot that prevents catalog dead tuple removal,
> so catalog_xmin handling can be addressed independently.".
> Isn't it equally important to advance this, otherwise, for long running REPACKs
> dead tuples will be accumulated needlessly? If so, do we have any ideas to
> avoid this?

My understanding is that dead tuple accumulation is common to all long-running
commands (including CLUSTER, VACUUM FULL, and REPACK without CONCURRENTLY). As
long as a command holds a snapshot for a long time while scanning and copying
data, the backend xmin will cause similar accumulation. So, this doesn't seem
like a new issue to me, and given that catalog_xmin only affect tuples in system
catalog which is less harmful, I thought it could be handled independently.
There was a proposal to improve this case in [1]. Sorry if I've missed something.

Attaching the v4 patch which improved the comments and commit message as
suggested.

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

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 May 2026 08:39:18 -0700

On Wed, May 27, 2026 at 10:18 PM Zhijie Hou (Fujitsu)
<[email-masked]> wrote:
> There was a proposal to improve this case in [1].
>

Fair enough. It makes sense to deal with catalog_xmin separately.

> Attaching the v4 patch which improved the comments and commit message as

I haven't tested it but otherwise the code changes looks good to me.

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 30 May 2026 00:25:12 +0200

> acceptable limits. I'm attaching 0002 as a reference test.

Pushed 0001, in two parts.  I rewrote the comment in
decode_concurrent_changes() though, hope it ended up okay.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
                                                   (G. K. Chesterton)

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 1 Jun 2026 02:25:59 +0000

On Saturday, May 30, 2026 6:25 AM Alvaro Herrera <[email-masked]> wrote:
> > I tried a bit, and the test complexity and speed (< 1s) appear to be
> > within acceptable limits. I'm attaching 0002 as a reference test.
> >
> > 0001 remains unchanged.
> 

Thank you for pushing the patches ! The comments look good to me.

In the original 0002 test patch, I realized that I missed to drop existing slots
from previous tests (I did locally but missed to merge into the patch), which
also prevented WAL removal, sorry for that. Attached is the fixed test patch.

Best Regards,
Hou zj

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 3 Jun 2026 19:27:00 +0200

Hello, everyone!

I think it is a good time to remind we still have a potential deadlock
issue leading to loosing vast amount of REPACK work.

Best soltuion I was able to craft so far is [1].

Best regards,
Mikhail.

P.S.
I'll continue working on the repack stress test suite in the near future.

[1]: https://www.postgresql.org/message-id/flat/CADzfLwXbUWuS6H4uJEFVL1jS1kzsVnuJ%2BzX1%2BtAEhQxBnEiGKw%40mail.gmail.com#99b8fbf05044d1fb82af01de69510c78