On Fri, Oct 1, 2021 at 6:23 PM Alvaro Herrera <alvhe...@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.
