Re: Adding REPACK [concurrently]

On 2026-Apr-04, Antonin Houska wrote:

> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > On 2026-Apr-04, Antonin Houska wrote:
> >
> > > 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.
> >
> > 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.

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)