Thread: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
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
Attachment
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.
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
isenabled"),
> - 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
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_datais not allowed when two_phase is enabled"),
> > - errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION
withrefresh = false, or with copy_data = false"
> > - ", or use DROP/CREATE
SUBSCRIPTION.")));
> > + errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION
withrefresh = 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.
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)
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.
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
Attachment
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.
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/
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