pgoutput incorrectly replaces missing values with NULL since PostgreSQL 15
While working on Materialize's streaming logical replication from Postgres [0]https://materialize.com/docs/sql/create-source/postgres,
my colleagues Sean Loiselle and Petros Angelatos (CC'd) discovered today what
appears to be a correctness bug in pgoutput, introduced in v15.
The problem goes like this. A table with REPLICA IDENTITY FULL and some
data in it...
CREATE TABLE t (a int);
ALTER TABLE t REPLICA IDENTITY FULL;
INSERT INTO t VALUES (1), (2), (3), ...;
...undergoes a schema change to add a new column with a default:
ALTER TABLE t ADD COLUMN b bool DEFAULT false NOT NULL;
PostgreSQL is smart and does not rewrite the entire table during the schema
change. Instead it updates the tuple description to indicate to future readers
of the table that if `b` is missing, it should be filled in with the default
value, `false`.
Unfortunately, since v15, pgoutput mishandles missing attributes. If a
downstream server is subscribed to changes from t via the pgoutput plugin, when
a row with a missing attribute is updated, e.g.:
UPDATE t SET a = 2 WHERE a = 1
pgoutput will incorrectly report b's value as NULL in the old tuple, rather than
false. Using the same example:
old: a=1, b=NULL
new: a=2, b=true
The subscriber will ignore the update (as it has no row with values
a=1, b=NULL), and thus the subscriber's copy of `t` will become out of sync with
the publisher's.
I bisected the problem to 52e4f0cd4 [1]https://github.com/postgres/postgres/commit/52e4f0cd472d39d07732b99559989ea3b615be78, which introduced row filtering for
publications. The problem appears to be the use of CreateTupleDescCopy where
CreateTupleDescCopyConstr is required, as the former drops the constraints
in the tuple description (specifically, the default value constraint) on the
floor.
I've attached a patch which both fixes the issue and includes a test. I've
verified that the test fails against the current master and passes against
the patched version.
I'm relatively unfamiliar with the project norms here, but assuming the patch is
acceptable, this strikes me as important enough to warrant a backport to both
v15 and v16.
[0]: https://materialize.com/docs/sql/create-source/postgres
[1]: https://github.com/postgres/postgres/commit/52e4f0cd472d39d07732b99559989ea3b615be78
Attachments:
0001-pgoutput-don-t-unconditionally-fill-in-missing-value.patchapplication/octet-stream; name=0001-pgoutput-don-t-unconditionally-fill-in-missing-value.patchDownload
From 7b875ebf568de3e2dd72d142ff7ba803a4651118 Mon Sep 17 00:00:00 2001
From: Nikhil Benesch <nikhil.benesch@gmail.com>
Date: Thu, 23 Nov 2023 02:19:53 -0500
Subject: [PATCH] pgoutput: don't unconditionally fill in missing values with
NULL
52e4f0cd4 introduced a bug in pgoutput in which missing values in tuples
were incorrectly filled in with NULL. The problem was the use of
CreateTupleDescCopy where CreateTupleDescCopyConstr was required, as the
former drops the constraints in the tuple description (specifically, the
default value constraint) on the floor.
The bug could result in incorrectness when a table replicated via
`REPLICA IDENTITY FULL` underwent a schema change that added a column
with a default value.
---
src/backend/replication/pgoutput/pgoutput.c | 4 +-
src/test/subscription/t/100_bugs.pl | 55 +++++++++++++++++++++
2 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index e8add5ee5d..f9ed1083df 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1136,8 +1136,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
* Create tuple table slots. Create a copy of the TupleDesc as it needs to
* live as long as the cache remains.
*/
- oldtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
- newtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
+ oldtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
+ newtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
entry->old_slot = MakeSingleTupleTableSlot(oldtupdesc, &TTSOpsHeapTuple);
entry->new_slot = MakeSingleTupleTableSlot(newtupdesc, &TTSOpsHeapTuple);
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 7e5221afff..089a4fcaf0 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -438,4 +438,59 @@ is( $node_subscriber->safe_psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that pgoutput was incorrectly replacing missing attributes in
+# tuples with NULL. This could result in incorrect replication with
+# `REPLICA IDENTITY FULL`.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+# Set up a table with schema `(a int, b bool)` where the `b` attribute is
+# missing for one row due to the `ALTER TABLE ... ADD COLUMN ... DEFAULT`
+# fast path.
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_default (a int)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE tab_default REPLICA IDENTITY FULL");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_default VALUES (1)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE tab_default ADD COLUMN b bool DEFAULT false NOT NULL");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_default VALUES (2, true)");
+
+# Replicate to the subscriber.
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_default (a int, b bool)");
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION pub1 FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1"
+);
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(1|f
+2|t), 'check snapshot on subscriber');
+
+# Update all rows in the table and ensure the rows with the missing `b`
+# attribute replicate correctly.
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_default SET a = a + 1");
+$node_publisher->wait_for_catchup('sub1');
+
+# When the bug is present, the `1|f` row will not be updated to `2|f` because
+# the publisher incorrectly fills in `NULL` for `b` and publishes an update
+# for `1|NULL`, which doe snot exist in the subscriber.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(2|f
+3|t), 'check replicated update on subscriber');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
+
done_testing();
--
2.41.0
On Thu, Nov 23, 2023 at 1:10 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
While working on Materialize's streaming logical replication from Postgres [0],
my colleagues Sean Loiselle and Petros Angelatos (CC'd) discovered today what
appears to be a correctness bug in pgoutput, introduced in v15.The problem goes like this. A table with REPLICA IDENTITY FULL and some
data in it...CREATE TABLE t (a int);
ALTER TABLE t REPLICA IDENTITY FULL;
INSERT INTO t VALUES (1), (2), (3), ...;...undergoes a schema change to add a new column with a default:
ALTER TABLE t ADD COLUMN b bool DEFAULT false NOT NULL;
PostgreSQL is smart and does not rewrite the entire table during the schema
change. Instead it updates the tuple description to indicate to future readers
of the table that if `b` is missing, it should be filled in with the default
value, `false`.Unfortunately, since v15, pgoutput mishandles missing attributes. If a
downstream server is subscribed to changes from t via the pgoutput plugin, when
a row with a missing attribute is updated, e.g.:UPDATE t SET a = 2 WHERE a = 1
pgoutput will incorrectly report b's value as NULL in the old tuple, rather than
false.
Thanks, I could reproduce this behavior. I'll look into your patch.
Using the same example:
old: a=1, b=NULL
new: a=2, b=trueThe subscriber will ignore the update (as it has no row with values
a=1, b=NULL), and thus the subscriber's copy of `t` will become out of sync with
the publisher's.I bisected the problem to 52e4f0cd4 [1], which introduced row filtering for
publications. The problem appears to be the use of CreateTupleDescCopy where
CreateTupleDescCopyConstr is required, as the former drops the constraints
in the tuple description (specifically, the default value constraint) on the
floor.I've attached a patch which both fixes the issue and includes a test. I've
verified that the test fails against the current master and passes against
the patched version.I'm relatively unfamiliar with the project norms here, but assuming the patch is
acceptable, this strikes me as important enough to warrant a backport to both
v15 and v16.
Right.
--
With Regards,
Amit Kapila.
On Thu, Nov 23, 2023 at 2:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Nov 23, 2023 at 1:10 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
While working on Materialize's streaming logical replication from Postgres [0],
my colleagues Sean Loiselle and Petros Angelatos (CC'd) discovered today what
appears to be a correctness bug in pgoutput, introduced in v15.The problem goes like this. A table with REPLICA IDENTITY FULL and some
data in it...CREATE TABLE t (a int);
ALTER TABLE t REPLICA IDENTITY FULL;
INSERT INTO t VALUES (1), (2), (3), ...;...undergoes a schema change to add a new column with a default:
ALTER TABLE t ADD COLUMN b bool DEFAULT false NOT NULL;
PostgreSQL is smart and does not rewrite the entire table during the schema
change. Instead it updates the tuple description to indicate to future readers
of the table that if `b` is missing, it should be filled in with the default
value, `false`.Unfortunately, since v15, pgoutput mishandles missing attributes. If a
downstream server is subscribed to changes from t via the pgoutput plugin, when
a row with a missing attribute is updated, e.g.:UPDATE t SET a = 2 WHERE a = 1
pgoutput will incorrectly report b's value as NULL in the old tuple, rather than
false.Thanks, I could reproduce this behavior. I'll look into your patch.
I verified your fix is good and made minor modifications in the
comment. Note, that the test doesn't work for PG15, needs minor
modifications.
--
With Regards,
Amit Kapila.
Attachments:
v2-0001-Avoid-unconditionally-filling-in-missing-values-w.patchapplication/octet-stream; name=v2-0001-Avoid-unconditionally-filling-in-missing-values-w.patchDownload
From 458d261bfda73f5e6916ddd68a35a73b6134652e Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 24 Nov 2023 15:31:05 +0530
Subject: [PATCH v2] Avoid unconditionally filling in missing values with NULL
in pgoutput.
52e4f0cd4 introduced a bug in pgoutput in which missing values in tuples
were incorrectly filled in with NULL. The problem was the use of
CreateTupleDescCopy where CreateTupleDescCopyConstr was required, as the
former drops the constraints in the tuple description (specifically, the
default value constraint) on the floor.
The bug could result in incorrectness when a table replicated via
`REPLICA IDENTITY FULL` underwent a schema change that added a column
with a default value.
Author: Nikhil Benesch
Reviewed-by: Amit Kapila
Backpatch-through: 15
Discussion: http://postgr.es/m/CAPWqQZTEpZQamYsGMn6ZDRvVywwpVPiKH6OY4KSgA+NmeqFNzA@mail.gmail.com
---
src/backend/replication/pgoutput/pgoutput.c | 4 +-
src/test/subscription/t/100_bugs.pl | 55 +++++++++++++++++++++
2 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index e8add5ee5d..f9ed1083df 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1136,8 +1136,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
* Create tuple table slots. Create a copy of the TupleDesc as it needs to
* live as long as the cache remains.
*/
- oldtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
- newtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
+ oldtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
+ newtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
entry->old_slot = MakeSingleTupleTableSlot(oldtupdesc, &TTSOpsHeapTuple);
entry->new_slot = MakeSingleTupleTableSlot(newtupdesc, &TTSOpsHeapTuple);
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 7e5221afff..30f167cf6f 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -438,4 +438,59 @@ is( $node_subscriber->safe_psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that pgoutput was incorrectly replacing missing attributes in
+# tuples with NULL. This could result in incorrect replication with
+# `REPLICA IDENTITY FULL`.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+# Set up a table with schema `(a int, b bool)` where the `b` attribute is
+# missing for one row due to the `ALTER TABLE ... ADD COLUMN ... DEFAULT`
+# fast path.
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_default (a int)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE tab_default REPLICA IDENTITY FULL");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_default VALUES (1)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE tab_default ADD COLUMN b bool DEFAULT false NOT NULL");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_default VALUES (2, true)");
+
+# Replicate to the subscriber.
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_default (a int, b bool)");
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION pub1 FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1"
+);
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(1|f
+2|t), 'check snapshot on subscriber');
+
+# Update all rows in the table and ensure the rows with the missing `b`
+# attribute replicate correctly.
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_default SET a = a + 1");
+$node_publisher->wait_for_catchup('sub1');
+
+# When the bug is present, the `1|f` row will not be updated to `2|f` because
+# the publisher incorrectly fills in `NULL` for `b` and publishes an update
+# for `1|NULL`, which doesn't exist in the subscriber.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(2|f
+3|t), 'check replicated update on subscriber');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
+
done_testing();
--
2.28.0.windows.1
On Friday, November 24, 2023 7:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Nov 23, 2023 at 2:33 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:On Thu, Nov 23, 2023 at 1:10 PM Nikhil Benesch <nikhil.benesch@gmail.com>
wrote:
While working on Materialize's streaming logical replication from
Postgres [0], my colleagues Sean Loiselle and Petros Angelatos
(CC'd) discovered today what appears to be a correctness bug in pgoutput,introduced in v15.
The problem goes like this. A table with REPLICA IDENTITY FULL and
some data in it...CREATE TABLE t (a int);
ALTER TABLE t REPLICA IDENTITY FULL;
INSERT INTO t VALUES (1), (2), (3), ...;...undergoes a schema change to add a new column with a default:
ALTER TABLE t ADD COLUMN b bool DEFAULT false NOT NULL;
PostgreSQL is smart and does not rewrite the entire table during the
schema change. Instead it updates the tuple description to indicate
to future readers of the table that if `b` is missing, it should be
filled in with the default value, `false`.Unfortunately, since v15, pgoutput mishandles missing attributes. If
a downstream server is subscribed to changes from t via the pgoutput
plugin, when a row with a missing attribute is updated, e.g.:UPDATE t SET a = 2 WHERE a = 1
pgoutput willz incorrectly report b's value as NULL in the old tuple,
rather than false.Thanks, I could reproduce this behavior. I'll look into your patch.
I verified your fix is good and made minor modifications in the comment. Note,
that the test doesn't work for PG15, needs minor modifications.
Thank you for fixing and reviewing the fix!
The fix also looks good to me. I verified that it can fix the problem in
HEAD ~ PG15 and the added tap test can detect the problem without the fix. I
tried to rebase the patch on PG15, and combines some queries into one safe_sql
block to simplify the code. Here are the patches for all branches.
Best Regards,
Hou zj
Attachments:
v3-HEAD-PG16-0001-Avoid-unconditionally-filling-in-missing-values-w.patchapplication/octet-stream; name=v3-HEAD-PG16-0001-Avoid-unconditionally-filling-in-missing-values-w.patchDownload
From d8b89d44c1e67c348198bcda1d00b3a20d8eaa3b Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 24 Nov 2023 15:31:05 +0530
Subject: [PATCH v3] Avoid unconditionally filling in missing values with NULL
in pgoutput.
52e4f0cd4 introduced a bug in pgoutput in which missing values in tuples
were incorrectly filled in with NULL. The problem was the use of
CreateTupleDescCopy where CreateTupleDescCopyConstr was required, as the
former drops the constraints in the tuple description (specifically, the
default value constraint) on the floor.
The bug could result in incorrectness when a table replicated via
`REPLICA IDENTITY FULL` underwent a schema change that added a column
with a default value.
Author: Nikhil Benesch
Reviewed-by: Amit Kapila, Hou Zhijie
Backpatch-through: 15
Discussion: http://postgr.es/m/CAPWqQZTEpZQamYsGMn6ZDRvVywwpVPiKH6OY4KSgA+NmeqFNzA@mail.gmail.com
---
src/backend/replication/pgoutput/pgoutput.c | 4 +-
src/test/subscription/t/100_bugs.pl | 53 +++++++++++++++++++++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index e8add5ee5d..f9ed1083df 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1136,8 +1136,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
* Create tuple table slots. Create a copy of the TupleDesc as it needs to
* live as long as the cache remains.
*/
- oldtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
- newtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
+ oldtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
+ newtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
entry->old_slot = MakeSingleTupleTableSlot(oldtupdesc, &TTSOpsHeapTuple);
entry->new_slot = MakeSingleTupleTableSlot(newtupdesc, &TTSOpsHeapTuple);
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 7e5221afff..d64be621e8 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -438,4 +438,57 @@ is( $node_subscriber->safe_psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that pgoutput was incorrectly replacing missing attributes in
+# tuples with NULL. This could result in incorrect replication with
+# `REPLICA IDENTITY FULL`.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+# Set up a table with schema `(a int, b bool)` where the `b` attribute is
+# missing for one row due to the `ALTER TABLE ... ADD COLUMN ... DEFAULT`
+# fast path.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_default (a int);
+ ALTER TABLE tab_default REPLICA IDENTITY FULL;
+ INSERT INTO tab_default VALUES (1);
+ ALTER TABLE tab_default ADD COLUMN b bool DEFAULT false NOT NULL;
+ INSERT INTO tab_default VALUES (2, true);
+ CREATE PUBLICATION pub1 FOR TABLE tab_default;
+));
+
+# Replicate to the subscriber.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_default (a int, b bool);
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(1|f
+2|t), 'check snapshot on subscriber');
+
+# Update all rows in the table and ensure the rows with the missing `b`
+# attribute replicate correctly.
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_default SET a = a + 1");
+$node_publisher->wait_for_catchup('sub1');
+
+# When the bug is present, the `1|f` row will not be updated to `2|f` because
+# the publisher incorrectly fills in `NULL` for `b` and publishes an update
+# for `1|NULL`, which doesn't exist in the subscriber.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(2|f
+3|t), 'check replicated update on subscriber');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
+
done_testing();
--
2.31.1
v3-PG15-0001-Avoid-unconditionally-filling-in-missing-values-w.patchapplication/octet-stream; name=v3-PG15-0001-Avoid-unconditionally-filling-in-missing-values-w.patchDownload
From 55220ad2713227789a62768ddc3d8702855837b8 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Fri, 24 Nov 2023 20:02:31 +0800
Subject: [PATCH v3] Avoid unconditionally filling in missing values with NULL
in pgoutput.
52e4f0cd4 introduced a bug in pgoutput in which missing values in tuples
were incorrectly filled in with NULL. The problem was the use of
CreateTupleDescCopy where CreateTupleDescCopyConstr was required, as the
former drops the constraints in the tuple description (specifically, the
default value constraint) on the floor.
The bug could result in incorrectness when a table replicated via
`REPLICA IDENTITY FULL` underwent a schema change that added a column
with a default value.
Author: Nikhil Benesch
Reviewed-by: Amit Kapila, Hou Zhijie
Backpatch-through: 15
Discussion: http://postgr.es/m/CAPWqQZTEpZQamYsGMn6ZDRvVywwpVPiKH6OY4KSgA+NmeqFNzA@mail.gmail.com
---
src/backend/replication/pgoutput/pgoutput.c | 4 +-
src/test/subscription/t/100_bugs.pl | 53 +++++++++++++++++++++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 63a84424aa..5fc0defec3 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1095,8 +1095,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
* Create tuple table slots. Create a copy of the TupleDesc as it needs to
* live as long as the cache remains.
*/
- oldtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
- newtupdesc = CreateTupleDescCopy(RelationGetDescr(relation));
+ oldtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
+ newtupdesc = CreateTupleDescCopyConstr(RelationGetDescr(relation));
entry->old_slot = MakeSingleTupleTableSlot(oldtupdesc, &TTSOpsHeapTuple);
entry->new_slot = MakeSingleTupleTableSlot(newtupdesc, &TTSOpsHeapTuple);
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 61c9f538e3..28ca4affbb 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -365,4 +365,57 @@ is( $node_subscriber_d_cols->safe_psql(
$node_publisher_d_cols->stop('fast');
$node_subscriber_d_cols->stop('fast');
+# The bug was that pgoutput was incorrectly replacing missing attributes in
+# tuples with NULL. This could result in incorrect replication with
+# `REPLICA IDENTITY FULL`.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+# Set up a table with schema `(a int, b bool)` where the `b` attribute is
+# missing for one row due to the `ALTER TABLE ... ADD COLUMN ... DEFAULT`
+# fast path.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_default (a int);
+ ALTER TABLE tab_default REPLICA IDENTITY FULL;
+ INSERT INTO tab_default VALUES (1);
+ ALTER TABLE tab_default ADD COLUMN b bool DEFAULT false NOT NULL;
+ INSERT INTO tab_default VALUES (2, true);
+ CREATE PUBLICATION pub1 FOR TABLE tab_default;
+));
+
+# Replicate to the subscriber.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_default (a int, b bool);
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(1|f
+2|t), 'check snapshot on subscriber');
+
+# Update all rows in the table and ensure the rows with the missing `b`
+# attribute replicate correctly.
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_default SET a = a + 1");
+$node_publisher->wait_for_catchup('sub1');
+
+# When the bug is present, the `1|f` row will not be updated to `2|f` because
+# the publisher incorrectly fills in `NULL` for `b` and publishes an update
+# for `1|NULL`, which doesn't exist in the subscriber.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b FROM tab_default");
+is($result, qq(2|f
+3|t), 'check replicated update on subscriber');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
+
done_testing();
--
2.30.0.windows.2
Thank you both for reviewing. The updated patch set LGTM.
Nikhil
On Fri, Nov 24, 2023 at 7:21 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
Show quoted text
On Friday, November 24, 2023 7:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Nov 23, 2023 at 2:33 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:On Thu, Nov 23, 2023 at 1:10 PM Nikhil Benesch <nikhil.benesch@gmail.com>
wrote:
While working on Materialize's streaming logical replication from
Postgres [0], my colleagues Sean Loiselle and Petros Angelatos
(CC'd) discovered today what appears to be a correctness bug in pgoutput,introduced in v15.
The problem goes like this. A table with REPLICA IDENTITY FULL and
some data in it...CREATE TABLE t (a int);
ALTER TABLE t REPLICA IDENTITY FULL;
INSERT INTO t VALUES (1), (2), (3), ...;...undergoes a schema change to add a new column with a default:
ALTER TABLE t ADD COLUMN b bool DEFAULT false NOT NULL;
PostgreSQL is smart and does not rewrite the entire table during the
schema change. Instead it updates the tuple description to indicate
to future readers of the table that if `b` is missing, it should be
filled in with the default value, `false`.Unfortunately, since v15, pgoutput mishandles missing attributes. If
a downstream server is subscribed to changes from t via the pgoutput
plugin, when a row with a missing attribute is updated, e.g.:UPDATE t SET a = 2 WHERE a = 1
pgoutput willz incorrectly report b's value as NULL in the old tuple,
rather than false.Thanks, I could reproduce this behavior. I'll look into your patch.
I verified your fix is good and made minor modifications in the comment. Note,
that the test doesn't work for PG15, needs minor modifications.Thank you for fixing and reviewing the fix!
The fix also looks good to me. I verified that it can fix the problem in
HEAD ~ PG15 and the added tap test can detect the problem without the fix. I
tried to rebase the patch on PG15, and combines some queries into one safe_sql
block to simplify the code. Here are the patches for all branches.Best Regards,
Hou zj
On Fri, Nov 24, 2023 at 7:23 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
Thank you both for reviewing. The updated patch set LGTM.
Pushed!
--
With Regards,
Amit Kapila.