pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18203: Logical Replication initial sync failure
8+ messages / 4 participants
[nested] [flat]
* BUG #18203: Logical Replication initial sync failure
@ 2023-11-18 01:25 PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: PG Bug reporting form @ 2023-11-18 01:25 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: zzzzz.graf@gmail.com
The following bug has been logged on the website:
Bug reference: 18203
Logged by: Justin G
Email address: zzzzz.graf@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 20
Description:
We’ve found an edge case that breaks logical replication.
Procedure:
alter system set wal_level to 'logical' ;
create database source; --pg 11
create database destin; --pg 15
create table no_col(); -- pg 11
insert into no_col default values;
insert into no_col default values;
insert into no_col default values;
insert into no_col default values;
insert into no_col default values;
CREATE PUBLICATION no_col_pub for table no_col;
create table no_col(); --pg 15
CREATE SUBSCRIPTION no_col_sub --pg15
CONNECTION
'host=<hostname/ip>
port=5432
user=<set username>
dbname=source
password=<set the password>'
PUBLICATION no_col_pub
WITH (
CREATE_SLOT = TRUE,
ENABLED = TRUE,
COPY_DATA = TRUE
);
We now have a table with 5 null rows. This is a valid table, pg_dump will
dump it and restore it. count(*) will return 5.
The dump file produced by pg_dump version 15 generates this SQL
COPY public.no_col FROM stdin;
However, the copy command from PG 15 using logical replication includes (),
which normally has a list of columns between the parentheses.
COPY public.no_col () TO STDOUT
Logical replication worker returns with the following error:
2023-11-17 20:40:16.141 UTC [151084] ERROR: could not start initial
contents copy for table "public.no_col": ERROR: syntax error at or near
")"
LINE 1: COPY public.no_col () TO STDOUT
If the direction of replication is reversed from PG15 to PG14, the above
error is not thrown, and the 5 rows are copied over.
Version tested
Publishers PG 11.14, 14.2, 15.2
Subscriber PG 14.2, 15.2, , RDS 14.7
The error has been observed in 15.2 and in RDS PostgreSQL 14.7.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
@ 2023-11-20 09:23 ` vignesh C <vignesh21@gmail.com>
2023-11-20 13:08 ` Re: BUG #18203: Logical Replication initial sync failure Amit Kapila <amit.kapila16@gmail.com>
2023-11-20 22:07 ` Re: BUG #18203: Logical Replication initial sync failure Peter Smith <smithpb2250@gmail.com>
0 siblings, 2 replies; 8+ messages in thread
From: vignesh C @ 2023-11-20 09:23 UTC (permalink / raw)
To: zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, 18 Nov 2023 at 18:10, PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 18203
> Logged by: Justin G
> Email address: zzzzz.graf@gmail.com
> PostgreSQL version: 15.2
> Operating system: Ubuntu 20
> Description:
>
> We’ve found an edge case that breaks logical replication.
>
> Procedure:
>
> alter system set wal_level to 'logical' ;
>
> create database source; --pg 11
> create database destin; --pg 15
>
> create table no_col(); -- pg 11
>
> insert into no_col default values;
> insert into no_col default values;
> insert into no_col default values;
> insert into no_col default values;
> insert into no_col default values;
>
> CREATE PUBLICATION no_col_pub for table no_col;
>
> create table no_col(); --pg 15
>
> CREATE SUBSCRIPTION no_col_sub --pg15
> CONNECTION
> 'host=<hostname/ip>
> port=5432
> user=<set username>
> dbname=source
> password=<set the password>'
> PUBLICATION no_col_pub
> WITH (
> CREATE_SLOT = TRUE,
> ENABLED = TRUE,
> COPY_DATA = TRUE
> );
>
>
> We now have a table with 5 null rows. This is a valid table, pg_dump will
> dump it and restore it. count(*) will return 5.
>
> The dump file produced by pg_dump version 15 generates this SQL
> COPY public.no_col FROM stdin;
>
> However, the copy command from PG 15 using logical replication includes (),
> which normally has a list of columns between the parentheses.
>
> COPY public.no_col () TO STDOUT
>
> Logical replication worker returns with the following error:
> 2023-11-17 20:40:16.141 UTC [151084] ERROR: could not start initial
> contents copy for table "public.no_col": ERROR: syntax error at or near
> ")"
> LINE 1: COPY public.no_col () TO STDOUT
Thanks for reporting this issue, I was able to reproduce the issue.
This issue is happening because we are trying to specify the column
list while the table sync worker is copying data for the table.
I felt we should not specify the column list when the table has no columns.
Attached patch has the changes to handle the same.
Regards,
Vignesh
Attachments:
[text/x-patch] v1-0001-Specify-parenthesis-for-tablesync-worker-copy-com.patch (1.9K, ../../CALDaNm1JKqMXWjDVUnzJS4DOb7YPLqrsjDj4oQiqAakM0FEvBA@mail.gmail.com/2-v1-0001-Specify-parenthesis-for-tablesync-worker-copy-com.patch)
download | inline diff:
From 2e2817a00adc045a4b67f144c2167ee7ad8c2b46 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 20 Nov 2023 14:40:22 +0530
Subject: [PATCH v1] Specify parenthesis for tablesync worker copy command only
if the table has some column.
Copy command was specifying parenthesis for tables have no columns too,
fixed it by specifying the parenthesis and columns only if table has
some columns.
---
src/backend/replication/logical/tablesync.c | 28 +++++++++++++--------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 37a0abe2f4..5172f3e7f8 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1124,22 +1124,30 @@ copy_table(Relation rel)
/* Regular table with no row filter */
if (lrel.relkind == RELKIND_RELATION && qual == NIL)
{
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s ",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, "(");
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ /*
+ * XXX Do we need to list the columns in all cases? Maybe we're
+ * replicating all columns?
+ */
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ if (i > 0)
+ appendStringInfoString(&cmd, ", ");
+
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ }
+
+ appendStringInfoString(&cmd, ")");
}
- appendStringInfoString(&cmd, ") TO STDOUT");
+ appendStringInfoString(&cmd, " TO STDOUT");
}
else
{
--
2.34.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
@ 2023-11-20 13:08 ` Amit Kapila <amit.kapila16@gmail.com>
2023-11-21 05:28 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Amit Kapila @ 2023-11-20 13:08 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Nov 20, 2023 at 2:53 PM vignesh C <vignesh21@gmail.com> wrote:
>
> >
> > Logical replication worker returns with the following error:
> > 2023-11-17 20:40:16.141 UTC [151084] ERROR: could not start initial
> > contents copy for table "public.no_col": ERROR: syntax error at or near
> > ")"
> > LINE 1: COPY public.no_col () TO STDOUT
>
> Thanks for reporting this issue, I was able to reproduce the issue.
> This issue is happening because we are trying to specify the column
> list while the table sync worker is copying data for the table.
> I felt we should not specify the column list when the table has no columns.
> Attached patch has the changes to handle the same.
>
I think we can add a test for such cases which tests both initial sync
and regular replication on a table with no column.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
2023-11-20 13:08 ` Re: BUG #18203: Logical Replication initial sync failure Amit Kapila <amit.kapila16@gmail.com>
@ 2023-11-21 05:28 ` vignesh C <vignesh21@gmail.com>
2023-11-21 08:00 ` Re: BUG #18203: Logical Replication initial sync failure Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: vignesh C @ 2023-11-21 05:28 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, 20 Nov 2023 at 18:38, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Nov 20, 2023 at 2:53 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > >
> > > Logical replication worker returns with the following error:
> > > 2023-11-17 20:40:16.141 UTC [151084] ERROR: could not start initial
> > > contents copy for table "public.no_col": ERROR: syntax error at or near
> > > ")"
> > > LINE 1: COPY public.no_col () TO STDOUT
> >
> > Thanks for reporting this issue, I was able to reproduce the issue.
> > This issue is happening because we are trying to specify the column
> > list while the table sync worker is copying data for the table.
> > I felt we should not specify the column list when the table has no columns.
> > Attached patch has the changes to handle the same.
> >
>
> I think we can add a test for such cases which tests both initial sync
> and regular replication on a table with no column.
I have added a test for the same and tested upto PG15 where the
changes were added.
The patches for the same are attached.
Regards,
Vignesh
Attachments:
[text/x-patch] v2-0001-Specify-parenthesis-for-tablesync-worker-copy-com.patch (4.4K, ../../CALDaNm2kSJCVree6R1iKxjbA=Sg4raFi3Fi_+wfF2xn2roxqCg@mail.gmail.com/2-v2-0001-Specify-parenthesis-for-tablesync-worker-copy-com.patch)
download | inline diff:
From 982e7bbd9651781c46f704ce54f723f5e62b5156 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 20 Nov 2023 14:40:22 +0530
Subject: [PATCH v2] Specify parenthesis for tablesync worker copy command only
if the table has some column.
Copy command was specifying parenthesis for tables have no columns too,
fixed it by specifying the parenthesis and columns only if table has
some columns.
---
src/backend/replication/logical/tablesync.c | 28 +++++++++++++--------
src/test/subscription/t/001_rep_changes.pl | 17 ++++++++++++-
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 37a0abe2f4..87785c6202 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1124,22 +1124,30 @@ copy_table(Relation rel)
/* Regular table with no row filter */
if (lrel.relkind == RELKIND_RELATION && qual == NIL)
{
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, " (");
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ /*
+ * XXX Do we need to list the columns in all cases? Maybe we're
+ * replicating all columns?
+ */
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ if (i > 0)
+ appendStringInfoString(&cmd, ", ");
+
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ }
+
+ appendStringInfoString(&cmd, ")");
}
- appendStringInfoString(&cmd, ") TO STDOUT");
+ appendStringInfoString(&cmd, " TO STDOUT");
}
else
{
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 87d44f634f..e49da00b29 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -57,6 +57,11 @@ $node_publisher->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_notrep (a int)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
@@ -87,13 +92,16 @@ $node_subscriber->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# replication of the table without columns
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+
# 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, tab_no_replidentity_index"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk, tab_no_replidentity_index, tab_no_col"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -141,6 +149,9 @@ $node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_no_replidentity_index VALUES(1)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -169,6 +180,10 @@ is( $node_subscriber->safe_psql(
1,
"value replicated to subscriber without replica identity index");
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_no_col");
+is($result, qq(2), 'check replicated changes for table having no columns');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.34.1
[text/x-patch] v2-0001-Specify-parenthesis-for-tablesync-worker-copy-com_PG15.patch (4.4K, ../../CALDaNm2kSJCVree6R1iKxjbA=Sg4raFi3Fi_+wfF2xn2roxqCg@mail.gmail.com/3-v2-0001-Specify-parenthesis-for-tablesync-worker-copy-com_PG15.patch)
download | inline diff:
From 288a5daedd0f285e57dc6995f7319d1a677fd5f8 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Tue, 21 Nov 2023 10:12:25 +0530
Subject: [PATCH v2] Specify parenthesis for tablesync worker copy command only
if the table has some column.
Copy command was specifying parenthesis for tables have no columns too,
fixed it by specifying the parenthesis and columns only if table has
some columns.
---
src/backend/replication/logical/tablesync.c | 28 +++++++++++++--------
src/test/subscription/t/001_rep_changes.pl | 17 ++++++++++++-
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index b7b933d468..013ce71f99 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1043,22 +1043,30 @@ copy_table(Relation rel)
/* Regular table with no row filter */
if (lrel.relkind == RELKIND_RELATION && qual == NIL)
{
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, " (");
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ /*
+ * XXX Do we need to list the columns in all cases? Maybe we're
+ * replicating all columns?
+ */
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ if (i > 0)
+ appendStringInfoString(&cmd, ", ");
+
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ }
+
+ appendStringInfoString(&cmd, ")");
}
- appendStringInfo(&cmd, ") TO STDOUT");
+ appendStringInfo(&cmd, " TO STDOUT");
}
else
{
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 6ed9265b2b..6e6ebd80e1 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -57,6 +57,11 @@ $node_publisher->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_notrep (a int)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
@@ -87,13 +92,16 @@ $node_subscriber->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# replication of the table without columns
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+
# 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, tab_no_replidentity_index"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk, tab_no_replidentity_index, tab_no_col"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -141,6 +149,9 @@ $node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_no_replidentity_index VALUES(1)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -169,6 +180,10 @@ is( $node_subscriber->safe_psql(
1,
"value replicated to subscriber without replica identity index");
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_no_col");
+is($result, qq(2), 'check replicated changes for table having no columns');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.34.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
2023-11-20 13:08 ` Re: BUG #18203: Logical Replication initial sync failure Amit Kapila <amit.kapila16@gmail.com>
2023-11-21 05:28 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
@ 2023-11-21 08:00 ` Peter Smith <smithpb2250@gmail.com>
2023-11-21 18:02 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Smith @ 2023-11-21 08:00 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
The v2 patch LGTM except for the commit message.
a) wording - "for tables have no columns too"
b) the plural of "parenthesis" is "parentheses"
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
2023-11-20 13:08 ` Re: BUG #18203: Logical Replication initial sync failure Amit Kapila <amit.kapila16@gmail.com>
2023-11-21 05:28 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
2023-11-21 08:00 ` Re: BUG #18203: Logical Replication initial sync failure Peter Smith <smithpb2250@gmail.com>
@ 2023-11-21 18:02 ` vignesh C <vignesh21@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: vignesh C @ 2023-11-21 18:02 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, 21 Nov 2023 at 13:31, Peter Smith <smithpb2250@gmail.com> wrote:
>
> The v2 patch LGTM except for the commit message.
>
> a) wording - "for tables have no columns too"
> b) the plural of "parenthesis" is "parentheses"
Thanks, these are fixed in the attached v3 version patch.
Regards,
Vignesh
Attachments:
[text/x-patch] v3-0001-Specify-parentheses-for-tablesync-worker-copy-com.patch (4.4K, ../../CALDaNm13V3nN5jCO-rr7sZEntJ7NWAsPAA46rskazBPOcCL=ng@mail.gmail.com/2-v3-0001-Specify-parentheses-for-tablesync-worker-copy-com.patch)
download | inline diff:
From 1cb78809a245edc7831078348243356d997cbdda Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 20 Nov 2023 14:40:22 +0530
Subject: [PATCH v3] Specify parentheses for tablesync worker copy command only
if the table has some column.
Copy command was specifying parentheses for tables having no columns too,
fixed it by specifying the parentheses and columns only if table has
some columns.
---
src/backend/replication/logical/tablesync.c | 28 +++++++++++++--------
src/test/subscription/t/001_rep_changes.pl | 17 ++++++++++++-
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 37a0abe2f4..87785c6202 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1124,22 +1124,30 @@ copy_table(Relation rel)
/* Regular table with no row filter */
if (lrel.relkind == RELKIND_RELATION && qual == NIL)
{
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, " (");
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ /*
+ * XXX Do we need to list the columns in all cases? Maybe we're
+ * replicating all columns?
+ */
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ if (i > 0)
+ appendStringInfoString(&cmd, ", ");
+
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ }
+
+ appendStringInfoString(&cmd, ")");
}
- appendStringInfoString(&cmd, ") TO STDOUT");
+ appendStringInfoString(&cmd, " TO STDOUT");
}
else
{
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 87d44f634f..e49da00b29 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -57,6 +57,11 @@ $node_publisher->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_notrep (a int)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
@@ -87,13 +92,16 @@ $node_subscriber->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# replication of the table without columns
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+
# 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, tab_no_replidentity_index"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk, tab_no_replidentity_index, tab_no_col"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -141,6 +149,9 @@ $node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_no_replidentity_index VALUES(1)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -169,6 +180,10 @@ is( $node_subscriber->safe_psql(
1,
"value replicated to subscriber without replica identity index");
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_no_col");
+is($result, qq(2), 'check replicated changes for table having no columns');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.34.1
[text/x-patch] v3-0001-Specify-parentheses-for-tablesync-worker-copy-com_PG15.patch (4.4K, ../../CALDaNm13V3nN5jCO-rr7sZEntJ7NWAsPAA46rskazBPOcCL=ng@mail.gmail.com/3-v3-0001-Specify-parentheses-for-tablesync-worker-copy-com_PG15.patch)
download | inline diff:
From f8b5649dd70a92982a2590e9dcee8bdebbb581a1 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Tue, 21 Nov 2023 10:12:25 +0530
Subject: [PATCH v3] Specify parentheses for tablesync worker copy command only
if the table has some column.
Copy command was specifying parentheses for tables having no columns too,
fixed it by specifying the parentheses and columns only if table has
some columns.
---
src/backend/replication/logical/tablesync.c | 28 +++++++++++++--------
src/test/subscription/t/001_rep_changes.pl | 17 ++++++++++++-
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index b7b933d468..013ce71f99 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1043,22 +1043,30 @@ copy_table(Relation rel)
/* Regular table with no row filter */
if (lrel.relkind == RELKIND_RELATION && qual == NIL)
{
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, " (");
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ /*
+ * XXX Do we need to list the columns in all cases? Maybe we're
+ * replicating all columns?
+ */
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ if (i > 0)
+ appendStringInfoString(&cmd, ", ");
+
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ }
+
+ appendStringInfoString(&cmd, ")");
}
- appendStringInfo(&cmd, ") TO STDOUT");
+ appendStringInfo(&cmd, " TO STDOUT");
}
else
{
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 6ed9265b2b..6e6ebd80e1 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -57,6 +57,11 @@ $node_publisher->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_notrep (a int)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
@@ -87,13 +92,16 @@ $node_subscriber->safe_psql('postgres',
"CREATE INDEX idx_no_replidentity_index ON tab_no_replidentity_index(c1)"
);
+# replication of the table without columns
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+
# 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, tab_no_replidentity_index"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk, tab_no_replidentity_index, tab_no_col"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -141,6 +149,9 @@ $node_publisher->safe_psql('postgres', "UPDATE tab_include SET a = -a");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_no_replidentity_index VALUES(1)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
+
$node_publisher->wait_for_catchup('tap_sub');
$result = $node_subscriber->safe_psql('postgres',
@@ -169,6 +180,10 @@ is( $node_subscriber->safe_psql(
1,
"value replicated to subscriber without replica identity index");
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_no_col");
+is($result, qq(2), 'check replicated changes for table having no columns');
+
# insert some duplicate rows
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_full SELECT generate_series(1,10)");
--
2.34.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
@ 2023-11-20 22:07 ` Peter Smith <smithpb2250@gmail.com>
2023-11-21 05:30 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Peter Smith @ 2023-11-20 22:07 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Nov 20, 2023 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
>
...
> Thanks for reporting this issue, I was able to reproduce the issue.
> This issue is happening because we are trying to specify the column
> list while the table sync worker is copying data for the table.
> I felt we should not specify the column list when the table has no columns.
> Attached patch has the changes to handle the same.
>
Hi Vignesh,
One small comment about the patch:
- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s ",
quote_qualified_identifier(lrel.nspname, lrel.relname));
- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, "(");
For consistent whitespace handling, and to prevent double-spacing when
there are no columns, I think that the trailing space of "COPY %s "
should be removed/replaced by a leading space for the "(".
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: BUG #18203: Logical Replication initial sync failure
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` Re: BUG #18203: Logical Replication initial sync failure vignesh C <vignesh21@gmail.com>
2023-11-20 22:07 ` Re: BUG #18203: Logical Replication initial sync failure Peter Smith <smithpb2250@gmail.com>
@ 2023-11-21 05:30 ` vignesh C <vignesh21@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: vignesh C @ 2023-11-21 05:30 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: zzzzz.graf@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, 21 Nov 2023 at 03:38, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Nov 20, 2023 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> ...
> > Thanks for reporting this issue, I was able to reproduce the issue.
> > This issue is happening because we are trying to specify the column
> > list while the table sync worker is copying data for the table.
> > I felt we should not specify the column list when the table has no columns.
> > Attached patch has the changes to handle the same.
> >
>
> Hi Vignesh,
>
> One small comment about the patch:
>
> - appendStringInfo(&cmd, "COPY %s (",
> + appendStringInfo(&cmd, "COPY %s ",
> quote_qualified_identifier(lrel.nspname, lrel.relname));
>
> - /*
> - * XXX Do we need to list the columns in all cases? Maybe we're
> - * replicating all columns?
> - */
> - for (int i = 0; i < lrel.natts; i++)
> + /* If the table has columns, then specify the columns */
> + if (lrel.natts)
> {
> - if (i > 0)
> - appendStringInfoString(&cmd, ", ");
> + appendStringInfoString(&cmd, "(");
>
> For consistent whitespace handling, and to prevent double-spacing when
> there are no columns, I think that the trailing space of "COPY %s "
> should be removed/replaced by a leading space for the "(".
This is fixed in the v2 version patch attached at:
https://www.postgresql.org/message-id/CALDaNm2kSJCVree6R1iKxjbA%3DSg4raFi3Fi_%2BwfF2xn2roxqCg%40mail...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-11-21 18:02 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-18 01:25 BUG #18203: Logical Replication initial sync failure PG Bug reporting form <noreply@postgresql.org>
2023-11-20 09:23 ` vignesh C <vignesh21@gmail.com>
2023-11-20 13:08 ` Amit Kapila <amit.kapila16@gmail.com>
2023-11-21 05:28 ` vignesh C <vignesh21@gmail.com>
2023-11-21 08:00 ` Peter Smith <smithpb2250@gmail.com>
2023-11-21 18:02 ` vignesh C <vignesh21@gmail.com>
2023-11-20 22:07 ` Peter Smith <smithpb2250@gmail.com>
2023-11-21 05:30 ` vignesh C <vignesh21@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox