RE: Logical replication row filter loses unchanged toasted columns

On Thursday, August 20, 2026 6:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Wed, Aug 19, 2026 at 10:55 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > >
> > > On Thu, Aug 20, 2026 at 4:54 AM Masahiko Sawada
> <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > It can cost us high in some cases that can occur in the narrow
> > > window of above conditions. I could think of two major points to consider:
> > > (a) Changed toasted columns don't need their old value logged at all
> > > — once a column is toasted in this transaction, its new value is
> > > already fully resolvable from the new tuple, so flattening the old
> > > tuple just pays a detoast fetch and extra WAL for a value nothing
> > > downstream reads; (b) The trigger condition (rf_exists_for_update &&
> > > id_key_changed && HeapTupleHasExternal(oldtup)) can't tell whether
> > > the filter's match result actually differs between old and new row
> > > (we need additional WAL only when row_filter matches new row but not
> > > old), since that needs evaluating the filter itself, already
> > > rejected as too invasive inside heap_update().
> >
> > I think we can somehow check (a) during heap_update(). But I agree
> > that we should not check (b) during heap_update().
> >
>
> Fair enough. I think we can try to come up with a patch and see if it is good for
> both HEAD and back-branches. BTW, one thing to note in at least the current
> posted patch, there is an ABI break with new member rf_exists_for_update in
> PublicationDesc. I feel as it can be added at the end of structure, the chances
> of breaking any existing extension are very less. Also, the new parameter in
> pub_rf_contains_invalid_column could be harmless as the function shouldn't
> be used but we can overcome that by having a separate function like
> pub_has_row_filter() as shown in Hou-San's patch for
> PG18 version.

Here is the patch (on HEAD) for the suggested approach. I extended extractReplicaIdentity
to also extract unchanged toasted columns, as it fits best there, we only need
to log unchanged toast columns when the replica identity changes.

Since it now extracts more than just the replica identity, I renamed it to the
more general BuildOldKeyTuple. The new logic to find unchanged toasted columns
reuses the existing loop that marks non-replica-identity columns as NULL for old
tuples, so the additional overhead (beyond the new WAL logging) should not be
noticeable.

Best Regards,
Zhijie Hou