RE: Added schema level support for publication. - Mailing list pgsql-hackers

On Friday, August 27, 2021 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> I have implemented this in the 0003 patch, I have kept it separate to reduce the
> testing effort and also it will be easier if someone disagrees with the syntax. I
> will merge it to the main patch later based on the feedback. Attached v22 patch
> has the changes for the same.
> Thoughts?

Hi,

Here are some comments for the new version patches.

About  0001
1)
+            rel->relpersistence = RELPERSISTENCE_PERMANENT;

It seems we don't need to set this since makeRangeVarFromNameList()
already set it.


2)
+            if (!relids || !schemarelids)
+                tables = list_concat(relids, schemarelids);
+            else
+                tables = list_concat_unique_oid(relids, schemarelids);
+        }

It seems we can simplify the above code like the following: 
tables = list_concat_unique_oid(relids, schemarelids);


3)
+        relids = GetPublicationRelations(pubform->oid,
+                                         PUBLICATION_PART_ALL);
+        schemarelids = GetAllSchemasPublicationRelations(pubform->oid,
+                                                         PUBLICATION_PART_ALL);
+        relids = list_concat(relids, schemarelids);

should we invoke list_concat_unique_oid here ?

4)

+                search_path = fetch_search_path(false);
+                if (search_path == NIL) /* nothing valid in search_path? */

It might be better to list_free(search_path) when not used.

5)
+            if (list_length(pubobj->name) == 1 &&
+                (strcmp(relname, "CURRENT_SCHEMA") == 0))
+                ereport(ERROR,
+                        errcode(ERRCODE_SYNTAX_ERROR),
+                        errmsg("invalid relation name at or near"),
+                        parser_errposition(pstate, pubobj->location));

Maybe we don't need this check, because it will report an error in
OpenTableList() anyway, "relation "CURRENT_SCHEMA" does not exist" , and that
message seems readable to me.


About  0002
6)

diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0c84d87873..0a479dfe36 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -6,7 +6,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 32;
+use Test::More tests => 46;

I think it might be better to move these testcases create a separate perl file.

About  0003
7) 
The v22-0003 seems simple and can remove lots of code in patch v22-0001, so
maybe we can merge 0001 and 0003 into one patch ?

Best regards,
Hou zj