Re: pgsql: Document XLOG_INCLUDE_XID a little better

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: pgsql: Document XLOG_INCLUDE_XID a little better
Date: 2021-10-20 13:39:15
Message-ID: 202110201339.vrrwchc7jkwo@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

API-wise, this seems a good improvement and it brings a lot of clarity
to what is really going on. Thanks for working on it.

Some minor comments:

Please do not revert the comment change in xlogrecord.h. It is not
wrong. The exceptions mentioned are values 252-255, so "a few" is
better than "a couple".

In IsTopTransactionIdLogPending(), I suggest to reorder the tests so
that the faster ones are first -- or at least, the last one should be at
the top, so in some cases we can return without additional function
calls. I don't think it'd be extremely noticeable, but as Tom likes to
say, a cycle shaved is a cycle earned.

XLogRecordAssemble's comment should explain its new output argument.
Maybe "*topxid_included is set if the topmost transaction ID is logged
with the current subtransaction".

I think these new routines IsTopTransactionIdLogPending and
MarkTopTransactionIdLogged should not be at the end of the file; a
location closer to where MarkCurrentTransactionIdLoggedIfAny() seems
more appropriate. Keep related things closer. (In this case these are
operating on TransactionStateData, and it looks right that they would
appear somewhat about AssignTransactionId and
GetStableLatestTransactionId.)

Does MarkTopTransactionIdLogged() have to be inside XLogInsertRecord's
critical section?

The names IsTopTransactionIdLogPending() and
MarkTopTransactionIdLogged() irk me somewhat. It's not at all obvious
from these names that these routines are mostly about actions taken for
a subtransaction. I propose IsSubxactTopXidLogPending() and
MarkSubxactTopXidLogged(). I don't feel the need to expand "Xid" to
"TransactionId" in these function names.

In TransactionStateData, I propose this wording for the comment:

bool topXidLogged; /* for a subxact: is top-level XID logged? */

Thanks!

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Dilip Kumar 2021-10-20 15:19:36 Re: pgsql: Document XLOG_INCLUDE_XID a little better
Previous Message Tom Lane 2021-10-20 13:33:44 Re: pgsql: Refactor the sslfiles Makefile target for ease of use

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Nancarrow 2021-10-20 13:40:53 Re: Data is copied twice when specifying both child and parent table in publication
Previous Message Robert Haas 2021-10-20 13:08:57 Re: ThisTimeLineID can be used uninitialized