Horton

RE: Skipping logical replication transactions on subscriber side

Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com>

View thread View on postgr.es
From: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
To: 'Masahiko Sawada' <sawada.mshk@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2022-01-17T08:03:17Z
Lists: pgsql-hackers 
On Monday, January 17, 2022 3:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated patch. Please review it.
Hi, thank you for sharing a new patch.
Few comments on the v6.

(1) doc/src/sgml/ref/alter_subscription.sgml

+      resort.  This option has no effect on the transaction that is already

One TAB exists between "resort" and "This".

(2) Minor improvement suggestion of comment in src/backend/replication/logical/worker.c

+ * reset during that.  Also, we don't skip receiving the changes in streaming
+ * cases, since we decide whether or not to skip applying the changes when

I sugguest that you don't use 'streaming cases', because
what "streaming cases" means sounds a bit broader than actual your implementation.
We do skip transaction of streaming cases but not during the spooling phase, right ?

I suggest below.

"We don't skip receiving the changes at the phase to spool streaming transactions"

(3) in the comment of apply_handle_prepare_internal, two full-width characters.

3-1
+	 * won’t be resent in a case where the server crashes between them.

3-2
+	 * COMMIT PREPARED or ROLLBACK PREPARED. But that’s okay because this

You have full-width characters for "won't" and "that's".
Could you please check ?


(4) typo

+ * the subscription if hte user has specified skip_xid. Once we start skipping

"hte" should "the" ?

(5)

I can miss something here but, in one of
the past discussions, there seems a consensus that
if the user specifies XID of a subtransaction,
it would be better to skip only the subtransaction.

This time, is it out of the range of the patch ?
If so, I suggest you include some description about it
either in the commit message or around codes related to it.

(6)

I feel it's a better idea to include a test whether
to skip aborted streaming transaction clears the XID
in the TAP test for this feature, in a sense to cover
various new code paths. Did you have any special reason
to omit the case ?

(7)

I want more explanation for the reason to restart the subscriber
in the TAP test because this is not mandatory operation.
(We can pass the TAP tests without this restart)

From :
# Restart the subscriber node to restart logical replication with no interval

IIUC, below would be better.

To :
# As an optimization to finish tests earlier, restart the subscriber with no interval,
# rather than waiting for new error to laucher a new apply worker.


Best Regards,
	Takamichi Osumi

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue