Hi Amit, Shi Yu,
> Generated column is introduced in PG12, and I reproduced generated column
problem
in PG12~PG15.
> For dropped column problem, I reproduced it in PG10~PG15. (Logical
replication
was introduced in PG10)
So, I'm planning to split the changes into two commits. The first one fixes
for dropped columns, and the second one adds generated columns check/test.
Is that the right approach for such a case?
> and backpatch the fix for dropped column to PG10.
Still, even the first commit fails to apply cleanly to PG12 (and below).
What is the procedure here? Should I be creating multiple patches per
version?
Or does the committer prefer to handle the conflicts? Depending on your
reply,
I can work on the followup.
I'm still attaching the dropped column patch for reference.
Thanks,
Onder
shiy.f...@fujitsu.com <shiy.f...@fujitsu.com>, 16 Mar 2023 Per, 13:38
tarihinde şunu yazdı:
> On Thu, Mar 16, 2023 5:23 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <onderkal...@gmail.com>
> > wrote:
> > >
> > > Attaching v2
> > >
> >
> > Can we change the comment to: "Ignore dropped and generated columns as
> > the publisher doesn't send those."? After your change, att =
> > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in
> > the same function.
> >
> > In test cases, let's change the comment to: "The bug was that when the
> > REPLICA IDENTITY FULL is used with dropped or generated columns, we
> > fail to apply updates and deletes.". Also, I think we don't need to
> > provide the email link as anyway commit message will have a link to
> > the discussion.
> >
> > Did you check this in the back branches?
> >
>
> I tried to reproduce this bug in backbranch.
>
> Generated column is introduced in PG12, and I reproduced generated column
> problem
> in PG12~PG15.
>
> For dropped column problem, I reproduced it in PG10~PG15. (Logical
> replication
> was introduced in PG10)
>
> So I think we should backpatch the fix for generated column to PG12, and
> backpatch the fix for dropped column to PG10.
>
> Regards,
> Shi Yu
>
From d25b2feac3b26646034bf3ace6ab93056c3c29af Mon Sep 17 00:00:00 2001
From: Onder Kalaci <onderkalaci@gmail.com>
Date: Thu, 16 Mar 2023 18:06:54 +0300
Subject: [PATCH v1 1/2] Ignore dropped columns when REPLICA IDENTITY FULL
Dropped columns are filled with NULL values on slot_store_data()
but not on table_scan_getnextslot(). With this commit, we skip
such columns while checking tuple equality.
---
src/backend/executor/execReplication.c | 9 ++++-
src/test/subscription/t/100_bugs.pl | 51 ++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index fa8628e3e1..7c8b0d2667 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -289,6 +289,13 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
Form_pg_attribute att;
TypeCacheEntry *typentry;
+ /*
+ * Ignore dropped columns as the publisher doesn't send those
+ */
+ att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
+ if (att->attisdropped)
+ continue;
+
/*
* If one value is NULL and other is not, then they are certainly not
* equal
@@ -302,8 +309,6 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
if (slot1->tts_isnull[attrnum] || slot2->tts_isnull[attrnum])
continue;
- att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
-
typentry = eq[attrnum];
if (typentry == NULL)
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 036576acab..255746094c 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -373,4 +373,55 @@ is( $result, qq(1
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that when the REPLICA IDENTITY FULL is used with dropped columns,
+# we fail to apply updates and deletes
+my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols');
+$node_publisher_d_cols->init(allows_streaming => 'logical');
+$node_publisher_d_cols->start;
+
+my $node_subscriber_d_cols = PostgreSQL::Test::Cluster->new('node_subscriber_d_cols');
+$node_subscriber_d_cols->init(allows_streaming => 'logical');
+$node_subscriber_d_cols->start;
+
+$node_publisher_d_cols->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE dropped_cols (a int, b_drop int, c int);
+ ALTER TABLE dropped_cols REPLICA IDENTITY FULL;
+ CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols;
+ -- some initial data
+ INSERT INTO dropped_cols VALUES (1, 1, 1);
+));
+
+$node_subscriber_d_cols->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE dropped_cols (a int, b_drop int, c int);
+));
+
+my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres';
+$node_subscriber_d_cols->safe_psql('postgres',
+ "CREATE SUBSCRIPTION sub_dropped_cols CONNECTION '$publisher_connstr_d_cols' PUBLICATION pub_dropped_cols"
+);
+$node_subscriber_d_cols->wait_for_subscription_sync;
+
+$node_publisher_d_cols->safe_psql(
+ 'postgres', qq(
+ ALTER TABLE dropped_cols DROP COLUMN b_drop;
+));
+$node_subscriber_d_cols->safe_psql(
+ 'postgres', qq(
+ ALTER TABLE dropped_cols DROP COLUMN b_drop;
+));
+
+$node_publisher_d_cols->safe_psql(
+ 'postgres', qq(
+ UPDATE dropped_cols SET a = 100;
+));
+$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols');
+
+is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"),
+ qq(1), 'replication with RI FULL and dropped columns');
+
+$node_publisher_d_cols->stop('fast');
+$node_subscriber_d_cols->stop('fast');
+
done_testing();
--
2.34.1
From b84f38deac1b02f057bfc05ff868fbf4a621b18b Mon Sep 17 00:00:00 2001
From: Onder Kalaci <onderkalaci@gmail.com>
Date: Thu, 16 Mar 2023 18:18:37 +0300
Subject: [PATCH v1 2/2] Ignore generated columns when REPLICA IDENTITY FULL
Generated columns are filled with NULL values on slot_store_data()
but not on table_scan_getnextslot(). With this commit, we skip
such columns while checking tuple equality.
---
src/backend/executor/execReplication.c | 5 +++--
src/test/subscription/t/100_bugs.pl | 10 +++++++++-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 7c8b0d2667..482806961a 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -290,10 +290,11 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
TypeCacheEntry *typentry;
/*
- * Ignore dropped columns as the publisher doesn't send those
+ * Ignore dropped and generated columns as the publisher doesn't
+ * send those
*/
att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
- if (att->attisdropped)
+ if (att->attisdropped || att->attgenerated)
continue;
/*
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 255746094c..96126428f3 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -387,14 +387,18 @@ $node_publisher_d_cols->safe_psql(
'postgres', qq(
CREATE TABLE dropped_cols (a int, b_drop int, c int);
ALTER TABLE dropped_cols REPLICA IDENTITY FULL;
- CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols;
+ CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int);
+ ALTER TABLE generated_cols REPLICA IDENTITY FULL;
+ CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols;
-- some initial data
INSERT INTO dropped_cols VALUES (1, 1, 1);
+ INSERT INTO generated_cols (a,c) VALUES (1, 1);
));
$node_subscriber_d_cols->safe_psql(
'postgres', qq(
CREATE TABLE dropped_cols (a int, b_drop int, c int);
+ CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int);
));
my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres';
@@ -415,12 +419,16 @@ $node_subscriber_d_cols->safe_psql(
$node_publisher_d_cols->safe_psql(
'postgres', qq(
UPDATE dropped_cols SET a = 100;
+ UPDATE generated_cols SET a = 100;
));
$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols');
is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"),
qq(1), 'replication with RI FULL and dropped columns');
+is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM generated_cols WHERE a = 100"),
+ qq(1), 'replication with RI FULL and generated columns');
+
$node_publisher_d_cols->stop('fast');
$node_subscriber_d_cols->stop('fast');
--
2.34.1
