MIME-Version: 1.0
References: 
 <CAA4eK1Jg21ODQ7fS2fvN5W_S5kDRhAP5inj3XMRQaa=s-GbYhw@mail.gmail.com>
 <202604071230.b5axxf3qna3m@alvherre.pgsql>
 <cdgw4sbbfcgk6du3iv54r2dgiy4tfywoklbotlmj4irxavdcr3@glxfw5jj277q>
 <227677.1775576304@localhost>
 <pveffyxhnuurhb44uzqlwo3rkyzorkfh2rot7uwzlf2axhfvbp@7nrs2omysxkc>
 <CAA4eK1JhuT5fyTosWDZ+Pgs+j7xEjObTyRMn80uNKgi_ivqHbw@mail.gmail.com>
 <CADzfLwWnbKcb3v8sStdgNE=WNc3uUqx5SiS4zftX2UaEfNzG5w@mail.gmail.com>
 <85813.1777901089@localhost> <27869.1777985266@localhost>
 <CAA4eK1KC6CGN-N2bUffSign8Sw4q6=8d3L-Xh4t+50GCdQb6zw@mail.gmail.com>
 <70574.1778512672@localhost> <82942.1778527801@localhost>
In-Reply-To: <82942.1778527801@localhost>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 12 May 2026 13:27:29 +0530
Message-ID: 
 <CAA4eK1L1SCa7LjOVzq3zmmqpSV9o7z7VBkPjbE6=2iRLTSBvXw@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Antonin Houska <ah@cybertec.at>
Cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
 Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>,
 Robert Treat <rob@xzilla.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1L1SCa7LjOVzq3zmmqpSV9o7z7VBkPjbE6%3D2iRLTSBvXw%40mail.gmail.com>
Precedence: bulk

On Tue, May 12, 2026 at 1:00=E2=80=AFAM Antonin Houska <ah@cybertec.at> wro=
te:
>
> Antonin Houska <ah@cybertec.at> wrote:
>
> > Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > On Tue, May 5, 2026 at 6:17=E2=80=AFPM Antonin Houska <ah@cybertec.at=
> wrote:
> > > >
> > > > Antonin Houska <ah@cybertec.at> wrote:
> > > >
> > > > I think the problem is that with database-specific snapshot,
> > > > SnapBuildProcessRunningXacts() returns early, w/o adjusting builder=
->xmin
> > > >
> > > >         /*
> > > >          * Database specific transaction info may exist to reach CO=
NSISTENT state
> > > >          * faster, however the code below makes no use of it. Moreo=
ver, such
> > > >          * record might cause problems because the following normal=
 (cluster-wide)
> > > >          * record can have lower value of oldestRunningXid. In that=
 case, let's
> > > >          * wait with the cleanup for the next regular cluster-wide =
record.
> > > >          */
> > > >         if (OidIsValid(running->dbid))
> > > >                 return;
> > > >
> > > > and thus some transactions whose XID is below running->oldestRunnin=
gXid may
> > > > continue to be incorrectly considered running.
> > > >
> > > > I originally thought that this should not happen because such trans=
actions
> > > > will be added to the builder's array of committed transactions by
> > > > SnapBuildCommitTxn() anyway. However, I failed to notice that COMMI=
T record of
> > > > a transaction listed in the xl_running_xacts WAL record is not guar=
anteed to
> > > > follow the xl_running_xacts record in WAL. In other words, even if
> > > > xl_running_xacts is created before a COMMIT record of the contained
> > > > transaction, it may end up at higher LSN in WAL. So the cleanup I r=
elied on
> > > > might not take place.
> > > >
> > >
> > > BTW, is it possible to write a test by using injection_points or via
> > > manual steps (by using debugger, etc) so that we can more clearly
> > > understand this problem and proposed fix?
> >
> > So far I could observe the situation in WAL, but have no idea how it ca=
n
> > happen. For example, transaction 49242 gets committed here
> >
> > rmgr: Transaction len (rec/tot): 46/ 46, tx: 49242, lsn: 0/18BC28C8, pr=
ev
> > 0/18BC2890, desc: COMMIT 2026-05-11 16:38:16.603265 CEST
> >
> > and then it appears in the 'xids' list of RUNNING_XACTS:
> >
> > rmgr: Standby     len (rec/tot):    106/   106, tx:          0, lsn:
> > 0/18BC3140, prev 0/18BC3100, desc: RUNNING_XACTS nextXid 49255
> > latestCompletedXid 49241 oldestRunningXid 49242; 13 xacts: 49248 49249 =
49246
> > 49243 49252 49251 49244 49245 49242 49250 49253 49254 49247; dbid:5
> >
> >
> > I thought the situation is quite common (and therefore nothing of
> > SnapBuildProcessRunningXacts() should be skipped), but when trying to
> > reproduce the problem, I noticed that LogStandbySnapshot() shouldn't al=
low
> > that ordering issue when logical decoding is enabled:
> >
> >       /*
> >        * GetRunningTransactionData() acquired ProcArrayLock, we must re=
lease it.
> >        * For Hot Standby this can be done before inserting the WAL reco=
rd
> >        * because ProcArrayApplyRecoveryInfo() rechecks the commit statu=
s using
> >        * the clog. For logical decoding, though, the lock can't be rele=
ased
> >        * early because the clog might be "in the future" from the POV o=
f the
> >        * historic snapshot. This would allow for situations where we're=
 waiting
> >        * for the end of a transaction listed in the xl_running_xacts re=
cord
> >        * which, according to the WAL, has committed before the xl_runni=
ng_xacts
> >        * record. Fortunately this routine isn't executed frequently, an=
d it's
> >        * only a shared lock.
> >        */
> >       if (!logical_decoding_enabled)
> >               LWLockRelease(ProcArrayLock);
> >
> > So I don't have the answer right now.
>
> I think now that "waiting for the end of a transaction listed in the
> xl_running_xacts record" in the comment is about transaction removal from
> procarray. However, the COMMIT record can still be ahead of xl_running_xa=
cts
> because RecordTransactionCommit() is called before
> ProcArrayEndTransaction().
>

I see your point. Due to this, once the xmin regresses based on
cluster-wide running_xact, some transaction could appear to be running
when it should have appeared as committed. However, still it is not
clear how it could lead to one update in the transaction as
successfully decoded and another one to be skipped. One theory could
be that before the second update, somehow invalidation happens and
when decoding tries to reload the catalog to decode second update, the
relation is not visible because xmin has regressed and the update is
somehow skipped. I can't see how it can happen in code but something
like that is happening. Assuming, the problematic case is something
like what I described, even than the fix of skipping cluster-wide
running xacts and instead LOG db-specific running_xacts to help
updating builder's xmin sounds inelegant and probably inefficient. For
example, I think such a dependency means we can never enable
db-specific snapshots on standby.

--=20
With Regards,
Amit Kapila.