On Wed, Sep 20, 2023 at 3:23 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Wed, Aug 16, 2023 at 7:57 PM Tomas Vondra > <tomas.von...@enterprisedb.com> wrote: > > > > I was reading through 0001, I noticed this comment in > ReorderBufferSequenceIsTransactional() function > > + * To decide if a sequence change should be handled as transactional or > applied > + * immediately, we track (sequence) relfilenodes created by each transaction. > + * We don't know if the current sub-transaction was already assigned to the > + * top-level transaction, so we need to check all transactions. > > It says "We don't know if the current sub-transaction was already > assigned to the top-level transaction, so we need to check all > transactions". But IIRC as part of the steaming of in-progress > transactions we have ensured that whenever we are logging the first > change by any subtransaction we include the top transaction ID in it. > > Refer this code > > LogicalDecodingProcessRecord(LogicalDecodingContext *ctx, > XLogReaderState *record) > { > ... > /* > * If the top-level xid is valid, we need to assign the subxact to the > * top-level xact. We need to do this for all records, hence we do it > * before the switch. > */ > if (TransactionIdIsValid(txid)) > { > ReorderBufferAssignChild(ctx->reorder, > txid, > XLogRecGetXid(record), > buf.origptr); > } > }
Some more comments 1. ReorderBufferSequenceIsTransactional and ReorderBufferSequenceGetXid are duplicated except the first one is just confirming whether relfilelocator was created in the transaction or not and the other is returning the XID as well so I think these two could be easily merged so that we can avoid duplicate codes. 2. /* + * ReorderBufferTransferSequencesToParent + * Copy the relfilenode entries to the parent after assignment. + */ +static void +ReorderBufferTransferSequencesToParent(ReorderBuffer *rb, + ReorderBufferTXN *txn, + ReorderBufferTXN *subtxn) If we agree with my comment in the previous email (i.e. the first WAL by a subxid will always include topxid) then we do not need this function at all and always add relfilelocator directly to the top transaction and we never need to transfer. That is all I have for now while first pass of 0001, later I will do a more detailed review and will look into other patches also. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com