Feedback-ID: ia2694551:Fastmail
Date: Sun, 5 Apr 2026 01:53:09 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Antonin Houska <ah@cybertec.at>
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]
Message-ID: <202604042326.nwa4o5sezj77@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="qr3jlalmmcpkiodg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <95398.1775316559@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/202604042326.nwa4o5sezj77%40alvherre.pgsql>
Precedence: bulk


--qr3jlalmmcpkiodg
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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.

-- 
Á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)

--qr3jlalmmcpkiodg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v53-0001-Introduce-an-option-to-make-logical-replication-.patch"