Re: logical decoding and replication of sequences, take 2

On 9/22/23 13:24, Dilip Kumar wrote:
> On Wed, Sep 20, 2023 at 3:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Wed, Aug 16, 2023 at 7:57 PM Tomas Vondra
>> <tomas.vondra@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.
> 

Right. The attached patch modifies the IsTransactional function to also
return the XID, and removes the GetXid one. It feels a bit weird because
now the IsTransactional function is called even in places where we know
the change is transactional. It's true two separate functions duplicated
a bit of code, ofc.

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

Good point! I don't recall why I thought this was necessary. I suspect
it was before I added the GetCurrentTransactionId() calls to ensure the
subxact has a XID. I replaced the ReorderBufferTransferSequencesToParent
call with an assert that the relfilenode hash table is empty, and I've
been unable to trigger any failures.

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

Thanks!

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company