| From: | Cagri Biroglu <cagri(dot)biroglu(at)adyen(dot)com> |
|---|---|
| To: | Yilin Zhang <jiezhilove(at)126(dot)com> |
| Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "smithpb2250(at)gmail(dot)com" <smithpb2250(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Per-table resync for logical replication subscriptions |
| Date: | 2026-08-15 07:23:23 |
| Message-ID: | CAA36msquAsMmy-trbvq=_MyB+YU-8cTbVp9nDEdoT6xu0ootxQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thank you for testing this, and please don't worry about the timing. You
found a data loss bug, which is worth much more to me than a fast reply.
You were right, and it was worse than the symptom you described. I rebuilt
v3 to check. With an inheritance parent and child both subscribed:
publisher: ip (own rows) = 2, ic = 2
v3, ALTER SUBSCRIPTION s REFRESH TABLE ip:
after refresh: ip ONLY = 0, ic = 0 <- child truncated too
catalog state: ip = 'i', ic = 'r' <- only ip reset
after enable: ip ONLY = 2, ic = 0 <- child never copied back
The child's rows were gone permanently. v3 passed the parse tree's
RangeVars to ExecuteTruncate(), and qualified_name produces RangeVars with
inh = true, so the truncation recursed into inheritance children while the
state reset only touched the relations named in the command. That is
exactly the mismatch in your fourth point.
The rule I should have written down at the start, and which is now a
comment in the code, is: never discard a relation unless we also reset the
state of whatever will refill it.
Rewriting this function to call ExecuteTruncateGuts() with resolved OIDs,
which Sawada-san had asked for on this thread, fixes it as a side effect,
because recursion is then driven by relkind and only follows partitions.
The same scenario on v4 ends at ip ONLY = 2, ic = 2, matching the
publisher.
> its current behavior is equivalent to the TRUNCATE ONLY syntax
For inheritance that is what v4 does deliberately, though v3 did the
opposite. Following children is not safe: a child is an independent
relation that may be subscribed in its own right, or may hold data that is
not replicated at all, so truncating it from the parent's refresh throws
away rows nothing will copy back. Naming both refreshes both:
ALTER SUBSCRIPTION s REFRESH TABLE parent, child;
Partitions are different and are still followed, because a partitioned
table is only tracked when the publication uses publish_via_partition_root,
and then its partitions are not tracked separately, so the root's re-copy
refills them by tuple routing.
> The partitioned parent table cannot be refreshed directly
Working as intended, but the error gave you no way to work that out, which
is fair. Without publish_via_partition_root the subscription tracks the
partitions, so naming the root hits "is not part of the subscription". v4
adds a hint:
HINT: The publication may publish its partitions individually; refresh
those instead.
> SELECT * FROM parent returns only residual child rows during resync
Real and expected on v4: the child's rows are still valid replicated data
and are left alone, while the parent's own rows are re-copied. The broader
point, that a refreshed table is visibly empty until its sync finishes, is
inherent to truncate-and-recopy. Removing it needs the copy to go into a
new relfilenode which is then swapped in, like REFRESH MATERIALIZED VIEW
CONCURRENTLY. I would rather keep that out of this patch, but it is the
natural follow-up.
A separate review then found the same class of bug across subscriptions: a
relation that another subscription also populates was truncated without
that subscription's state being reset. v4 rejects that up front.
v4 is posted in my reply to Masahiko on this thread, so I am not
duplicating the attachment. v3 was the last version posted and three
reviews landed on it, so v4 folds in all of them at once. The parts that
came from your report are the inheritance fix above, the hint for an
untracked partitioned root, documentation of the partition and inheritance
behaviour, and new regression checks.
Thanks again for catching this.
Regards,
Cagri Biroglu
On Tue, Aug 11, 2026 at 12:21 PM Yilin Zhang <jiezhilove(at)126(dot)com> wrote:
> At 2026-08-10 16:53:04, "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).
> > 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.
>
> hi,
> I have some review feedback on the v3-0001 patch.
> The handling of inheritance tables and partition child tables with REFRESH
> TABLE seems incomplete; its current behavior is equivalent to the TRUNCATE
> ONLY syntax.
> This could result in the following problems:
> After executing REFRESH TABLE and before the tablesync worker completes
> synchronization, running SELECT * FROM parent will only return residual
> data from child tables, while rows residing directly in the parent table
> will be missing.
> The partitioned parent table cannot be refreshed directly.
> After the subscription is disabled, the states of child tables are not
> reset.
> I originally expected data conflicts would arise, yet testing showed no
> critical data anomalies, and the data eventually became consistent.
> Could you please assess whether this constitutes an issue that needs
> addressing?
> Besides, I began reviewing this patch yesterday but have progressed rather
> slowly. The patch is now tagged "Waiting on Author". I apologize if this
> has held up your development work.
>
> Best regards,
> --
> Yilin Zhang
>
>
>
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Cagri Biroglu | 2026-08-15 07:38:21 | Re: Per-table resync for logical replication subscriptions |
| Previous Message | Cagri Biroglu | 2026-08-15 07:21:31 | Re: Per-table resync for logical replication subscriptions |