On Mon, 31 Aug 2026 at 08:45, Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Friday, August 28, 2026 3:16 PM Shinya Kato <[email protected]>
> wrote:
> > On Thu, Aug 27, 2026 at 11:23 AM Zhijie Hou (Fujitsu)
> > <[email protected]> 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
use strict;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
if ($ENV{enable_injection_points} ne 'yes')
{
plan skip_all => 'Injection points not supported by this build';
}
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->start;
if (!$node_publisher->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
$node_publisher->safe_psql('postgres', 'CREATE EXTENSION injection_points');
# payload is int, so it is fixed-length and can never be stored out-of-line,
# whatever it holds.
$node_publisher->safe_psql(
'postgres', qq{
CREATE TABLE tab_race (id int PRIMARY KEY, payload int);
CREATE PUBLICATION pub_sync FOR TABLE tab_race;
CREATE PUBLICATION pub_filtered;
});
$node_subscriber->safe_psql('postgres',
'CREATE TABLE tab_race (id int PRIMARY KEY, payload int)');
my $connstr = $node_publisher->connstr . ' dbname=postgres';
$node_subscriber->safe_psql(
'postgres', "
CREATE SUBSCRIPTION sub
CONNECTION '$connstr application_name=sub'
PUBLICATION pub_sync, pub_filtered");
# Sync while the table is empty, so it reaches READY without copying anything.
$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub');
# From here the table belongs to no publication, so the row below is never
# published. The subscriber's pg_subscription_rel is unaffected, so the table
# stays READY.
$node_publisher->safe_psql('postgres',
'ALTER PUBLICATION pub_sync DROP TABLE tab_race');
$node_publisher->safe_psql('postgres',
'INSERT INTO tab_race VALUES (1, 42)');
is( $node_publisher->safe_psql(
'postgres', q{
SELECT attlen FROM pg_attribute
WHERE attrelid = 'tab_race'::regclass AND attname = 'payload'}),
'4',
'the payload column is fixed-length');
$node_publisher->wait_for_catchup('sub');
is( $node_subscriber->safe_psql('postgres', 'SELECT count(*) FROM tab_race'),
'0',
'nothing is replicated while the table is in no publication');
###############################################################################
# Hold the UPDATE between reading the publication descriptor and writing WAL,
# and add the row filter in that window.
###############################################################################
my $upd = $node_publisher->background_psql('postgres');
$upd->query_safe('SELECT injection_points_set_local()');
$upd->query_safe(
"SELECT injection_points_attach('heap_update-before-pin', 'wait')");
# Moves the row into the filter's set, leaving the payload untouched. Issued
# without waiting, since it is about to park.
$upd->query_until(
qr/^issued$/m, qq{
\\echo issued
UPDATE tab_race SET id = 2 WHERE id = 1;
});
$node_publisher->wait_for_event('client backend', 'heap_update-before-pin');
# ShareUpdateExclusiveLock does not conflict with the updating backend's
# RowExclusiveLock, so this commits while that backend is parked.
$node_publisher->safe_psql('postgres',
'ALTER PUBLICATION pub_filtered ADD TABLE tab_race WHERE (id = 2)');
is( $node_publisher->safe_psql(
'postgres', q{
SELECT count(*) FROM pg_publication_rel r
JOIN pg_publication p ON p.oid = r.prpubid
WHERE p.pubname = 'pub_filtered' AND r.prqual IS NOT NULL}),
'1',
'the row filter is committed while the update is parked');
$node_publisher->safe_psql(
'postgres', "
SELECT injection_points_wakeup('heap_update-before-pin');
SELECT injection_points_detach('heap_update-before-pin');");
ok($upd->quit, 'the update completes');
$node_publisher->wait_for_catchup('sub');
###############################################################################
# The transformed INSERT must carry the whole row, and for a fixed-length column
# it always can: the value is in the new tuple image in WAL.
###############################################################################
is( $node_subscriber->safe_psql(
'postgres', 'SELECT count(*) FROM tab_race WHERE id = 2'),
'1',
'the transformed INSERT reaches the subscriber');
is( $node_subscriber->safe_psql(
'postgres', 'SELECT payload FROM tab_race WHERE id = 2'),
'42',
'a fixed-length value survives the race that loses an out-of-line one');
# Nothing was silently nulled: slot_store_data()'s UNCHANGED fallback was never
# reached, because the publisher never had reason to send UNCHANGED.
is( $node_subscriber->safe_psql(
'postgres', q{
SELECT count(*) FROM tab_race WHERE id = 2 AND payload IS NULL}),
'0',
'the fixed-length column is not NULL on the subscriber');
$node_subscriber->stop;
$node_publisher->stop;
done_testing();
use strict;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
if ($ENV{enable_injection_points} ne 'yes')
{
plan skip_all => 'Injection points not supported by this build';
}
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->start;
if (!$node_publisher->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
$node_publisher->safe_psql('postgres', 'CREATE EXTENSION injection_points');
# SET STORAGE EXTERNAL keeps the value out-of-line and uncompressed, which is
# what makes it absent from the update's WAL record when it is left unchanged.
$node_publisher->safe_psql(
'postgres', qq{
CREATE TABLE tab_race (id int PRIMARY KEY, payload text);
ALTER TABLE tab_race ALTER COLUMN payload SET STORAGE EXTERNAL;
CREATE PUBLICATION pub_sync FOR TABLE tab_race;
CREATE PUBLICATION pub_filtered;
});
$node_subscriber->safe_psql('postgres',
'CREATE TABLE tab_race (id int PRIMARY KEY, payload text)');
my $connstr = $node_publisher->connstr . ' dbname=postgres';
$node_subscriber->safe_psql(
'postgres', "
CREATE SUBSCRIPTION sub
CONNECTION '$connstr application_name=sub'
PUBLICATION pub_sync, pub_filtered");
# Sync while the table is empty, so it reaches READY without copying anything.
$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub');
# From here the table belongs to no publication, so the row below is never
# published. The subscriber's pg_subscription_rel is unaffected, so the table
# stays READY.
$node_publisher->safe_psql('postgres',
'ALTER PUBLICATION pub_sync DROP TABLE tab_race');
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_race VALUES (1, repeat('a', 5000))");
is( $node_publisher->safe_psql(
'postgres', q{
SELECT pg_column_toast_chunk_id(payload) IS NOT NULL
FROM tab_race WHERE id = 1}),
't',
'the payload is stored out-of-line');
$node_publisher->wait_for_catchup('sub');
is( $node_subscriber->safe_psql('postgres', 'SELECT count(*) FROM tab_race'),
'0',
'nothing is replicated while the table is in no publication');
###############################################################################
# Hold the UPDATE between reading the publication descriptor and writing WAL,
# and add the row filter in that window.
###############################################################################
my $upd = $node_publisher->background_psql('postgres');
$upd->query_safe('SELECT injection_points_set_local()');
$upd->query_safe(
"SELECT injection_points_attach('heap_update-before-pin', 'wait')");
# Moves the row into the filter's set, leaving the payload untouched. Issued
# without waiting, since it is about to park.
$upd->query_until(
qr/^issued$/m, qq{
\\echo issued
UPDATE tab_race SET id = 2 WHERE id = 1;
});
$node_publisher->wait_for_event('client backend', 'heap_update-before-pin');
# ShareUpdateExclusiveLock does not conflict with the updating backend's
# RowExclusiveLock, so this commits while that backend is parked.
$node_publisher->safe_psql('postgres',
'ALTER PUBLICATION pub_filtered ADD TABLE tab_race WHERE (id = 2)');
is( $node_publisher->safe_psql(
'postgres', q{
SELECT count(*) FROM pg_publication_rel r
JOIN pg_publication p ON p.oid = r.prpubid
WHERE p.pubname = 'pub_filtered' AND r.prqual IS NOT NULL}),
'1',
'the row filter is committed while the update is parked');
$node_publisher->safe_psql(
'postgres', "
SELECT injection_points_wakeup('heap_update-before-pin');
SELECT injection_points_detach('heap_update-before-pin');");
ok($upd->quit, 'the update completes');
$node_publisher->wait_for_catchup('sub');
###############################################################################
# The update crossed the filter's boundary, so it arrives as an INSERT. That
# INSERT must carry the whole row.
###############################################################################
is( $node_subscriber->safe_psql(
'postgres', 'SELECT count(*) FROM tab_race WHERE id = 2'),
'1',
'the transformed INSERT reaches the subscriber');
is( $node_subscriber->safe_psql(
'postgres', q{
SELECT count(*) FROM tab_race
WHERE id = 2 AND payload = repeat('a', 5000)}),
'1',
'the transformed INSERT carries the unchanged out-of-line value')
or diag(
'the row filter was committed after heap_update() had already decided '
. 'not to preserve the value, so it is missing from WAL; the subscriber '
. 'stored NULL');
$node_subscriber->stop;
$node_publisher->stop;
done_testing();
