deferred primary key and logical replication
Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a
table
with a deferred primary key is not updatable in logical replication. AFAICS
it
has been like that since the beginning of logical decoding and seems to be
an
oversight while designing logical decoding. I don't envision a problem with
a
deferred primary key in an after commit scenario. Am I missing something?
Just in case, I'm attaching a patch to fix it and also add a test to cover
this
scenario.
--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Table-with-deferred-PK-is-not-updatable-in-logical-r.patchtext/x-patch; charset=US-ASCII; name=0001-Table-with-deferred-PK-is-not-updatable-in-logical-r.patchDownload
From 746385e6adaa1668f3be7c7d037e3868ecadae60 Mon Sep 17 00:00:00 2001
From: Euler Taveira <euler.taveira@2ndquadrant.com>
Date: Sun, 19 Apr 2020 20:04:39 -0300
Subject: [PATCH] Table with deferred PK is not updatable in logical
replication
In logical replication, an UPDATE or DELETE cannot be executed if a
table has a primary key that is marked as deferred. RelationGetIndexList
does not fill rd_replidindex accordingly. The consequence is that UPDATE
or DELETE cannot be executed in a deferred PK table. Deferrable primary
key cannot prevent a primary key to be used as replica identity.
---
src/backend/utils/cache/relcache.c | 7 +++----
src/test/subscription/t/001_rep_changes.pl | 21 +++++++++++++++++++--
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9f1f11d0c1..fb35af4962 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4555,12 +4555,11 @@ RelationGetIndexList(Relation relation)
result = lappend_oid(result, index->indexrelid);
/*
- * Invalid, non-unique, non-immediate or predicate indexes aren't
- * interesting for either oid indexes or replication identity indexes,
- * so don't check them.
+ * Invalid, non-unique or predicate indexes aren't interesting for
+ * either oid indexes or replication identity indexes, so don't check
+ * them.
*/
if (!index->indisvalid || !index->indisunique ||
- !index->indimmediate ||
!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
continue;
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 6da7f71ca3..eacdf2aa00 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 24;
# Initialize publisher node
my $node_publisher = get_new_node('publisher');
@@ -34,6 +34,8 @@ $node_publisher->safe_psql('postgres',
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab_include (a int, b text, CONSTRAINT covering PRIMARY KEY(a) INCLUDE(b))"
);
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_deferred_pk (a int PRIMARY KEY DEFERRABLE)");
# Let this table with REPLICA IDENTITY NOTHING, allowing only INSERT changes.
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_nothing (a int)");
$node_publisher->safe_psql('postgres',
@@ -58,13 +60,17 @@ $node_subscriber->safe_psql('postgres',
"CREATE TABLE tab_include (a int, b text, CONSTRAINT covering PRIMARY KEY(a) INCLUDE(b))"
);
+# replication of the table with deferrable primary key
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_deferred_pk (a int PRIMARY KEY DEFERRABLE)");
+
# Setup logical replication
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
$node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub");
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub_ins_only WITH (publish = insert)");
$node_publisher->safe_psql('postgres',
- "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_deferred_pk"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -111,6 +117,13 @@ $node_publisher->safe_psql('postgres',
"DELETE FROM tab_include WHERE a > 20");
$node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_deferred_pk VALUES (1),(2),(3)");
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_deferred_pk SET a = 11 WHERE a = 1");
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM tab_deferred_pk WHERE a = 2");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -134,6 +147,10 @@ $result = $node_subscriber->safe_psql('postgres',
is($result, qq(20|-20|-1),
'check replicated changes with primary key index with included columns');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*), min(a), max(a) FROM tab_deferred_pk");
+is($result, qq(2|3|11), 'check replicated changes with deferred primary key');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.20.1
The patch no longer applies, because of additions in the test source. Otherwise, I have tested the patch and confirmed that updates and deletes on tables with deferred primary keys work with logical replication.
The new status of this patch is: Waiting on Author
On Fri, 24 Jul 2020 at 05:16, Ajin Cherian <itsajin@gmail.com> wrote:
The patch no longer applies, because of additions in the test source.
Otherwise, I have tested the patch and confirmed that updates and deletes
on tables with deferred primary keys work with logical replication.The new status of this patch is: Waiting on Author
Thanks for testing. I attached a rebased patch.
--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Table-with-deferred-PK-is-not-updatable-in-logical-r.patchtext/x-patch; charset=US-ASCII; name=0001-Table-with-deferred-PK-is-not-updatable-in-logical-r.patchDownload
From c1ad1581962a56c83183bec1501df6f54406db89 Mon Sep 17 00:00:00 2001
From: Euler Taveira <euler.taveira@2ndquadrant.com>
Date: Sun, 19 Apr 2020 20:04:39 -0300
Subject: [PATCH] Table with deferred PK is not updatable in logical
replication
In logical replication, an UPDATE or DELETE cannot be executed if a
table has a primary key that is marked as deferred. RelationGetIndexList
does not fill rd_replidindex accordingly. The consequence is that UPDATE
or DELETE cannot be executed in a deferred PK table. Deferrable primary
key cannot prevent a primary key to be used as replica identity.
---
src/backend/utils/cache/relcache.c | 7 +++----
src/test/subscription/t/001_rep_changes.pl | 21 +++++++++++++++++++--
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index a2453cf1f4..8996279f3b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4556,12 +4556,11 @@ RelationGetIndexList(Relation relation)
result = lappend_oid(result, index->indexrelid);
/*
- * Invalid, non-unique, non-immediate or predicate indexes aren't
- * interesting for either oid indexes or replication identity indexes,
- * so don't check them.
+ * Invalid, non-unique or predicate indexes aren't interesting for
+ * either oid indexes or replication identity indexes, so don't check
+ * them.
*/
if (!index->indisvalid || !index->indisunique ||
- !index->indimmediate ||
!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
continue;
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 3f8318fc7c..f164d23a94 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 24;
# Initialize publisher node
my $node_publisher = get_new_node('publisher');
@@ -38,6 +38,8 @@ $node_publisher->safe_psql('postgres',
"CREATE TABLE tab_full_pk (a int primary key, b text)");
$node_publisher->safe_psql('postgres',
"ALTER TABLE tab_full_pk REPLICA IDENTITY FULL");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_deferred_pk (a int PRIMARY KEY DEFERRABLE)");
# Let this table with REPLICA IDENTITY NOTHING, allowing only INSERT changes.
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_nothing (a int)");
$node_publisher->safe_psql('postgres',
@@ -66,13 +68,17 @@ $node_subscriber->safe_psql('postgres',
"CREATE TABLE tab_include (a int, b text, CONSTRAINT covering PRIMARY KEY(a) INCLUDE(b))"
);
+# replication of the table with deferrable primary key
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_deferred_pk (a int PRIMARY KEY DEFERRABLE)");
+
# Setup logical replication
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
$node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub");
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub_ins_only WITH (publish = insert)");
$node_publisher->safe_psql('postgres',
- "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk, tab_deferred_pk"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -122,6 +128,13 @@ $node_publisher->safe_psql('postgres',
"DELETE FROM tab_include WHERE a > 20");
$node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_deferred_pk VALUES (1),(2),(3)");
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_deferred_pk SET a = 11 WHERE a = 1");
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM tab_deferred_pk WHERE a = 2");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -145,6 +158,10 @@ $result = $node_subscriber->safe_psql('postgres',
is($result, qq(20|-20|-1),
'check replicated changes with primary key index with included columns');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*), min(a), max(a) FROM tab_deferred_pk");
+is($result, qq(2|3|11), 'check replicated changes with deferred primary key');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.20.1
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Patch applies cleanly. Tested that update/delete of tables with deferred primary keys now work with logical replication. Code/comments look fine.
The new status of this patch is: Ready for Committer
On Mon, May 11, 2020 at 2:41 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:
Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a table
with a deferred primary key is not updatable in logical replication. AFAICS it
has been like that since the beginning of logical decoding and seems to be an
oversight while designing logical decoding.
I am not sure if it is an oversight because we document that the index
must be non-deferrable, see "USING INDEX records the old values of the
columns covered by the named index, which must be unique, not partial,
not deferrable, and include only columns marked NOT NULL." in docs
[1]: https://www.postgresql.org/docs/devel/sql-altertable.html
Now sure this constraint is when we use USING INDEX for REPLICA
IDENTITY but why it has to be different for PRIMARY KEY especially
when UNIQUE constraint will have similar behavior and the same is
documented?
[1]: https://www.postgresql.org/docs/devel/sql-altertable.html
--
With Regards,
Amit Kapila.
On Mon, 5 Oct 2020 at 08:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, May 11, 2020 at 2:41 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a
table
with a deferred primary key is not updatable in logical replication.
AFAICS it
has been like that since the beginning of logical decoding and seems to
be an
oversight while designing logical decoding.
I am not sure if it is an oversight because we document that the index
must be non-deferrable, see "USING INDEX records the old values of the
columns covered by the named index, which must be unique, not partial,
not deferrable, and include only columns marked NOT NULL." in docs
[1].
Inspecting this patch again, I forgot to consider
that RelationGetIndexList()
is called by other backend modules. Since logical decoding deals with
finished
transactions, it is ok to use a deferrable primary key. However, this patch
is
probably wrong because it does not consider the other modules.
--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Oct 25, 2020 at 9:39 PM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:
On Mon, 5 Oct 2020 at 08:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, May 11, 2020 at 2:41 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a table
with a deferred primary key is not updatable in logical replication. AFAICS it
has been like that since the beginning of logical decoding and seems to be an
oversight while designing logical decoding.I am not sure if it is an oversight because we document that the index
must be non-deferrable, see "USING INDEX records the old values of the
columns covered by the named index, which must be unique, not partial,
not deferrable, and include only columns marked NOT NULL." in docs
[1].Inspecting this patch again, I forgot to consider that RelationGetIndexList()
is called by other backend modules. Since logical decoding deals with finished
transactions, it is ok to use a deferrable primary key.
But starting PG-14, we do support logical decoding of in-progress
transactions as well.
--
With Regards,
Amit Kapila.
On 27.10.2020 13:46, Amit Kapila wrote:
On Sun, Oct 25, 2020 at 9:39 PM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:On Mon, 5 Oct 2020 at 08:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, May 11, 2020 at 2:41 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a table
with a deferred primary key is not updatable in logical replication. AFAICS it
has been like that since the beginning of logical decoding and seems to be an
oversight while designing logical decoding.I am not sure if it is an oversight because we document that the index
must be non-deferrable, see "USING INDEX records the old values of the
columns covered by the named index, which must be unique, not partial,
not deferrable, and include only columns marked NOT NULL." in docs
[1].Inspecting this patch again, I forgot to consider that RelationGetIndexList()
is called by other backend modules. Since logical decoding deals with finished
transactions, it is ok to use a deferrable primary key.But starting PG-14, we do support logical decoding of in-progress
transactions as well.
Commitfest entry status update.
As far as I see, this patch needs some further work, so I move it to
"Waiting on author".
Euler, are you going to continue working on it?
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Tue, Nov 24, 2020 at 3:04 AM Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
On 27.10.2020 13:46, Amit Kapila wrote:
On Sun, Oct 25, 2020 at 9:39 PM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:On Mon, 5 Oct 2020 at 08:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, May 11, 2020 at 2:41 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:Hi,
While looking at an old wal2json issue, I stumbled on a scenario that a table
with a deferred primary key is not updatable in logical replication. AFAICS it
has been like that since the beginning of logical decoding and seems to be an
oversight while designing logical decoding.I am not sure if it is an oversight because we document that the index
must be non-deferrable, see "USING INDEX records the old values of the
columns covered by the named index, which must be unique, not partial,
not deferrable, and include only columns marked NOT NULL." in docs
[1].Inspecting this patch again, I forgot to consider that RelationGetIndexList()
is called by other backend modules. Since logical decoding deals with finished
transactions, it is ok to use a deferrable primary key.But starting PG-14, we do support logical decoding of in-progress
transactions as well.Commitfest entry status update.
As far as I see, this patch needs some further work, so I move it to
"Waiting on author".
I think this should be marked as "Returned with Feedback" as there is
no response to the feedback for a long time and also it is not very
clear if this possible.
--
With Regards,
Amit Kapila.