RE: logical decoding and replication of sequences, take 2

On Thursday, October 12, 2023 11:06 PM Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> 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().

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.

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 ?

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.

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

Best Regards,
Hou zj