On Mon, Feb 19, 2024 at 3:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Feb 19, 2024 at 10:26:43AM +0100, Tomas Vondra wrote:
> > On 2/19/24 07:57, Michael Paquier wrote:
> > > On Sun, Feb 18, 2024 at 02:28:06AM +0100, Tomas Vondra wrote:
> >>> 1) Do we really need to modify RecordTransactionCommitPrepared and
> >>> XactLogCommitRecord to return the LSN of the commit record? Can't we
> >>> just use XactLastRecEnd? It's good enough for advancing
> >>> replorigin_session_origin_lsn, why wouldn't it be good enough for this?
> >>
> >> Or XactLastCommitEnd?
> >
> > But that's not set in RecordTransactionCommitPrepared (or twophase.c in
> > general), and the patch seems to need that.
>
> Hmm. Okay.
>
> > > The changes in AtEOXact_PgStat() are not really
> > > attractive for what's a static variable in all the backends.
> >
> > I'm sorry, I'm not sure what "changes not being attractive" means :-(
>
> It just means that I am not much a fan of the signature changes done
> for RecordTransactionCommit() and AtEOXact_PgStat_Database(), adding a
> dependency to a specify LSN. Your suggestion to switching to
> XactLastRecEnd should avoid that.
Attached is an updated patch that uses XactLastCommitEnd and therefore
avoids all of those signature changes.
We can't use XactLastCommitEnd because it gets set to 0 immediately
after RecordTransactionCommit() sets XactLastCommitEnd to its value.
I added a test for two-phase commit, as well, and in so doing I
discovered something that I found curious: when I do "COMMIT PREPARED
't1'", not only does RecordTransactionCommitPrepared() get called, but
eventually RecordTransactionCommit() is called as well before the
command returns (despite the comments for those functions describing
them as two equivalents you need to change at the same time).
So it appears that we don't need to set XactLastCommitEnd in
RecordTransactionCommitPrepared() for this to work, and, indeed,
adding some logging to verify, the value of XactLastRecEnd we'd use to
update XactLastCommitEnd is the same at the end of both of those
functions during COMMIT PREPARED.
I'd like to have someone weigh in on whether relying on
RecordTransactionCommit() being called during COMMIT PREPARED is
correct or not (if perchance there are non-obvious reasons why we
shouldn't).
Regards,
James Coleman
