Re: Logical replication row filter loses unchanged toasted columns

>
> 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 took a close look at option B and it might not be so attractive (what
ever is? :-))
and might need invasive changes.

> 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.
>
>
But this backpatching reason still holds good as far as I can see!

Regards,
Nikhil

> - 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
>