RE: logical decoding and replication of sequences, take 2

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>

To: Tomas Vondra <tomas.vondra@enterprisedb.com>

Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Dilip Kumar <dilipbalaut@gmail.com>

Date: 2023-10-24T11:31:29Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Migrate logical slots to the new node during an upgrade.
- 29d0a77fa660 17.0 cited
Make test_decoding ddl.out shorter
- d6677b93c79b 17.0 landed
- c5c5832600e9 14.9 landed
- b1dc946eee3d 16.0 landed
- 3bb8b9342f8a 15.4 landed
Fix snapshot handling in logicalmsg_decode
- 949ac32e1267 15.3 landed
- 8b9cbd42b61f 14.8 landed
- 4df581fa0f4b 13.11 landed
- 497f863f0598 12.15 landed
- 8de91ebf2ac1 11.20 landed
- 7fe1aa991b62 16.0 landed
doc: Adjust a few more references to "postmaster"
- 17e72ec45d31 16.0 cited
Revert "Logical decoding of sequences"
- 2c7ea57e56ca 15.0 cited

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