On Wed, Nov 10, 2021 at 11:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignesh21@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)));
Modified
> 2) Typo: It is "One of the specified publications exists."
> +# One of the specified publication exist.
Modified
> 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.
Modified
> 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");
> +
Modified
> 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");
Modified
Thanks for the comments, the attached v13 patch has the fixes for the same.
Regards,
Vignesh
