On Friday, August 27, 2021 2:13 PM vignesh C <[email protected]> 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
