Re: Adding REPACK [concurrently]

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

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

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

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