RE: Added schema level support for publication.

On Friday, August 27, 2021 2:13 PM vignesh C <vignesh21(at)gmail(dot)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