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
