RE: Logical replication row filter loses unchanged toasted columns

On Monday, August 17, 2026 3:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Aug 14, 2026 at 10:09 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> > 0001: heap_update check
> >
>
> Few comments on 0001:

Thanks for the comments.

> ===================
> 1.
> @@ -3453,6 +3478,34 @@ heap_update(Relation relation, const
> ItemPointerData *otid, HeapTuple newtup,
> id_attrs, &oldtup,
> newtup, &id_has_external);
>
> + id_changed = bms_overlap(modified_attrs, id_attrs);
> +
> + /*
> + * If the update could be transformed into an insert by a publication
> + row
> + * filter during decoding, reject it when it would lose an unchanged
> + * out-of-line value of a column that is not part of the replica identity.
> + */
> + if (check_unchanged_external && id_changed)
>
> Why did you place the above check in heap_update before label l2? If the
> check ran before l2: (e.g. right where modified_attrs/id_key_changed are first
> computed), a raised
> ereport(ERROR) there could fire for an update attempt that was never actually
> going to happen, the row might get updated by someone else in the interim,
> EvalPlanQual retries with a different row version, and our error would have
> been wrong or at least premature. Placing the check after the TM_Ok
> confirmation and after the VM-pin retry (i.e.
> after every goto l2 site) guarantees no more retries follow, so raising the error
> here means the update really was about to proceed against this exact tuple.

Right, I agree we should move this after l2.

>
> 2. Can we check the required value from relation's pubdesc before calling
> RelationBuildPublicationDesc()?

I think we can do this by adding a new relcache API that only accesses the new
flag. I've done that in this version.

>
> > I haven't added doc yet, but I can add it once we reach consensus.
> >
>
> Feel free to add where required.

Added.

Here's the updated version.

In this version, I extended pub_rf_contains_invalid_column to also check for row
filter existence, rather than adding a new function. This is fine for HEAD, but
for back branches we typically avoid changing public interfaces, so a new
function might be needed there. However, since this is an internal cache
function and I couldn't find any extensions (via GitHub or Debian code search)
that use it, changing the interface is probably acceptable.

I am sharing one version v2_PG18 that does not change existing function
interface for PG18 for reference.

BTW, I also couldn't find any extensions that depend on the size of this struct, so
I personally think backpatching should be fine.

Best Regards,
Zhijie Hou