Horton

RE: Skipping logical replication transactions on subscriber side

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

View thread View on postgr.es
From: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Alexey Lesovsky <lesovsky@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2021-07-22T11:53:26Z
Lists: pgsql-hackers 
On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
> > > I've attached the updated version patch that incorporated all
> > > comments I got so far except for the clearing error details part I
> > > mentioned above. After getting a consensus on those parts, I'll
> > > incorporate the idea into the patches.
> >
> > 3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
> >    assigned, and then will the change be skipped when the xid is assigned in
> >    the future even if it doesn't cause any conflicts ?
> 
> Yes. Currently, setting a correct xid is the user's responsibility. I think it would
> be better to disable it or emit WARNING/ERROR when the user mistakenly set
> the wrong xid if we find out a convenient way to detect that.

Thanks for the explanation. As Amit suggested, it seems we can document the
risk of misusing skip_xid. Besides, I found some minor things in the patch.

1) In 0002 patch

+ */
+static void
+pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
+{
+	if (subscriptionErrHash != NULL)
+		return;
+

+static void
+pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
+{

the second paramater "len" seems not used in the function
pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().


2) in 0003 patch

  * Helper function for apply_handle_commit and apply_handle_stream_commit.
  */
 static void
-apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
+apply_handle_commit_internal(LogicalRepCommitData *commit_data)
 {

This looks like a separate change which remove unused paramater in existing
code, maybe we can get this committed first ?

Best regards,
Houzj

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