public inbox for [email protected]  

help / color / mirror / Atom feed

From: Antonin Houska <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Srinath Reddy Sadipiralla <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Mihail Nikalayeu <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Robert Treat <[email protected]>
Subject: Re: Adding REPACK [concurrently]
Date: Sun, 05 Apr 2026 09:54:49 +0200
Message-ID: <19989.1775375689@localhost> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

Alvaro Herrera <[email protected]> wrote:

> On 2026-Apr-04, Antonin Houska wrote:
> 
> > Alvaro Herrera <[email protected]> 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

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Adding REPACK [concurrently]
  In-Reply-To: <19989.1775375689@localhost>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox