> > 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 >
v2-0001-refresh-table.patch
Description: Binary data
