Horton

Thread

Commits

  1. Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

  1. Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-10-17T03:09:29Z

    Hi,

While working on some logical replication related features.
I found the HINT message could be improved when I tried to add a publication to
a subscription which was disabled.

alter subscription sub add publication pub2;
--
ERROR: ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions
HINT: Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).
--

Because I was executing the ADD PUBLICATION command, I feel the hint should
also mention it instead of SET PUBLICATION.

Best regards,
Hou zj

  2. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Amit Kapila <amit.kapila16@gmail.com> — 2022-10-17T04:13:14Z

    On Mon, Oct 17, 2022 at 8:39 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> While working on some logical replication related features.
> I found the HINT message could be improved when I tried to add a publication to
> a subscription which was disabled.
>
> alter subscription sub add publication pub2;
> --
> ERROR: ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions
> HINT: Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).
> --
>
> Because I was executing the ADD PUBLICATION command, I feel the hint should
> also mention it instead of SET PUBLICATION.
>

+1. I haven't tested it yet but the changes look sane.

-- 
With Regards,
Amit Kapila.

  3. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-17T07:43:42Z

    Hello

On 2022-Oct-17, houzj.fnst@fujitsu.com wrote:

> alter subscription sub add publication pub2;

> Because I was executing the ADD PUBLICATION command, I feel the hint should
> also mention it instead of SET PUBLICATION.

Hmm, ok.  But:


> @@ -1236,8 +1237,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
>  						ereport(ERROR,
>  								(errcode(ERRCODE_SYNTAX_ERROR),
>  								 errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
> -								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
> -										 ", or use DROP/CREATE SUBSCRIPTION.")));
> +								 errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION with refresh = false, or with copy_data = false"
> +										 ", or use DROP/CREATE SUBSCRIPTION.",
> +										 isadd ? "ADD" : "DROP")));

This looks confusing for translators.  I propose to move the whole
command out of the message, not just one piece of it:

+    /*- translator: %s is an ALTER DDL command */
+    errhint("Use %s with refresh = false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.",
             isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION" : ALTER SUBSCRIPTION ... DROP PUBLICATION")

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"You don't solve a bad join with SELECT DISTINCT" #CupsOfFail
https://twitter.com/connor_mc_d/status/1431240081726115845

  4. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Peter Smith <smithpb2250@gmail.com> — 2022-10-17T08:45:07Z

    On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello
>
> On 2022-Oct-17, houzj.fnst@fujitsu.com wrote:
>
> > alter subscription sub add publication pub2;
>
> > Because I was executing the ADD PUBLICATION command, I feel the hint should
> > also mention it instead of SET PUBLICATION.
>
> Hmm, ok.  But:
>
>
> > @@ -1236,8 +1237,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
> >                                               ereport(ERROR,
> >                                                               (errcode(ERRCODE_SYNTAX_ERROR),
> >                                                                errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
> > -                                                              errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
> > -                                                                              ", or use DROP/CREATE SUBSCRIPTION.")));
> > +                                                              errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION with refresh = false, or with copy_data = false"
> > +                                                                              ", or use DROP/CREATE SUBSCRIPTION.",
> > +                                                                              isadd ? "ADD" : "DROP")));
>
> This looks confusing for translators.  I propose to move the whole
> command out of the message, not just one piece of it:
>
> +    /*- translator: %s is an ALTER DDL command */
> +    errhint("Use %s with refresh = false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.",
>              isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION" : ALTER SUBSCRIPTION ... DROP PUBLICATION")
>
> I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
> like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.
>

I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
make it the same as similar messages in the same function when
incompatible parameters are specified.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

  5. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-17T09:10:52Z

    On 2022-Oct-17, Peter Smith wrote:

> On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
> > like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.
> 
> I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
> make it the same as similar messages in the same function when
> incompatible parameters are specified.

Hmm, yeah, I guess that's also a possibility.

Maybe we need a specific errcode, "incompatible logical replication
configuration", within that class ("object not in prerequisite state" is
a generic SQLSTATE class 55), given that the class itself is a mishmash
of completely unrelated things.  I think I already mentioned this in
some other thread ... ah yes:

https://postgr.es/m/20220928084641.xecjrgym476fihtn@alvherre.pgsql
"incompatible publication definition" 55PR1 is what I suggested then.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

  6. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Amit Kapila <amit.kapila16@gmail.com> — 2022-10-17T10:14:23Z

    On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Oct-17, Peter Smith wrote:
>
> > On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > > I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
> > > like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.
> >
> > I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
> > make it the same as similar messages in the same function when
> > incompatible parameters are specified.
>
> Hmm, yeah, I guess that's also a possibility.
>

Right, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE seems to suite better here.

> Maybe we need a specific errcode, "incompatible logical replication
> configuration", within that class ("object not in prerequisite state" is
> a generic SQLSTATE class 55), given that the class itself is a mishmash
> of completely unrelated things.  I think I already mentioned this in
> some other thread ... ah yes:
>
> https://postgr.es/m/20220928084641.xecjrgym476fihtn@alvherre.pgsql
> "incompatible publication definition" 55PR1 is what I suggested then.
>

Yeah, this is another way to deal with it. But, won't it be better to
survey all call sites of ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE and
then try to subdivide it instead of doing it for
subscription/publication cases? I know that is a much bigger ask and
we don't need to do it for this patch but that seems like a more
future-proof way if we can build a consensus for the same.

-- 
With Regards,
Amit Kapila.

  7. RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-10-18T04:00:20Z

    On Monday, October 17, 2022 6:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >
> > On 2022-Oct-17, Peter Smith wrote:
> >
> > > On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> >
> > > > I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here;
> > > > sounds like ERRCODE_FEATURE_NOT_SUPPORTED might be more
> appropriate.
> > >
> > > I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which
> > > would make it the same as similar messages in the same function when
> > > incompatible parameters are specified.
> >
> > Hmm, yeah, I guess that's also a possibility.
> >
> 
> Right, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE seems to suite better
> here.

Agreed. Here is new version patch which changed the error code and
moved the whole command out of the message according to Álvaro's comment.

Best regards,
Hou zj

  8. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Japin Li <japinli@hotmail.com> — 2022-10-18T05:40:02Z

    On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

> Agreed. Here is new version patch which changed the error code and
> moved the whole command out of the message according to Álvaro's comment.
>

My bad!  The patch looks good to me.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

  9. Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-18T09:50:26Z

    On 2022-Oct-18, Japin Li wrote:

> 
> On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> 
> > Agreed. Here is new version patch which changed the error code and
> > moved the whole command out of the message according to Álvaro's comment.
> 
> My bad!  The patch looks good to me.

Thank you, I pushed it to both branches, because I realized we were
saying "SET PUBLICATION" when we meant "ADD/DROP"; that hint could be
quite damaging if anybody decides to actually follow it ISTM.

I noted that no test needed to be changed because of this, which is
perhaps somewhat concerning.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  10. RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-10-19T01:51:37Z

    On Tuesday, October 18, 2022 5:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 
> On 2022-Oct-18, Japin Li wrote:
> 
> >
> > On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > > Agreed. Here is new version patch which changed the error code and
> > > moved the whole command out of the message according to Álvaro's
> comment.
> >
> > My bad!  The patch looks good to me.
> 
> Thank you, I pushed it to both branches, because I realized we were saying "SET
> PUBLICATION" when we meant "ADD/DROP"; that hint could be quite
> damaging if anybody decides to actually follow it ISTM.

Thanks for pushing!

Best regards,
Hou zj