public inbox for [email protected]  
help / color / mirror / Atom feed
From: Peter Eisentraut <[email protected]>
To: Euler Taveira <[email protected]>
To: Rahila Syed <[email protected]>
Cc: Önder Kalacı <[email protected]>
Cc: japin <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: David Steele <[email protected]>
Cc: Craig Ringer <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Amit Langote <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: row filtering for logical replication
Date: Thu, 25 Mar 2021 12:15:56 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CA+HiwqH+9NK5m-=BOYpzjH5UEpeWjT_3HEm4XBYNrda88naGMQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<ME3P282MB16678160F5EA09DCB962474EB6B69@ME3P282MB1667.AUSP282.PROD.OUTLOOK.COM>
	<[email protected]>
	<YBjc/[email protected]>
	<MEYP282MB16699D041638CE2F8284701FB6B59@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
	<MEYP282MB1669F5063EC084C0C5854EB1B6B59@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
	<[email protected]>
	<CACawEhW0NUMHHAQjVP_xM8gWXKvy9haK9nKhC-H=Qn2yTCv-=g@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CAH2L28tjTYctr+eu6Vg5U6iihVhe6P+qKnB_diMGy4=CfwXzdg@mail.gmail.com>
	<CAH2L28sS8=jY6Lj2-xVu6exxQ9gK8uGZr9jf64zDw9hacMrkyA@mail.gmail.com>
	<[email protected]>

On 22.03.21 03:15, Euler Taveira wrote:
> I attached a new patch set that addresses:
> 
> * fix documentation;
> * rename PublicationRelationQual to PublicationRelationInfo;
> * remove the memset that was leftover from a previous patch set;
> * add new tests to improve coverage (INSERT/UPDATE/DELETE to exercise 
> the row
>    filter code).

I have committed the 0001 patch.

Attached are a few fixup patches that I recommend you integrate into 
your patch set.  They address backward compatibility with PG13, and a 
few more stylistic issues.

I suggest you combine your 0002, 0003, and 0004 patches into one.  They 
can't be used separately, and for example the psql changes in patch 0003 
already appear as regression test output changes in 0002, so this 
arrangement isn't useful.  (0005 can be kept separately, since it's 
mostly for debugging right now.)

From c29459505db88c86dd7aa61019fa406202c30b0a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 11:57:48 +0100
Subject: [PATCH 1/6] fixup! Row filter for logical replication

Remove unused header files, clean up whitespace.
---
 src/backend/catalog/pg_publication.c        | 2 --
 src/backend/replication/pgoutput/pgoutput.c | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index f31ae28de2..78f5780fb7 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -33,11 +33,9 @@
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "miscadmin.h"
-
 #include "parser/parse_clause.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_relation.h"
-
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/catcache.h"
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index ce6da8de19..6151f34925 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -16,14 +16,10 @@
 #include "catalog/partition.h"
 #include "catalog/pg_publication.h"
 #include "catalog/pg_publication_rel.h"
-#include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "executor/executor.h"
 #include "fmgr.h"
-#include "nodes/execnodes.h"
 #include "nodes/nodeFuncs.h"
-#include "optimizer/planner.h"
-#include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "replication/logical.h"
 #include "replication/logicalproto.h"
-- 
2.30.2


From 91c20ecb45a8627a9252ed589fe6b85927be8b45 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 11:59:13 +0100
Subject: [PATCH 2/6] fixup! Row filter for logical replication

Allow replication from older PostgreSQL versions without prqual.
---
 src/backend/replication/logical/tablesync.c | 70 +++++++++++----------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 40d84dadb5..246510c82e 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -796,49 +796,53 @@ fetch_remote_table_info(char *nspname, char *relname,
 	walrcv_clear_result(res);
 
 	/* Get relation qual */
-	resetStringInfo(&cmd);
-	appendStringInfo(&cmd,
-					 "SELECT pg_get_expr(prqual, prrelid) "
-					 "  FROM pg_publication p "
-					 "  INNER JOIN pg_publication_rel pr "
-					 "       ON (p.oid = pr.prpubid) "
-					 " WHERE pr.prrelid = %u "
-					 "   AND p.pubname IN (", lrel->remoteid);
-
-	first = true;
-	foreach(lc, MySubscription->publications)
+	if (walrcv_server_version(wrconn) >= 140000)
 	{
-		char	   *pubname = strVal(lfirst(lc));
+		resetStringInfo(&cmd);
+		appendStringInfo(&cmd,
+						 "SELECT pg_get_expr(prqual, prrelid) "
+						 "  FROM pg_publication p "
+						 "  INNER JOIN pg_publication_rel pr "
+						 "       ON (p.oid = pr.prpubid) "
+						 " WHERE pr.prrelid = %u "
+						 "   AND p.pubname IN (", lrel->remoteid);
+
+		first = true;
+		foreach(lc, MySubscription->publications)
+		{
+			char	   *pubname = strVal(lfirst(lc));
 
-		if (first)
-			first = false;
-		else
-			appendStringInfoString(&cmd, ", ");
+			if (first)
+				first = false;
+			else
+				appendStringInfoString(&cmd, ", ");
 
-		appendStringInfoString(&cmd, quote_literal_cstr(pubname));
-	}
-	appendStringInfoChar(&cmd, ')');
+			appendStringInfoString(&cmd, quote_literal_cstr(pubname));
+		}
+		appendStringInfoChar(&cmd, ')');
 
-	res = walrcv_exec(wrconn, cmd.data, 1, qualRow);
+		res = walrcv_exec(wrconn, cmd.data, 1, qualRow);
 
-	if (res->status != WALRCV_OK_TUPLES)
-		ereport(ERROR,
-				(errmsg("could not fetch relation qualifications for table \"%s.%s\" from publisher: %s",
-						nspname, relname, res->err)));
+		if (res->status != WALRCV_OK_TUPLES)
+			ereport(ERROR,
+					(errmsg("could not fetch relation qualifications for table \"%s.%s\" from publisher: %s",
+							nspname, relname, res->err)));
 
-	slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
-	while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))
-	{
-		Datum		rf = slot_getattr(slot, 1, &isnull);
+		slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+		while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))
+		{
+			Datum		rf = slot_getattr(slot, 1, &isnull);
 
-		if (!isnull)
-			*qual = lappend(*qual, makeString(TextDatumGetCString(rf)));
+			if (!isnull)
+				*qual = lappend(*qual, makeString(TextDatumGetCString(rf)));
 
-		ExecClearTuple(slot);
+			ExecClearTuple(slot);
+		}
+		ExecDropSingleTupleTableSlot(slot);
+
+		walrcv_clear_result(res);
 	}
-	ExecDropSingleTupleTableSlot(slot);
 
-	walrcv_clear_result(res);
 	pfree(cmd.data);
 }
 
-- 
2.30.2


From d40b463def29cdded18579d6e584cb9f5f4764d6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:00:35 +0100
Subject: [PATCH 3/6] fixup! Row filter for logical replication

Use more idiomatic style for checking for empty or nonempty lists.
---
 src/backend/replication/logical/tablesync.c | 4 ++--
 src/backend/replication/pgoutput/pgoutput.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 246510c82e..c3d6847b35 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -881,7 +881,7 @@ copy_table(Relation rel)
 	initStringInfo(&cmd);
 
 	/* Regular table with no row filter */
-	if (lrel.relkind == RELKIND_RELATION && list_length(qual) == 0)
+	if (lrel.relkind == RELKIND_RELATION && qual == NIL)
 		appendStringInfo(&cmd, "COPY %s TO STDOUT",
 						 quote_qualified_identifier(lrel.nspname, lrel.relname));
 	else
@@ -902,7 +902,7 @@ copy_table(Relation rel)
 		appendStringInfo(&cmd, " FROM %s",
 						 quote_qualified_identifier(lrel.nspname, lrel.relname));
 		/* list of AND'ed filters */
-		if (list_length(qual) > 0)
+		if (qual != NIL)
 		{
 			ListCell   *lc;
 			bool		first = true;
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 6151f34925..593a8c96c8 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -568,7 +568,7 @@ pgoutput_row_filter(Relation relation, HeapTuple oldtuple, HeapTuple newtuple, L
 	bool		result = true;
 
 	/* Bail out if there is no row filter */
-	if (list_length(rowfilter) == 0)
+	if (rowfilter == NIL)
 		return true;
 
 	elog(DEBUG3, "table \"%s.%s\" has row filter",
@@ -1372,7 +1372,7 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue)
 		entry->pubactions.pubdelete = false;
 		entry->pubactions.pubtruncate = false;
 
-		if (list_length(entry->qual) > 0)
+		if (entry->qual != NIL)
 			list_free_deep(entry->qual);
 		entry->qual = NIL;
 	}
-- 
2.30.2


From b82e7b05bb3fd73bede39f480c31d657fe410e41 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:02:48 +0100
Subject: [PATCH 4/6] fixup! Row filter for logical replication

elog arguments are not evaluated unless the message level is
interesting, so the previous workaround is unnecessary.
---
 src/backend/replication/pgoutput/pgoutput.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 593a8c96c8..00ad8f001f 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -603,17 +603,11 @@ pgoutput_row_filter(Relation relation, HeapTuple oldtuple, HeapTuple newtuple, L
 		/* Evaluates row filter */
 		result = pgoutput_row_filter_exec_expr(exprstate, ecxt);
 
-		if (message_level_is_interesting(DEBUG3))
-		{
-			char	   *s = NULL;
-
-			s = TextDatumGetCString(DirectFunctionCall2(pg_get_expr, CStringGetTextDatum(nodeToString(rfnode)), ObjectIdGetDatum(relation->rd_id)));
-			if (result)
-				elog(DEBUG3, "row filter \"%s\" matched", s);
-			else
-				elog(DEBUG3, "row filter \"%s\" not matched", s);
-			pfree(s);
-		}
+		elog(DEBUG3, "row filter \"%s\" %smatched",
+			 TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
+													 CStringGetTextDatum(nodeToString(rfnode)),
+													 ObjectIdGetDatum(relation->rd_id))),
+			 result ? "" : " not");
 
 		/* If the tuple does not match one of the row filters, bail out */
 		if (!result)
-- 
2.30.2


From af012ad7d7592f88e353642fd715a3825ea2ec72 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:07:11 +0100
Subject: [PATCH 5/6] fixup! Publication WHERE condition support for pg_dump

Make pg_dump backward compatible.

Also add necessary parentheses around expression.  pg_get_expr will
supply the parentheses in many cases, but it won't for things like
"WHERE TRUE".
---
 src/bin/pg_dump/pg_dump.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 72d87f21c8..af57a50f27 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4141,10 +4141,16 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
 	query = createPQExpBuffer();
 
 	/* Collect all publication membership info. */
-	appendPQExpBufferStr(query,
-						 "SELECT tableoid, oid, prpubid, prrelid, "
-						 "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual "
-						 "FROM pg_catalog.pg_publication_rel");
+	if (fout->remoteVersion >= 140000)
+		appendPQExpBufferStr(query,
+							 "SELECT tableoid, oid, prpubid, prrelid, "
+							 "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual "
+							 "FROM pg_catalog.pg_publication_rel");
+	else
+		appendPQExpBufferStr(query,
+							 "SELECT tableoid, oid, prpubid, prrelid, "
+							 "NULL AS prrelqual "
+							 "FROM pg_catalog.pg_publication_rel");
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
 	ntups = PQntuples(res);
@@ -4233,7 +4239,7 @@ dumpPublicationTable(Archive *fout, const PublicationRelInfo *pubrinfo)
 	appendPQExpBuffer(query, " %s",
 					  fmtQualifiedDumpable(tbinfo));
 	if (pubrinfo->pubrelqual)
-		appendPQExpBuffer(query, " WHERE %s", pubrinfo->pubrelqual);
+		appendPQExpBuffer(query, " WHERE (%s)", pubrinfo->pubrelqual);
 	appendPQExpBufferStr(query, ";\n");
 
 	/*
-- 
2.30.2


From 326c489012f020f8ef38e1187aea72b332d1b9a4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:08:51 +0100
Subject: [PATCH 6/6] fixup! Print publication WHERE condition in psql

Make psql backward compatible.  Also add necessary parentheses.
---
 src/bin/psql/describe.c                   | 11 +++++++----
 src/test/regress/expected/publication.out |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 983ba512f7..6dce968414 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6038,9 +6038,12 @@ describePublications(const char *pattern)
 		if (!puballtables)
 		{
 			printfPQExpBuffer(&buf,
-							  "SELECT n.nspname, c.relname,\n"
-							  "  pg_get_expr(pr.prqual, c.oid)\n"
-							  "FROM pg_catalog.pg_class c,\n"
+							  "SELECT n.nspname, c.relname");
+			if (pset.sversion >= 140000)
+				appendPQExpBuffer(&buf,
+								  ", pg_get_expr(pr.prqual, c.oid)");
+			appendPQExpBuffer(&buf,
+							  "\nFROM pg_catalog.pg_class c,\n"
 							  "     pg_catalog.pg_namespace n,\n"
 							  "     pg_catalog.pg_publication_rel pr\n"
 							  "WHERE c.relnamespace = n.oid\n"
@@ -6070,7 +6073,7 @@ describePublications(const char *pattern)
 								  PQgetvalue(tabres, j, 1));
 
 				if (!PQgetisnull(tabres, j, 2))
-					appendPQExpBuffer(&buf, "  WHERE %s",
+					appendPQExpBuffer(&buf, " WHERE (%s)",
 									  PQgetvalue(tabres, j, 2));
 
 				printTableAddFooter(&cont, buf.data);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index c8cf1b685e..da49a2e215 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -181,7 +181,7 @@ ERROR:  cannot use a WHERE clause when removing table from publication "testpub5
 --------------------------+------------+---------+---------+---------+-----------+----------
  regress_publication_user | f          | t       | t       | t       | t         | f
 Tables:
-    "public.testpub_rf_tbl3"  WHERE ((e > 300) AND (e < 500))
+    "public.testpub_rf_tbl3" WHERE (((e > 300) AND (e < 500)))
 
 DROP TABLE testpub_rf_tbl1;
 DROP TABLE testpub_rf_tbl2;
-- 
2.30.2



Attachments:

  [text/plain] 0001-fixup-Row-filter-for-logical-replication.patch (1.6K, ../[email protected]/2-0001-fixup-Row-filter-for-logical-replication.patch)
  download | inline diff:
From c29459505db88c86dd7aa61019fa406202c30b0a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 11:57:48 +0100
Subject: [PATCH 1/6] fixup! Row filter for logical replication

Remove unused header files, clean up whitespace.
---
 src/backend/catalog/pg_publication.c        | 2 --
 src/backend/replication/pgoutput/pgoutput.c | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index f31ae28de2..78f5780fb7 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -33,11 +33,9 @@
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "miscadmin.h"
-
 #include "parser/parse_clause.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_relation.h"
-
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/catcache.h"
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index ce6da8de19..6151f34925 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -16,14 +16,10 @@
 #include "catalog/partition.h"
 #include "catalog/pg_publication.h"
 #include "catalog/pg_publication_rel.h"
-#include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "executor/executor.h"
 #include "fmgr.h"
-#include "nodes/execnodes.h"
 #include "nodes/nodeFuncs.h"
-#include "optimizer/planner.h"
-#include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "replication/logical.h"
 #include "replication/logicalproto.h"
-- 
2.30.2



  [text/plain] 0002-fixup-Row-filter-for-logical-replication.patch (3.2K, ../[email protected]/3-0002-fixup-Row-filter-for-logical-replication.patch)
  download | inline diff:
From 91c20ecb45a8627a9252ed589fe6b85927be8b45 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 11:59:13 +0100
Subject: [PATCH 2/6] fixup! Row filter for logical replication

Allow replication from older PostgreSQL versions without prqual.
---
 src/backend/replication/logical/tablesync.c | 70 +++++++++++----------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 40d84dadb5..246510c82e 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -796,49 +796,53 @@ fetch_remote_table_info(char *nspname, char *relname,
 	walrcv_clear_result(res);
 
 	/* Get relation qual */
-	resetStringInfo(&cmd);
-	appendStringInfo(&cmd,
-					 "SELECT pg_get_expr(prqual, prrelid) "
-					 "  FROM pg_publication p "
-					 "  INNER JOIN pg_publication_rel pr "
-					 "       ON (p.oid = pr.prpubid) "
-					 " WHERE pr.prrelid = %u "
-					 "   AND p.pubname IN (", lrel->remoteid);
-
-	first = true;
-	foreach(lc, MySubscription->publications)
+	if (walrcv_server_version(wrconn) >= 140000)
 	{
-		char	   *pubname = strVal(lfirst(lc));
+		resetStringInfo(&cmd);
+		appendStringInfo(&cmd,
+						 "SELECT pg_get_expr(prqual, prrelid) "
+						 "  FROM pg_publication p "
+						 "  INNER JOIN pg_publication_rel pr "
+						 "       ON (p.oid = pr.prpubid) "
+						 " WHERE pr.prrelid = %u "
+						 "   AND p.pubname IN (", lrel->remoteid);
+
+		first = true;
+		foreach(lc, MySubscription->publications)
+		{
+			char	   *pubname = strVal(lfirst(lc));
 
-		if (first)
-			first = false;
-		else
-			appendStringInfoString(&cmd, ", ");
+			if (first)
+				first = false;
+			else
+				appendStringInfoString(&cmd, ", ");
 
-		appendStringInfoString(&cmd, quote_literal_cstr(pubname));
-	}
-	appendStringInfoChar(&cmd, ')');
+			appendStringInfoString(&cmd, quote_literal_cstr(pubname));
+		}
+		appendStringInfoChar(&cmd, ')');
 
-	res = walrcv_exec(wrconn, cmd.data, 1, qualRow);
+		res = walrcv_exec(wrconn, cmd.data, 1, qualRow);
 
-	if (res->status != WALRCV_OK_TUPLES)
-		ereport(ERROR,
-				(errmsg("could not fetch relation qualifications for table \"%s.%s\" from publisher: %s",
-						nspname, relname, res->err)));
+		if (res->status != WALRCV_OK_TUPLES)
+			ereport(ERROR,
+					(errmsg("could not fetch relation qualifications for table \"%s.%s\" from publisher: %s",
+							nspname, relname, res->err)));
 
-	slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
-	while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))
-	{
-		Datum		rf = slot_getattr(slot, 1, &isnull);
+		slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+		while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))
+		{
+			Datum		rf = slot_getattr(slot, 1, &isnull);
 
-		if (!isnull)
-			*qual = lappend(*qual, makeString(TextDatumGetCString(rf)));
+			if (!isnull)
+				*qual = lappend(*qual, makeString(TextDatumGetCString(rf)));
 
-		ExecClearTuple(slot);
+			ExecClearTuple(slot);
+		}
+		ExecDropSingleTupleTableSlot(slot);
+
+		walrcv_clear_result(res);
 	}
-	ExecDropSingleTupleTableSlot(slot);
 
-	walrcv_clear_result(res);
 	pfree(cmd.data);
 }
 
-- 
2.30.2



  [text/plain] 0003-fixup-Row-filter-for-logical-replication.patch (2.1K, ../[email protected]/4-0003-fixup-Row-filter-for-logical-replication.patch)
  download | inline diff:
From d40b463def29cdded18579d6e584cb9f5f4764d6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:00:35 +0100
Subject: [PATCH 3/6] fixup! Row filter for logical replication

Use more idiomatic style for checking for empty or nonempty lists.
---
 src/backend/replication/logical/tablesync.c | 4 ++--
 src/backend/replication/pgoutput/pgoutput.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 246510c82e..c3d6847b35 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -881,7 +881,7 @@ copy_table(Relation rel)
 	initStringInfo(&cmd);
 
 	/* Regular table with no row filter */
-	if (lrel.relkind == RELKIND_RELATION && list_length(qual) == 0)
+	if (lrel.relkind == RELKIND_RELATION && qual == NIL)
 		appendStringInfo(&cmd, "COPY %s TO STDOUT",
 						 quote_qualified_identifier(lrel.nspname, lrel.relname));
 	else
@@ -902,7 +902,7 @@ copy_table(Relation rel)
 		appendStringInfo(&cmd, " FROM %s",
 						 quote_qualified_identifier(lrel.nspname, lrel.relname));
 		/* list of AND'ed filters */
-		if (list_length(qual) > 0)
+		if (qual != NIL)
 		{
 			ListCell   *lc;
 			bool		first = true;
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 6151f34925..593a8c96c8 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -568,7 +568,7 @@ pgoutput_row_filter(Relation relation, HeapTuple oldtuple, HeapTuple newtuple, L
 	bool		result = true;
 
 	/* Bail out if there is no row filter */
-	if (list_length(rowfilter) == 0)
+	if (rowfilter == NIL)
 		return true;
 
 	elog(DEBUG3, "table \"%s.%s\" has row filter",
@@ -1372,7 +1372,7 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue)
 		entry->pubactions.pubdelete = false;
 		entry->pubactions.pubtruncate = false;
 
-		if (list_length(entry->qual) > 0)
+		if (entry->qual != NIL)
 			list_free_deep(entry->qual);
 		entry->qual = NIL;
 	}
-- 
2.30.2



  [text/plain] 0004-fixup-Row-filter-for-logical-replication.patch (1.6K, ../[email protected]/5-0004-fixup-Row-filter-for-logical-replication.patch)
  download | inline diff:
From b82e7b05bb3fd73bede39f480c31d657fe410e41 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:02:48 +0100
Subject: [PATCH 4/6] fixup! Row filter for logical replication

elog arguments are not evaluated unless the message level is
interesting, so the previous workaround is unnecessary.
---
 src/backend/replication/pgoutput/pgoutput.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 593a8c96c8..00ad8f001f 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -603,17 +603,11 @@ pgoutput_row_filter(Relation relation, HeapTuple oldtuple, HeapTuple newtuple, L
 		/* Evaluates row filter */
 		result = pgoutput_row_filter_exec_expr(exprstate, ecxt);
 
-		if (message_level_is_interesting(DEBUG3))
-		{
-			char	   *s = NULL;
-
-			s = TextDatumGetCString(DirectFunctionCall2(pg_get_expr, CStringGetTextDatum(nodeToString(rfnode)), ObjectIdGetDatum(relation->rd_id)));
-			if (result)
-				elog(DEBUG3, "row filter \"%s\" matched", s);
-			else
-				elog(DEBUG3, "row filter \"%s\" not matched", s);
-			pfree(s);
-		}
+		elog(DEBUG3, "row filter \"%s\" %smatched",
+			 TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
+													 CStringGetTextDatum(nodeToString(rfnode)),
+													 ObjectIdGetDatum(relation->rd_id))),
+			 result ? "" : " not");
 
 		/* If the tuple does not match one of the row filters, bail out */
 		if (!result)
-- 
2.30.2



  [text/plain] 0005-fixup-Publication-WHERE-condition-support-for-pg_dum.patch (1.9K, ../[email protected]/6-0005-fixup-Publication-WHERE-condition-support-for-pg_dum.patch)
  download | inline diff:
From af012ad7d7592f88e353642fd715a3825ea2ec72 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:07:11 +0100
Subject: [PATCH 5/6] fixup! Publication WHERE condition support for pg_dump

Make pg_dump backward compatible.

Also add necessary parentheses around expression.  pg_get_expr will
supply the parentheses in many cases, but it won't for things like
"WHERE TRUE".
---
 src/bin/pg_dump/pg_dump.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 72d87f21c8..af57a50f27 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4141,10 +4141,16 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
 	query = createPQExpBuffer();
 
 	/* Collect all publication membership info. */
-	appendPQExpBufferStr(query,
-						 "SELECT tableoid, oid, prpubid, prrelid, "
-						 "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual "
-						 "FROM pg_catalog.pg_publication_rel");
+	if (fout->remoteVersion >= 140000)
+		appendPQExpBufferStr(query,
+							 "SELECT tableoid, oid, prpubid, prrelid, "
+							 "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual "
+							 "FROM pg_catalog.pg_publication_rel");
+	else
+		appendPQExpBufferStr(query,
+							 "SELECT tableoid, oid, prpubid, prrelid, "
+							 "NULL AS prrelqual "
+							 "FROM pg_catalog.pg_publication_rel");
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
 	ntups = PQntuples(res);
@@ -4233,7 +4239,7 @@ dumpPublicationTable(Archive *fout, const PublicationRelInfo *pubrinfo)
 	appendPQExpBuffer(query, " %s",
 					  fmtQualifiedDumpable(tbinfo));
 	if (pubrinfo->pubrelqual)
-		appendPQExpBuffer(query, " WHERE %s", pubrinfo->pubrelqual);
+		appendPQExpBuffer(query, " WHERE (%s)", pubrinfo->pubrelqual);
 	appendPQExpBufferStr(query, ";\n");
 
 	/*
-- 
2.30.2



  [text/plain] 0006-fixup-Print-publication-WHERE-condition-in-psql.patch (2.2K, ../[email protected]/7-0006-fixup-Print-publication-WHERE-condition-in-psql.patch)
  download | inline diff:
From 326c489012f020f8ef38e1187aea72b332d1b9a4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 25 Mar 2021 12:08:51 +0100
Subject: [PATCH 6/6] fixup! Print publication WHERE condition in psql

Make psql backward compatible.  Also add necessary parentheses.
---
 src/bin/psql/describe.c                   | 11 +++++++----
 src/test/regress/expected/publication.out |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 983ba512f7..6dce968414 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6038,9 +6038,12 @@ describePublications(const char *pattern)
 		if (!puballtables)
 		{
 			printfPQExpBuffer(&buf,
-							  "SELECT n.nspname, c.relname,\n"
-							  "  pg_get_expr(pr.prqual, c.oid)\n"
-							  "FROM pg_catalog.pg_class c,\n"
+							  "SELECT n.nspname, c.relname");
+			if (pset.sversion >= 140000)
+				appendPQExpBuffer(&buf,
+								  ", pg_get_expr(pr.prqual, c.oid)");
+			appendPQExpBuffer(&buf,
+							  "\nFROM pg_catalog.pg_class c,\n"
 							  "     pg_catalog.pg_namespace n,\n"
 							  "     pg_catalog.pg_publication_rel pr\n"
 							  "WHERE c.relnamespace = n.oid\n"
@@ -6070,7 +6073,7 @@ describePublications(const char *pattern)
 								  PQgetvalue(tabres, j, 1));
 
 				if (!PQgetisnull(tabres, j, 2))
-					appendPQExpBuffer(&buf, "  WHERE %s",
+					appendPQExpBuffer(&buf, " WHERE (%s)",
 									  PQgetvalue(tabres, j, 2));
 
 				printTableAddFooter(&cont, buf.data);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index c8cf1b685e..da49a2e215 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -181,7 +181,7 @@ ERROR:  cannot use a WHERE clause when removing table from publication "testpub5
 --------------------------+------------+---------+---------+---------+-----------+----------
  regress_publication_user | f          | t       | t       | t       | t         | f
 Tables:
-    "public.testpub_rf_tbl3"  WHERE ((e > 300) AND (e < 500))
+    "public.testpub_rf_tbl3" WHERE (((e > 300) AND (e < 500)))
 
 DROP TABLE testpub_rf_tbl1;
 DROP TABLE testpub_rf_tbl2;
-- 
2.30.2



view thread (489+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: row filtering for logical replication
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox