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)
