Re: logical decoding and replication of sequences, take 2

Hi!

On 10/24/23 13:31, Zhijie Hou (Fujitsu) wrote:
> On Thursday, October 12, 2023 11:06 PM Tomas Vondra  wrote:
>>
> 
> Hi,
> 
> I have been reviewing the patch set, and here are some initial comments.
> 
> 1.
> 
> I think we need to mark the RBTXN_HAS_STREAMABLE_CHANGE flag for transactional
> sequence change in ReorderBufferQueueChange().
> 

True. It's unlikely for a transaction to only have sequence increments
and be large enough to get streamed, and other changes would make it to
have this flag. But it's certainly more correct to set the flag even for
sequence changes.

The updated patch modifies ReorderBufferQueueChange to do this.

> 2.
> 
> ReorderBufferSequenceIsTransactional
> 
> It seems we call the above function once in sequence_decode() and call it again
> in ReorderBufferQueueSequence(), would it better to avoid the second call as
> the hashtable search looks not cheap.
> 

In principle yes, but I don't think it's worth it - I doubt the overhead
is going to be measurable.

Based on earlier reviews I tried to reduce the code duplication (there
used to be two separate functions doing the lookup), and I did consider
doing just one call in sequence_decode() and passing the XID to
ReorderBufferQueueSequence() - determining the XID is the only purpose
of the call there. But it didn't seem nice/worth it.

> 3.
> 
> The patch cleans up the sequence hash table when COMMIT or ABORT a transaction
> (via ReorderBufferAbort() and ReorderBufferReturnTXN()), while it doesn't seem
> destory the hash table when PREPARE the transaction. It's not a big porblem but
> would it be better to release the memory earlier by destory the table for
> prepare ?
> 

I think you're right. I added the sequence cleanup to a couple places,
right before cleanup of the transaction. I wonder if we should simply
call ReorderBufferSequenceCleanup() from ReorderBufferCleanupTXN().

> 4.
> 
> +pg_decode_stream_sequence(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
> ...
> +	/* output BEGIN if we haven't yet, but only for the transactional case */
> +	if (transactional)
> +	{
> +		if (data->skip_empty_xacts && !txndata->xact_wrote_changes)
> +		{
> +			pg_output_begin(ctx, data, txn, false);
> +		}
> +		txndata->xact_wrote_changes = true;
> +	}
> 
> I think we should call pg_output_stream_start() instead of pg_output_begin()
> for streaming sequence changes.
> 

Good catch! Fixed.

> 5.
> +	/*
> +	 * Schema should be sent using the original relation because it
> +	 * also sends the ancestor's relation.
> +	 */
> +	maybe_send_schema(ctx, txn, relation, relentry);
> 
> The comment seems a bit misleading here, I think it was used for the partition
> logic in pgoutput_change().

True. I've removed the comment.


Attached is an updated patch, with all those tweaks/fixes.

regards

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