From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Identify missing publications from publisher while create/alter subscription. |
Date: | 2021-11-13 07:19:59 |
Message-ID: | CALDaNm1g3tVgdBbXRvND4NakbEicB1eX5v3cP-dU7wN=rUL-BQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Nov 10, 2021 at 11:16 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignesh21(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v13-0001-Identify-missing-publications-from-publisher-whi.patch | application/x-patch | 19.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Deng Kaisheng | 2021-11-13 08:41:34 | Anything I can contribute? |
Previous Message | Thomas Munro | 2021-11-13 05:40:41 | Re: Invalid Unicode escape value at or near "\u0000" |