RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tuesday, October 18, 2022 5:50 PM Alvaro Herrera wrote: > > On 2022-Oct-18, Japin Li wrote: > > > > > On Tue, 18 Oct 2022 at 12:00, houzj.f...@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
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On 2022-Oct-18, Japin Li wrote: > > On Tue, 18 Oct 2022 at 12:00, houzj.f...@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/
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tue, 18 Oct 2022 at 12:00, houzj.f...@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.
RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Monday, October 17, 2022 6:14 PM Amit Kapila wrote: > > On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera > wrote: > > > > On 2022-Oct-17, Peter Smith wrote: > > > > > On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera > 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 v2-0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patch Description: v2-0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patch
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera wrote:
>
> On 2022-Oct-17, Peter Smith wrote:
>
> > On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera
> > 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.
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On 2022-Oct-17, Peter Smith wrote:
> On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera
> 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)
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera wrote:
>
> Hello
>
> On 2022-Oct-17, houzj.f...@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.
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
Hello
On 2022-Oct-17, houzj.f...@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
Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Oct 17, 2022 at 8:39 AM houzj.f...@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.
