On Thu, Apr 9, 2020 at 2:40 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > I have rebased the patch on the latest head. I haven't yet changed > anything for xid assignment thing because it is not yet concluded. > Some review comments from 0001-Immediately-WAL-log-*.patch,
+bool +IsSubTransactionAssignmentPending(void) +{ + if (!XLogLogicalInfoActive()) + return false; + + /* we need to be in a transaction state */ + if (!IsTransactionState()) + return false; + + /* it has to be a subtransaction */ + if (!IsSubTransaction()) + return false; + + /* the subtransaction has to have a XID assigned */ + if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny())) + return false; + + /* and it needs to have 'assigned' */ + return !CurrentTransactionState->assigned; + +} IMHO, it's important to reduce the complexity of this function since it's been called for every WAL insertion. During the lifespan of a transaction, any of these if conditions will only be evaluated if previous conditions are true. So, we can maintain some state machine to avoid multiple evaluation of a condition inside a transaction. But, if the overhead is not much, it's not worth I guess. +#define SizeOfTransactionId (sizeof(TransactionId) + sizeof(char)) This looks wrong. We should change the name of this Macro or we can add the 1 byte directly in HEADER_SCRATCH_SIZE and some comments. @@ -195,6 +197,10 @@ XLogResetInsertion(void) { int i; + /* reset the subxact assignment flag (if needed) */ + if (curinsert_flags & XLOG_INCLUDE_XID) + MarkSubTransactionAssigned(); The comment looks contradictory. XLogSetRecordFlags(uint8 flags) { Assert(begininsert_called); - curinsert_flags = flags; + curinsert_flags |= flags; } I didn't understand why we need this change in this patch. + txid = XLogRecGetTopXid(record); + + /* + * If the toplevel_xid is valid, we need to assign the subxact to the + * toplevel transaction. We need to do this for all records, hence we + * do it before the switch. + */ s/toplevel_xid/toplevel xid or s/toplevel_xid/txid if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT && - info != XLOG_XACT_ASSIGNMENT) + !TransactionIdIsValid(r->toplevel_xid)) Perhaps, XLogRecGetTopXid() can be used. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com