Re: Logical replication row filter loses unchanged toasted columns

Hi Kato-san,

> I see three ways to deal with this.
>
> Option A: detect the missing value in pgoutput_row_filter() and raise
> an error naming the table and the column, trading silent data loss for
> a loud failure. The catch is that the error is not recoverable. Once
> the change is in WAL, neither ALTER TABLE ... REPLICA IDENTITY FULL
> nor dropping the row filter helps, because decoding uses the historic
> catalog snapshot from the time of the change. The only way forward is
> pg_replication_slot_advance() or recreating the subscription.
>
>
If the subscriber apply worker raises the error instead, replication stops
on that
one transaction and ALTER SUBSCRIPTION ... SKIP steps over exactly it.
That has been available since 15, so it covers every affected branch.
The user can also disable the subscription, repair the row by hand and
re-enable it. The failure is just as loud, but recovery is ordinary
rather than drastic. Additionally, if the subscriber was using "NOT NULL"
constraint on such a column, the issue will be handled similarly anyways.

The apply worker has enough context for a useful message: it knows the
remote relation and the column name, and can hint at REPLICA IDENTITY
FULL. It knows less about the cause than the publisher does, but the
hint can cover that.

One detail if this route is taken: the check should not go into
slot_store_data() itself. That function is also used for old tuples in
apply_handle_update() and apply_handle_delete(), and in
apply_handle_update_internal() and apply_handle_tuple_routing() it
materialises an UPDATE's new tuple purely for conflict reporting
(CT_UPDATE_MISSING and CT_UPDATE_ORIGIN_DIFFERS). In that last case an
unchanged out-of-line column is perfectly legitimate, so a check there
would fire on ordinary updates whose target row happens to be missing
locally.

apply_handle_insert(), immediately after slot_store_data(), looks like
the right place, and appears to be the only one needed: a partitioned
target routes the already-stored slot through apply_handle_tuple_routing()
with CMD_INSERT, so one check covers both cases.

Option B: when a table belongs to a publication with a row filter,
> make heap_update() log the whole old tuple, as it already does for
> REPLICA IDENTITY FULL. The existing copy loop then finds the value,
> the INSERT is complete, and no error is needed. This is the real fix,
> but every UPDATE of such a table writes the unchanged out-of-line
> value to WAL even when no transformation happens, which can be a large
> regression. It also needs a new field in PublicationDesc, so I do not
> think it can be back-patched.
>
>
The concern with option B is that every UPDATE of a row-filtered table
would write the unchanged out-of-line value even when no transformation
happens. I think that can be narrowed considerably: the extra logging
can be done based on HeapTupleHasExternal(), which is a single infomask bit
test requiring no deforming. ExtractReplicaIdentity() already uses
exactly that gate for REPLICA IDENTITY FULL.

The cost then falls on every UPDATE of a row-filtered table whose row
currently holds out-of-line values, rather than on every UPDATE of such
a table. Where the wide column is usually NULL or stays inline this is
close to free, and where every row is toasted the cost is real -- but
that is precisely the case where the current behaviour loses data.

What makes B attractive is that it needs no change to the output plugin,
the protocol or the subscriber. Once the old tuple carries the
flattened values, the copy loop already in pgoutput_row_filter() finds
them and the INSERT goes out complete. It is the same mechanism that
makes REPLICA IDENTITY FULL work today.

I agree it cannot be back-patched, for the reasons given: it needs to
know at heap_update() time that the table is published with a row
filter, and it would introduce a WAL volume regression in a minor
release.

> Option C: document the restriction and leave the behavior alone. This
> is the only option that changes nothing on the back branches, but the
> value keeps disappearing without any warning.

I lean towards A because losing data silently seems worse than
> stopping, but the unrecoverable error bothers me. Which approach do
> you prefer, and should the fix be back-patched?
>
>

Rather than choosing among the three, would this combination work?

- back branches (15 and up): the subscriber-side error described
above, together with a documentation note in the UPDATE
transformation section stating that a column which is stored
out-of-line, unchanged, and outside the replica identity cannot be
carried through the transformation, and that REPLICA IDENTITY FULL
avoids it.

- master: option B, so the INSERT is complete and no error is needed.

Option C then becomes the documentation half of the first item rather
than a standalone choice.

One thing worth being explicit about: back-patching an error changes
behaviour in a minor release. I still think it is the right trade, since the
alternative is undetectable data loss, and where the column is NOT NULL
replication already fails today—just with a constraint violation that points
at the symptom rather than the cause.

Thanks,
---
Nikhil Sontakke
PlanetScale