Re: Adding REPACK [concurrently]

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

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