RE: Per-table resync for logical replication subscriptions

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Cagri Biroglu' <cagri(dot)biroglu(at)adyen(dot)com>
Cc: "smithpb2250(at)gmail(dot)com" <smithpb2250(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Subject: RE: Per-table resync for logical replication subscriptions
Date: 2026-08-17 08:09:09
Message-ID: OS9PR01MB12149489D6B3C370CE8FEC97CF5A72@OS9PR01MB12149.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Cagri,

Thanks for the update. I found this patch could cause an issue if two subscriptions
are modifying the same table. The scenario:

1. A tablesync worker for sub1 acquired opened a table with the RowExclusive in LogicalRepSyncTableStart().
2. User ran ALTER SUBSCRIPTION sub2 REFRESH TABLE command.
3. The backend acquired a AccessExclusive lock for pg_subscription_rel, then
tried to acquire a AccessExclusive Lock for tables. It would wait till the tablesync
worker released.
4. The tablesync worker also tried to acquire AccessShare lock for pg_subscription_rel,
but it would be blocked by the backend. It's done in
copy_table()->logicalrep_rel_open()->GetSubscriptionRelState().
5. The deadlock detector detected the wait-for graph is now circle,
thus it terminates either of them.

One idea for the fix is to acquire AccessShare locks for user-defined tables
first, then acquire the AccessExclusive lock after the CheckRefreshTableNotShared().
This avoids to acquire strong locks only if it's needed, and my reproducer can
reject by the function. Thought?

Also, below are my cosmetic comments.

01.
```
+ LOCKMODE lockmode = AccessExclusiveLock;
```

It might be matter of taste, but I feel the variable lock_mode is not needed:
the pattern is mainly used when the mode can be different based on situations.

02.
```
+static void
+CheckRefreshTableNotShared(Relation pgsubrel, Subscription *sub,
+ List *subrelids, List *relids)
```

The name CheckRefreshTableNotShared() is not suitable, because not sure the
meaning "Shared". How about CheckRefreshTableNotInOtherSubscriptions?

03.
```
+ /*
+ * Tuples reach a partition through its ancestors, so a subscription
+ * tracking any of them keeps this relation populated too. A named
+ * relation's own entry belongs to this subscription and is skipped
+ * below by the srsubid test.
+ */
+ feeders = lappend_oid(get_partition_ancestors(relid), relid);
```

get_partition_ancestors() seems to assume that the given relation has at least
one parent, but the patch does not ensure. Maybe get_rel_relispartition() or
similar functions can be used.

04.
You missed to update meson.build file.

05.
```
+$node_subscriber->append_conf('postgresql.conf',
+ "wal_retrieve_retry_interval = 1ms");
```

Can you clarify the reason why the setting is required? Can we remove if not needed?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message 陈列行 2026-08-17 08:14:20 Redundant outer DISTINCT adds Sort and Unique above EXCEPT
Previous Message Michael Paquier 2026-08-17 08:04:14 Switch opclass option functions to be STRICT (currently non-STRICT)