Re: Per-table resync for logical replication subscriptions

>
> Thanks a lot for looking at this.
>
> Before the point-by-point: one change after getting feedback on this

from the sketch in my first mail

is that I dropped the WITH (copy_data, truncate) options. For refreshing
> a table that is already subscribed, only one combination is useful, so
> REFRESH TABLE now always truncates the local copy and re-copies (what
> would have been copy_data = true, truncate = true). Without truncating,
> the re-copy hits duplicate-key errors or doubles the rows; skipping the
> copy leaves the drift in place. So neither other combination is
> meaningful, and there is nothing to configure.
>
> > REFRESH TABLE is better to me. It's more consistent with the existing
> > commands such as REFRESH SEQUENCES.
>
> Great, I've kept REFRESH TABLE.
>
> > I think it might be a good idea that the REFRESH TABLE command can
> > take multiple tables rather than one table.
>
> Agreed. v2 makes REFRESH TABLE take a comma-separated list:
>
> ALTER SUBSCRIPTION name REFRESH TABLE t1, t2, ...
>
> It's all-or-nothing: every named relation is validated (part of the
> subscription, not a sequence, exists) before any of them is reset, so a
> bad table name aborts the whole command without touching the others. The
> listed tables are also truncated together, which lets a set connected by
> foreign keys be re-seeded as a unit.
>
> One note on scope: this doesn't add resource risk over doing one table
> at a time, because the re-copy is still performed by tablesync workers
> that are capped by max_sync_workers_per_subscription; resetting N tables
> just queues them rather than starting N copies at once.
>
> > Why does the REFRESH TABLE command require for the subscription to be
> > disabled while REFRESH PUBLICATIONS doesn't?
>
> Good question, but I'd actually argue the disabled requirement is a
> reasonable design choice here rather than a limitation to remove.
>
> The reason the two commands differ is that REFRESH PUBLICATION only adds
> *new* relations to pg_subscription_rel (in init state), whereas REFRESH
> TABLE resets a relation that is already in ready state, and those hit
> different caches in the apply worker:
>
> - The set of not-ready relations is cached (table_states_not_ready in
> syncutils.c) and correctly invalidated by the SUBSCRIPTIONRELMAP
> syscache callback (InvalidateSyncingRelStates). So after the reset the
> apply worker does notice the table is non-ready and does launch a
> tablesync worker for it. That part works whether enabled or not.
>
> - However, whether the apply worker applies incoming changes for a
> relation is decided by should_apply_changes_for_rel(), which reads the
> per-relation LogicalRepRelMapEntry.state. That state is only refreshed
> from the catalog while it is not READY:
>
> /* relation.c, logicalrep_rel_open() */
> if (entry->state != SUBREL_STATE_READY)
> entry->state = GetSubscriptionRelState(...);
>
> For an already-ready table the entry stays READY, and the relcache
> invalidation only clears localrelvalid (rebuilds the attrmap); it does
> not reset entry->state. So the running apply worker keeps treating the
> table as READY and keeps applying live changes to it, at the same time
> as the freshly launched tablesync worker re-copies it, with no handoff
> on the sync LSN. In my testing that reliably diverges the table.
>
> For a newly added table (REFRESH PUBLICATION) there is no stale READY
> entry, so the normal init -> datasync -> syncdone -> ready handoff works
> without any disable.
>
> Requiring the subscription to be disabled sidesteps this cleanly. When
> you re-enable, a fresh apply worker starts and reads the reset table's
> state from the catalog as init, so the same init -> datasync -> syncdone
> -> ready handoff kicks in, exactly as for a newly added table. And the
> disable only needs to bracket the quick catalog reset, not the re-copy
> itself: REFRESH TABLE just truncates and flips the state, and after
> ENABLE the tablesync worker does the (possibly long) copy while the
> apply worker resumes the other tables as usual, so the pause is short
> and scoped to the metadata change, not the data copy.
>
> Supporting the enabled case would instead mean reaching into the running
> apply worker to force it to drop and re-read the cached state of the
> reset relation mid-flight. That is doable, but it adds moving parts and
> a new invalidation path to get right, for what is a deliberate,
> occasional maintenance action where briefly disabling the subscription
> does no harm. So my inclination is to keep the disabled requirement here
> rather than trade that simplicity away. Happy to reconsider if you think
> the enabled case matters enough to justify the extra machinery or i am
> missing sth.

Regards,
> Cagri Biroglu
>