Re: Per-table resync for logical replication subscriptions

Hello Ajin,

Thank you for the review. This needs a change outside the feature;
v7 is attached.

> 1. The header above CheckRefreshTableNotInOtherSubscriptions() has two
> sets of comments, the first one I feel is meant for
> AlterSubscription_refresh_table() which is without a header.

Right , done.

> 2.
> + /*
> + * The local copy is discarded below, so require the same privilege
> + * TRUNCATE itself would.
> + */
> + aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)),
> + rv->relname);
>
> rather than doing it this way, I believe you need to call
> truncate_check_rel(relid, rel->rd_rel);
> truncate_check_perms(relid, rel->rd_rel);
> truncate_check_activity(rel);
>
> else you will miss out on checking for temp tables of other backends
> and invoking InvokeObjectTruncateHook(relid) which are object access
> hooks.

Agreed. v6 does call the hook, but by hand;
v7 does it the way you describe, which also brings in the temp table check
that the hand-rolled version was still missing.

So v7 makes the three functions extern, which is no functional change on its
own, and calls them as you describe:

truncate_check_rel(relid, userrel->rd_rel);
truncate_check_activity(userrel);

for every relation that will be truncated, and

truncate_check_perms(relid, userrel->rd_rel);

for the relations actually named in the command. The privilege check stays
in
the first pass, earlier than the other two, so that the command is rejected
before it escalates any lock. It is deliberately not applied to the
partitions
that come along with a named partitioned table, which follows what
ExecuteTruncate() does for inheritance children:

* Inherited TRUNCATE commands perform access permission
* checks on the parent table only. So we skip checking the
* children's permissions and don't call
* truncate_check_perms() here.
*/
truncate_check_rel(RelationGetRelid(rel), rel->rd_rel);
truncate_check_activity(rel);

Two consequences are behaviour changes rather than tidying, so they are
worth
naming.

Your point about other backends' temp tables was the substantive one. The
check was being applied only to the partitions the command collects, on the
argument that a temp table cannot be a partition of a permanent one; the
relations named in the command were not covered at all, which left that case
resting on an untested assumption about what can appear in
pg_subscription_rel. That is not the kind of thing to lean on.
truncate_check_activity() now covers them, and errors rather than skips, as
ExecuteTruncate() does for explicitly named relations.

truncate_check_activity() calls CheckTableNotInUse(rel, "TRUNCATE"), so that
message now says TRUNCATE rather than naming the subscription command. I
preferred that to keeping a hand-rolled copy for the sake of one string, but
say the word if you would rather it read ALTER SUBSCRIPTION ... REFRESH
TABLE
and I will thread the statement name through.

Best regards,
Cagri Biroglu

On Tue, Aug 25, 2026 at 7:52 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:

> On Wed, Aug 19, 2026 at 9:10 PM Cagri Biroglu <cagri(dot)biroglu(at)adyen(dot)com>
> wrote:
> >
> > Hello Hayato,
> >
> > Thank you for the review, v5 is attached.
> >
>
> Hello Cagri,
>
> Some minor comments:
> 1. The header above CheckRefreshTableNotInOtherSubscriptions() has two
> sets of comments, the first one I feel is meant for
> AlterSubscription_refresh_table() which is without a header.
>
> 2.
> + /*
> + * The local copy is discarded below, so require the same privilege
> + * TRUNCATE itself would.
> + */
> + aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)),
> + rv->relname);
>
> rather than doing it this way, I believe you need to call
> truncate_check_rel(relid, rel->rd_rel);
> truncate_check_perms(relid, rel->rd_rel);
> truncate_check_activity(rel);
>
> else you will miss out on checking for temp tables of other backends
> and invoking InvokeObjectTruncateHook(relid) which are object access
> hooks.
>
> regards,
> Ajin Cherian
> Fujitsu Australia
>