| From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
|---|---|
| To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
| Cc: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>, Shinya Kato <shinya11(dot)kato(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Logical replication row filter loses unchanged toasted columns |
| Date: | 2026-08-20 07:31:49 |
| Message-ID: | CAD21AoBH_8Y2CAUMrmJN5LEYyKBNGPBgqSkpw1O_81NXvW01zg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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:
> >
> > On Fri, Aug 14, 2026 at 4:50 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > I think even if we want to block operations that can create such a
> > > situation, we should reject only the specific updates that lead to the
> > > problem, not every update on a table that merely has the potential for
> > > it. We already do something similar: UPDATE/DELETE is rejected when
> > > there's no replica identity and the table's publications publish those
> > > operations. I'd like to apply the same principle here.
> > >
> > > With that in mind, I could think of following two options:
> > >
> > > Option 1
> > > Check at DML time, inside heap_update(): Detect the problem per-row,
> > > at the point where old/new tuple data is actually available when
> > > following conditions are met: the relation is published and has
> > > UPDATEs enabled, (b) some publication defines a row filter on it, (c)
> > > the replica identity key changed value in this UPDATE, (d) the old
> > > tuple has some externally-stored (TOASTed) attribute
> > > (HeapTupleHasExternal()), and (e) some specific non-replica-identity
> > > column's value is unchanged and still stored out-of-line.
> > >
> > > Only an UPDATE that actually satisfies all five conditions is
> > > rejected, with an error naming the offending column. All other UPDATEs
> > > on the same table proceed normally, including ones that don't touch
> > > the key, or ones where the TOASTed column did change.
> >
> > It looks like this bug can happen when all of the above five
> > conditions are met, which seems to be narrow in practice. If the
> > affected cases are that narrow, why don't we log the whole old tuple
> > image in exactly that case, instead of erroring out?
> >
> 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().
>
> So, combining both cases, I am worried it could be additional CPU and
> WAL cost when the same is not required though only in narrow cases.
> But I see there is an argument that all of this work is done when
> toasting for both old and new tuples is there which is not a very hot
> code path but still I am not sure this additional cost is worth it.
>
> > While it does
> > write more WAL, but only in that narrow case, It would be better than
> > requiring users to change RI setting or publication settings. Also, I
> > guess it can be back-patched as it neither adds a new WAL record type
> > nor changes the existing WAL format. Ideally, it would be sufficient
> > to write RI + unchanged out-of-line column data. But that needs new
> > logic to select the attribute, so I would leave it for the master.
> >
>
> Right, we can have this solution for master but thought first we can
> have a somewhat less risky fix for backbranches (including master).
> Then as a separate task try to improve the situation in master by
> considering various options like the one you suggested, or have
> something like INCLUDE kind of syntax for RI, or yet another one is to
> consider evaluating row_filter for such narrow cases.
While the proposed fix is less risky, I'd like to be clear about what
a user can actually do when it fires. The patch suggests "To enable
updating the table, set REPLICA IDENTITY FULL or remove the row filter
from the publication.". As far as I can see, possible actions are:
1. Set RI FULL.
2. Remove the row filter from the publication.
3. Rewrite the offending statements to also modify the out-of-line column.
4. Change the application so that it never issues such updates.
(1) doesn't actually work if the publication has a column list.
pub_contains_invalid_column() rejects that combination (RI FULL +
column list). So every UPDATE on the table fails. Even if users remove
the column list from the publication, RI FULL logs the whole old tuple
on every UPDATE, which is more WAL and more CPU than what I proposed.
(2) could require some system architecture changes. Dropping the
filter sends the subscriber rows it was deliberately not supposed to
receive, and adds the network traffic and apply cost for all of them.
(3) works: e.g., "UPDATE t SET id = 7, body = body || ''" passes the
check. But it writes a fresh copy of the value into the toast table,
so it costs at least as much WAL as logging the old value would have,
and leaves the previous toast rows behind for vacuum.
(4) I don't think this is reachable in practice.
Am I missing an option? If there is a workaround that keeps both the
row filter and the column list, and does not cost more than the WAL we
are trying to avoid, it would be great. Otherwise, I'm concerned that
prohibiting these updates leaves users without a practical answer.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | jian he | 2026-08-20 07:41:41 | Re: MERGE/SPLIT PARTITIONS issues/questions |
| Previous Message | Andrei Lepikhov | 2026-08-20 07:23:38 | Re: Allow a prosupport function to be attached to an aggregate |