Re: Per-table resync for logical replication subscriptions

On Mon, Aug 10, 2026 at 1:53 AM Cagri Biroglu <cagri(dot)biroglu(at)adyen(dot)com> wrote:
>
> Hi again,
>
> Thanks, and thanks for picking this up. I've registered it in the open
> CommitFest (PG20-2).

+1

>
> Attached is v3, which is v2 rebased onto current master. Master removed
> Subscription.conninfo, so the one code change is that the mid-sync path
> now resolves the connection string with SubscriptionConninfo(sub). It
> does that at the point of use rather than up front, because the common
> case here is a relation in ready state, which needs no publisher
> connection at all. Nothing else changed, so your review of v2 still
> applies.
>

Thank you for updating the patch! I've reviewed the v3 patch and here
are review comments:

---
+ PG_TRY();
+ {
+ ReplicationSlotNameForTablesync(sub->oid, relid, syncslotname,
+ sizeof(syncslotname));
+ ReplicationSlotDropAtPubNode(wrconn, syncslotname, true);
+ }
+ PG_FINALLY();
+ {
+ walrcv_disconnect(wrconn);
+ }
+ PG_END_TRY();

I think it's better to drop the replication slots at the very end of
AlterSubscription_refresh_table() if possible because
ExecuteTruncate() can fail for many reasons (e.g., foreign key
constraints, insufficient privileges etc.).

Or is it worth considering an alternative design that the tablesync
worker truncates the table in the same transaction of the COPY?

---
Since REFRESH TABLE always copies the table data, I think we need the
following twophasestate check that is done in REFRESH PUBLICATION
command:

if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED &&
opts.copy_data)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION ... REFRESH PUBLICATION with
copy_data is not allowed when two_phase is enabled"),
errhint("Use ALTER SUBSCRIPTION ... REFRESH PUBLICATION
with copy_data = false, or use DROP/CREATE SUBSCRIPTION.")));

---
+
+ relid = RangeVarGetRelid(rv, AccessShareLock, false);
+

I think we should take an AccessExclusiveLock when opening the table
instead of escalating the lock level during the truncation.

---
+ tstmt = makeNode(TruncateStmt);
+ tstmt->relations = truncrels;
+ tstmt->restart_seqs = false;
+ tstmt->behavior = DROP_RESTRICT;
+ ExecuteTruncate(tstmt);

Since we already resolve individual specified table names we should
use their OID rather than passing a list of RangeVar to let
ExecuteTransaction() resolve the OIDs again. So we should use
ExecuteTruncateGuts() instead.

---
+ if (sub->enabled)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("%s is not allowed for enabled
subscriptions",
+ "ALTER SUBSCRIPTION ... REFRESH TABLE"),
+ errhint("Disable the subscription with
ALTER SUBSCRIPTION ... DISABLE first."));

I think we need to check if the apply worker actually stopped, in
addition to this check. Sub->enabled being false doesn't guarantee
that the apply worker is not working.

---
+ foreach_oid(relid, relids)
+ {
...
+ if (relstate != SUBREL_STATE_READY && relstate != SUBREL_STATE_SYNCDONE)
+ {
...
+
+ must_use_password = sub->passwordrequired && !sub->ownersuperuser;
+ wrconn = walrcv_connect(SubscriptionConninfo(sub), true, true,
+ must_use_password, sub->name, &err);

The function establishes connections for each relation. We should use
the one connection for all relations.

---
Please update psql's tab-completion for the new syntax.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com