Re: pgsql: Document XLOG_INCLUDE_XID a little better

On 2021-Oct-01, Amit Kapila wrote:

> AFAICS, there are two possibilities w.r.t global variables: (a) use
> curinsert_flags which we are doing now, (b) another is to introduce a
> new global variable, set it after we make the association, and then
> reset it after we mark SubTransaction assigned and on error. I have
> also thought of passing it via XLogCtlInsert but as that is shared by
> different processes, it can be set by one process and be read by
> another process which we don't want here.

So, in my mind, curinsert_flags is a way for the high-level user of
XLogInsert to pass info about the record being inserted to the low-level
xloginsert.c infrastructure. In contrast, XLOG_INCLUDE_XID is being
used solely within xloginsert.c, by one piece of low-level
infrastructure to communicate to another piece of low-level
infrastructure that some cleanup is needed. Nothing outside of
xloginsert.c needs to know anything about XLOG_INCLUDE_XID, in contrast
with the other bits that can be set by XLogSetRecordFlags. You could
move the #define to xloginsert.c and everything would compile fine.

Another tell-tale sign that things are not fitting right is that
XLOG_INCLUDE_XID is not set via XLogSetRecordFlags, contrary the comment
above those defines.

(Aside: I wonder why do we have XLogSetRecordFlags at all -- I think we
could just pass the other two flags via XLogBeginInsert).

Regarding XLOG_INCLUDE_XID, I don't think replacing it with a bit in
shared memory is a good idea, since it only applies to the insertion
being carried out by the current process, right?

I think a straight standalone variable (probably a static boolean in
xloginsert.c) might be less confusing.

... so, reading the xact.c code again, TransactionState->assigned really
means "whether the subXID-to-topXID association has been wal-logged",
which is a completely different meaning from what the term 'assigned'
means in all other comments in xact.c ... and I think the subroutine
name MarkSubTransactionAssigned() is not a great choice either.

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)