| From: | vignesh C <vignesh21(at)gmail(dot)com> |
|---|---|
| To: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
| Cc: | Shinya Kato <shinya11(dot)kato(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
| Subject: | Re: Logical replication row filter loses unchanged toasted columns |
| Date: | 2026-09-01 13:43:42 |
| Message-ID: | CALDaNm2MF18JPUz_rwMzbgeqy1PUwxVKwEU8E1cWPObq-yqxrg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, 31 Aug 2026 at 08:45, Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, August 28, 2026 3:16 PM Shinya Kato <shinya11(dot)kato(at)gmail(dot)com> wrote:
> > On Thu, Aug 27, 2026 at 11:23 AM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > Thanks for the comments. They look good to me and I have addressed them
> > > in V4 patch.
> >
> > Thanks for working on this! I reviewed v4 and have two comments for now.
>
> Thanks for the comments.
>
> >
> > 1. The extra logging does not actually depend on the key changing.
> >
> > ```
> > + old_key_tuple = BuildOldKeyTuple(relation, &oldtup, newtup,
> > + bms_overlap(modified_attrs, id_attrs) ||
> > + id_has_external,
> > + log_unchanged_external &&
> > + HeapTupleHasExternal(&oldtup) &&
> > + HeapTupleHasExternal(newtup),
> > + &old_key_copied);
> > ```
> >
> > The new columns are only kept when BuildOldKeyTuple() gets past its
> > `if (!key_required) return NULL`, and the key_required passed above is
> > `bms_overlap(modified_attrs, id_attrs) || id_has_external`, so
> > id_has_external alone gets it there. HeapDetermineColumnsInfo() sets
> > id_has_external when a replica identity column of the old tuple is
> > stored externally, with no key change at all. So on a table whose
> > replica identity covers a toasted column and which has another toasted
> > column outside it, an UPDATE that touches neither cannot be
> > transformed, yet still detoasts and flattens that column into the WAL
> > record.
>
> Right. I updated the patch to skip logging for this case.
>
> >
> > 2. The test never checks that val is actually stored out-of-line. If
> > that ever changed, the test would keep passing without exercising the
> > fix, since an inline value replicates fine anyway. Asserting
> > pg_column_toast_chunk_id(val) IS NOT NULL on the publisher before the
> > UPDATE would pin that down.
>
> Added the test.
>
> Here is the updated patch which addressed all comments including Kuroda-San's[1].
I found another issue with the UPDATE-to-INSERT transformation when an
unchanged column is stored out-of-line, where the row-filter
publication is added while the UPDATE is in progress.
The relcache check in heap_update() can become stale before the WAL
record is written. For example, ALTER PUBLICATION ... ADD TABLE can
commit while the UPDATE is parked, since its ShareUpdateExclusiveLock
does not conflict with the UPDATE's RowExclusiveLock. The UPDATE
therefore does not preserve the unchanged TOAST value, but pgoutput
later sees the row filter and transforms the UPDATE into an INSERT.
I added two tests. The fixed-length test passes, but the TOAST test fails:
ok 5 - the transformed INSERT reaches the subscriber
not ok 6 - the transformed INSERT carries the out-of-line value
# got: '0'
# expected: '1'
The subscriber receives the row, but the unchanged TOAST value becomes NULL.
The attached row_filter_concurrent_alter_fixedlen_v1.pl and
row_filter_concurrent_alter_toast_v1.pl tests demonstrate the
difference between the fixed-length and TOAST cases, including the
failure with the TOASTed value.
Thoughts?
Regards,
Vignesh
| Attachment | Content-Type | Size |
|---|---|---|
| row_filter_concurrent_alter_fixedlen_v1.pl | text/x-perl-script | 4.7 KB |
| row_filter_concurrent_alter_toast_v1.pl | text/x-perl-script | 4.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Zsolt Parragi | 2026-09-01 13:50:04 | Re: Offline data checksum changes can cause incorrect checksum state on standbys |
| Previous Message | William Bernbaum | 2026-09-01 13:15:14 | Price hashed aggregation for hash tables that exceed CPU cache capacity |