From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
    Amit Kapila <amit.kapila16@gmail.com>,
    Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
    Matthias van de Meent <boekewurm+postgres@gmail.com>,
    Pg Hackers <pgsql-hackers@lists.postgresql.org>,
    Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
In-reply-to: <202604042326.nwa4o5sezj77@alvherre.pgsql>
References: <202604042326.nwa4o5sezj77@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
   message dated "Sun, 05 Apr 2026 01:53:09 +0200."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Sun, 05 Apr 2026 09:54:49 +0200
Message-ID: <19989.1775375689@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/19989.1775375689%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=nocfbot.v53-0001.2-Introduce-an-option-to-make-logical-replication-.patch