Re: pgsql: Document XLOG_INCLUDE_XID a little better

On Fri, Oct 1, 2021 at 6:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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).
>

Agreed, I think we can do that if we want but we still need to set
curinsert_flags or some other similar variable in xloginsert.c so that
we can later use and reset it.

> 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?
>

Right. Ideally, we can set this in a local variable via
XLogRecordAssemble() and then use it in XLogInsertRecord() as is done
in 0001-Refactor-code-for-logging-the-top-transaction-id-in-Approach2.
Basically, we just need to ensure that we mark the
CurrentTransactionState for this flag once we are sure that the
function XLogInsertRecord() will perform the insertion and won't
return InvalidXLogRecPtr. OTOH, I see the point in using a global
static variable to achieve this purpose as that allows to do the
required work only in xloginsert.c.

> 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 ...
>

I think you have interpreted it correctly and we make this association
logged with the first WAL of each subtransaction if the WAL level is
logical.

-- 
With Regards,
Amit Kapila.