On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignes...@gmail.com> wrote:
> Attached v12 version is rebased on top of Head.
Thanks for the patch. Here are some comments on v12:
1) I think ERRCODE_TOO_MANY_ARGUMENTS isn't the right error code, the
ERRCODE_UNDEFINED_OBJECT is more meaningful. Please change.
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
+ errmsg_plural("publication %s does not exist in the publisher",
+ "publications %s do not exist in the publisher",
The existing code using
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("subscription \"%s\" does not exist", subname)));
2) Typo: It is "One of the specified publications exists."
+# One of the specified publication exist.
3) I think we can remove the test case "+# Specified publication does
not exist." because the "+# One of the specified publication exist."
covers the code.
4) Do we need the below test case? Even with refresh = false, it does
call connect_and_check_pubs() right? Please remove it.
+# Specified publication does not exist with refresh = false.
+($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
+ "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub
WITH (REFRESH = FALSE, VALIDATE_PUBLICATION = TRUE)"
+);
+ok( $stderr =~
+ m/ERROR: publication "non_existent_pub" does not exist in
the publisher/,
+ "Alter subscription for non existent publication fails");
+
5) Change the test case names to different ones instead of the same.
Have something like:
"Create subscription fails with single non-existent publication");
"Create subscription fails with multiple non-existent publications");
"Create subscription fails with mutually exclusive options");
"Alter subscription add publication fails with non-existent publication");
"Alter subscription set publication fails with non-existent publication");
"Alter subscription set publication fails with connection to a
non-existent database");
Unnecessary test cases would add up to the "make check-world" times,
please remove them.
Regards,
Bharath Rupireddy.
