Horton

Thread

Commits

  1. Remove publicationcmds.c's expr_allowed_in_node as a function

  2. Improve some publication-related error messages

  1. A doubt about a newly added errdetail

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-26T07:04:26Z

    I saw the following message recently added to publicationcmds.c.

(ERROR: cannot use publication column list for relation "%s.%s"")
> DETAIL: Column list cannot be specified if any schema is part of the publication or specified in the list.

As my reading, the "the list" at the end syntactically means "Column
list" but that is actually wrong; it could be read as "the list
following TABLES IN" but that doesn't seem reasonable.

If I am right, it might should be something like the following:

+ Column list cannot be specified if any schema is part of the publication or specified in the command.
+ Column list cannot be specified if any schema is part of the publication or specified together.

What do you think about this?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-26T07:39:49Z

    On 2022-Sep-26, Kyotaro Horiguchi wrote:

> I saw the following message recently added to publicationcmds.c.
> 
> (ERROR: cannot use publication column list for relation "%s.%s"")
> > DETAIL: Column list cannot be specified if any schema is part of the publication or specified in the list.
> 
> As my reading, the "the list" at the end syntactically means "Column
> list" but that is actually wrong; it could be read as "the list
> following TABLES IN" but that doesn't seem reasonable.
> 
> If I am right, it might should be something like the following:
> 
> + Column list cannot be specified if any schema is part of the publication or specified in the command.
> + Column list cannot be specified if any schema is part of the publication or specified together.

I propose

ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
DETAIL:  Column lists cannot be specified in publications containing FOR TABLES IN SCHEMA elements.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  3. Re: A doubt about a newly added errdetail

    Amit Kapila <amit.kapila16@gmail.com> — 2022-09-26T08:01:29Z

    On Mon, Sep 26, 2022 at 1:10 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Sep-26, Kyotaro Horiguchi wrote:
>
> > I saw the following message recently added to publicationcmds.c.
> >
> > (ERROR: cannot use publication column list for relation "%s.%s"")
> > > DETAIL: Column list cannot be specified if any schema is part of the publication or specified in the list.
> >
> > As my reading, the "the list" at the end syntactically means "Column
> > list" but that is actually wrong; it could be read as "the list
> > following TABLES IN" but that doesn't seem reasonable.
> >
> > If I am right, it might should be something like the following:
> >
> > + Column list cannot be specified if any schema is part of the publication or specified in the command.
> > + Column list cannot be specified if any schema is part of the publication or specified together.
>
> I propose
>
> ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
> DETAIL:  Column lists cannot be specified in publications containing FOR TABLES IN SCHEMA elements.
>

This looks mostly good to me. BTW, is it a good idea to add ".. in
publication "%s"" to the error message as this can happen even during
create publication? If so, I think we can change the nearby message as
below to include the same:

if (!pubviaroot &&
pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot use publication column list for relation \"%s\"",

I think even if we don't include the publication name, there won't be
any confusion because there won't be multiple publications in the
command.

-- 
With Regards,
Amit Kapila.

  4. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-26T08:33:12Z

    On 2022-Sep-26, Amit Kapila wrote:

> On Mon, Sep 26, 2022 at 1:10 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
> > DETAIL:  Column lists cannot be specified in publications containing FOR TABLES IN SCHEMA elements.
> 
> This looks mostly good to me. BTW, is it a good idea to add ".. in
> publication "%s"" to the error message as this can happen even during
> create publication?

Hmm, I don't see why not.  The publication is new, sure, but it would
already have a name, so there's no possible confusion as to what this
means.

(My main change was to take the word "publication" out of the phrase
"publication column list", because that seemed a bit strange; it isn't
the publication that has a column list, but the relation.)


> If so, I think we can change the nearby message as below to include
> the same:
> 
> if (!pubviaroot &&
> pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("cannot use publication column list for relation \"%s\"",

WFM.

> I think even if we don't include the publication name, there won't be
> any confusion because there won't be multiple publications in the
> command.

True, and the whole error report is likely to contain a STATEMENT line.

However, you could argue that specifying the publication in errmsg is
redundant.  But then, the "for relation %s.%s" is also redundant (since
that is *also* in the STATEMENT line), and could even be misleading: if
you have a command that specifies *two* relations with column lists, why
specify only the first one you find?  The user could be angry that they
remove the column list from that relation and retry, and then receive
the exact same error for the next relation with a list that they didn't
edit.  But I think people don't work that way.  So if you wanted to be
super precise you would also omit the relation name unless you scanned
the whole list and verified that only this relation is specifying a
column list; but whom does that help?

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

  5. Re: A doubt about a newly added errdetail

    Amit Kapila <amit.kapila16@gmail.com> — 2022-09-26T08:56:37Z

    On Mon, Sep 26, 2022 at 2:03 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Sep-26, Amit Kapila wrote:
>
> > On Mon, Sep 26, 2022 at 1:10 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > > ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
> > > DETAIL:  Column lists cannot be specified in publications containing FOR TABLES IN SCHEMA elements.
> >
> > This looks mostly good to me. BTW, is it a good idea to add ".. in
> > publication "%s"" to the error message as this can happen even during
> > create publication?
>
> Hmm, I don't see why not.  The publication is new, sure, but it would
> already have a name, so there's no possible confusion as to what this
> means.
>
> (My main change was to take the word "publication" out of the phrase
> "publication column list", because that seemed a bit strange; it isn't
> the publication that has a column list, but the relation.)
>

Okay, that makes sense.

>
> > If so, I think we can change the nearby message as below to include
> > the same:
> >
> > if (!pubviaroot &&
> > pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > errmsg("cannot use publication column list for relation \"%s\"",
>
> WFM.
>

Okay.

-- 
With Regards,
Amit Kapila.

  6. RE: A doubt about a newly added errdetail

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-09-26T09:03:22Z

    On Monday, September 26, 2022 4:57 PM Amit Kapila <amit.kapila16@gmail.com>
> 
> On Mon, Sep 26, 2022 at 2:03 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >
> > On 2022-Sep-26, Amit Kapila wrote:
> >
> > > On Mon, Sep 26, 2022 at 1:10 PM Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> >
> > > > ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
> > > > DETAIL:  Column lists cannot be specified in publications containing FOR
> TABLES IN SCHEMA elements.
> > >
> > > This looks mostly good to me. BTW, is it a good idea to add ".. in
> > > publication "%s"" to the error message as this can happen even
> > > during create publication?
> >
> > Hmm, I don't see why not.  The publication is new, sure, but it would
> > already have a name, so there's no possible confusion as to what this
> > means.
> >
> > (My main change was to take the word "publication" out of the phrase
> > "publication column list", because that seemed a bit strange; it isn't
> > the publication that has a column list, but the relation.)
> >
> 
> Okay, that makes sense.

+1

> >
> > > If so, I think we can change the nearby message as below to include
> > > the same:
> > >
> > > if (!pubviaroot &&
> > > pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > > ereport(ERROR,
> > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > errmsg("cannot use publication column list for relation \"%s\"",
> >
> > WFM.
> >
> 
> Okay.

While reviewing this part, I notice an unused parameter(queryString) of function
CheckPubRelationColumnList. I feel we can remove that as well while on it. I
plan to post a patch to fix the error message and parameter soon.

Best regards,
Hou zj

  7. RE: A doubt about a newly added errdetail

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-09-26T11:15:48Z

    On Monday, September 26, 2022 5:03 PM houzj.fnst@fujitsu.com wrote:
> 
> On Monday, September 26, 2022 4:57 PM Amit Kapila
> <amit.kapila16@gmail.com>
> >
> > On Mon, Sep 26, 2022 at 2:03 PM Alvaro Herrera
> > <alvherre@alvh.no-ip.org>
> > wrote:
> > >
> > > On 2022-Sep-26, Amit Kapila wrote:
> > >
> > > > On Mon, Sep 26, 2022 at 1:10 PM Alvaro Herrera
> > <alvherre@alvh.no-ip.org> wrote:
> > >
> > > > > ERROR:  cannot use column list for relation "%s.%s" in publication "%s"
> > > > > DETAIL:  Column lists cannot be specified in publications
> > > > > containing FOR
> > TABLES IN SCHEMA elements.
> > > >
> > > > This looks mostly good to me. BTW, is it a good idea to add ".. in
> > > > publication "%s"" to the error message as this can happen even
> > > > during create publication?
> > >
> > > Hmm, I don't see why not.  The publication is new, sure, but it
> > > would already have a name, so there's no possible confusion as to
> > > what this means.
> > >
> > > (My main change was to take the word "publication" out of the phrase
> > > "publication column list", because that seemed a bit strange; it
> > > isn't the publication that has a column list, but the relation.)
> > >
> >
> > Okay, that makes sense.
> 
> +1
> 
> > >
> > > > If so, I think we can change the nearby message as below to
> > > > include the same:
> > > >
> > > > if (!pubviaroot &&	
> > > > pri->relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > > > ereport(ERROR,
> > > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > > errmsg("cannot use publication column list for relation \"%s\"",
> > >
> > > WFM.
> > >
> >
> > Okay.
> 
> While reviewing this part, I notice an unused parameter(queryString) of function
> CheckPubRelationColumnList. I feel we can remove that as well while on it. I plan
> to post a patch to fix the error message and parameter soon.
> 

Attach the patch. (The patch can apply on both HEAD and PG15)

Best regards,
Hou zj

  8. Re: A doubt about a newly added errdetail

    Amit Kapila <amit.kapila16@gmail.com> — 2022-09-26T12:03:46Z

    On Mon, Sep 26, 2022 at 4:45 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
>
> Attach the patch. (The patch can apply on both HEAD and PG15)
>

The patch looks good to me.

*
- errmsg("cannot add schema to the publication"),
+ errmsg("cannot add schema to publication \"%s\"",
+    stmt->pubname),

I see that you have improved an additional message in the patch which
appears okay to me.

-- 
With Regards,
Amit Kapila.

  9. Re: A doubt about a newly added errdetail

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-27T01:40:49Z

    At Mon, 26 Sep 2022 17:33:46 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Mon, Sep 26, 2022 at 4:45 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > Attach the patch. (The patch can apply on both HEAD and PG15)
> >
> 
> The patch looks good to me.
> 
> *
> - errmsg("cannot add schema to the publication"),
> + errmsg("cannot add schema to publication \"%s\"",
> +    stmt->pubname),
> 
> I see that you have improved an additional message in the patch which
> appears okay to me.

Overall +1 from me, thanks!

By the way, this is not an issue caused by the proposed patch, I see
the following message in the patch.

-					 errdetail("Column list cannot be used for a partitioned table when %s is false.",
+					 errdetail("Column list cannot be specified for a partitioned table when %s is false.",
 							   "publish_via_partition_root")));

I think that the purpose of such separation of variable names is to
unify multiple messages differing only by the names (to keep
translation labor (relatively:p) low). In that sense, that separation
here is useless since I see no chance of having the same message with
another variable in future.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-27T07:21:05Z

    On 2022-Sep-27, Kyotaro Horiguchi wrote:

> By the way, this is not an issue caused by the proposed patch, I see
> the following message in the patch.
> 
> -					 errdetail("Column list cannot be used for a partitioned table when %s is false.",
> +					 errdetail("Column list cannot be specified for a partitioned table when %s is false.",
>  							   "publish_via_partition_root")));
> 
> I think that the purpose of such separation of variable names is to
> unify multiple messages differing only by the names (to keep
> translation labor (relatively:p) low). In that sense, that separation
> here is useless since I see no chance of having the same message with
> another variable in future.

Well, it also reduces chances for typos and such, so while it's not
strictly necessary to do it this way, I tend to prefer it on new
messages.  However, as you say it's not very interesting when there's no
possibility of duplication, so changing existing messages to this style
when we have no other reason to change the message, is not a useful use
of time.  In this case we're changing the message in another way too, so
I think it's okay.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

  11. RE: A doubt about a newly added errdetail

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2022-09-27T09:28:18Z

    On Tuesday, September 27, 2022 3:21 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 
> On 2022-Sep-27, Kyotaro Horiguchi wrote:
> 
> > By the way, this is not an issue caused by the proposed patch, I see
> > the following message in the patch.
> >
> > -					 errdetail("Column list cannot be used
> for a partitioned table when %s is false.",
> > +					 errdetail("Column list cannot be
> specified for a partitioned
> > +table when %s is false.",
> >
> "publish_via_partition_root")));
> >
> > I think that the purpose of such separation of variable names is to
> > unify multiple messages differing only by the names (to keep
> > translation labor (relatively:p) low). In that sense, that separation
> > here is useless since I see no chance of having the same message with
> > another variable in future.
> 
> Well, it also reduces chances for typos and such, so while it's not strictly
> necessary to do it this way, I tend to prefer it on new messages.  However, as
> you say it's not very interesting when there's no possibility of duplication, so
> changing existing messages to this style when we have no other reason to
> change the message, is not a useful use of time.  In this case we're changing
> the message in another way too, so I think it's okay.

Thanks for reviewing!

Just in case I misunderstand, it seems you mean the message style[1] is OK, right ?

[1]
errdetail("Column list cannot be specified for a partitioned table when %s is false.",
	 "publish_via_partition_root")));

Best regards,
Hou zj

  12. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-27T10:19:35Z

    On 2022-Sep-27, houzj.fnst@fujitsu.com wrote:

> Thanks for reviewing!
> 
> Just in case I misunderstand, it seems you mean the message style[1] is OK, right ?
> 
> [1]
> errdetail("Column list cannot be specified for a partitioned table when %s is false.",
> 	 "publish_via_partition_root")));

Yeah, since you're changing another word in that line, it's ok to move
the parameter line off-string.  (If you were only changing the parameter
to %s and there was no message duplication, I would reject the patch as
useless.)

I'm going over that patch now, I have a few other changes as attached,
intend to commit soon.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)

  13. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-27T12:42:49Z

    While reading this code, I noticed that function expr_allowed_in_node()
has a very strange API: it doesn't have any return convention at all
other than "if we didn't modify errdetail_str then all is good".  I was
tempted to add an "Assert(*errdetail_msg == NULL)" at the start of it,
just to make sure that it is not called if a message is already set.

I think it would be much saner to inline the few lines of that function
in its sole caller, as in the attached.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)

  14. Re: A doubt about a newly added errdetail

    Amit Kapila <amit.kapila16@gmail.com> — 2022-09-28T03:28:56Z

    On Tue, Sep 27, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> While reading this code, I noticed that function expr_allowed_in_node()
> has a very strange API: it doesn't have any return convention at all
> other than "if we didn't modify errdetail_str then all is good".  I was
> tempted to add an "Assert(*errdetail_msg == NULL)" at the start of it,
> just to make sure that it is not called if a message is already set.
>
> I think it would be much saner to inline the few lines of that function
> in its sole caller, as in the attached.
>

LGTM.

-- 
With Regards,
Amit Kapila.

  15. Re: A doubt about a newly added errdetail

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-28T06:00:34Z

    At Tue, 27 Sep 2022 12:19:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> Yeah, since you're changing another word in that line, it's ok to move
> the parameter line off-string.  (If you were only changing the parameter
> to %s and there was no message duplication, I would reject the patch as
> useless.)

I'm fine with that. By the way, related to the area, I found the
following error messages.

>	 errmsg("publication \"%s\" is defined as FOR ALL TABLES",
>			NameStr(pubform->pubname)),
>	 errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES publications.")));

It looks tome that the errmsg and errordetail are reversed. Isn't the following order common?

>	 errmsg("schemas cannot be added to or dropped from publication \"%s\".",
>			NameStr(pubform->pubname)),
>	 errdetail("The publication is defined as FOR ALL TABLES.")));

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: A doubt about a newly added errdetail

    Amit Kapila <amit.kapila16@gmail.com> — 2022-09-28T08:17:25Z

    On Wed, Sep 28, 2022 at 11:30 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Tue, 27 Sep 2022 12:19:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in
> > Yeah, since you're changing another word in that line, it's ok to move
> > the parameter line off-string.  (If you were only changing the parameter
> > to %s and there was no message duplication, I would reject the patch as
> > useless.)
>
> I'm fine with that. By the way, related to the area, I found the
> following error messages.
>
> >        errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> >                       NameStr(pubform->pubname)),
> >        errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES publications.")));
>
> It looks tome that the errmsg and errordetail are reversed. Isn't the following order common?
>
> >        errmsg("schemas cannot be added to or dropped from publication \"%s\".",
> >                       NameStr(pubform->pubname)),
> >        errdetail("The publication is defined as FOR ALL TABLES.")));
>

This one seems to be matching with the below existing message:
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("publication \"%s\" is defined as FOR ALL TABLES",
NameStr(pubform->pubname)),
errdetail("Tables cannot be added to or dropped from FOR ALL TABLES
publications.")));


-- 
With Regards,
Amit Kapila.

  17. Re: A doubt about a newly added errdetail

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-09-28T08:46:41Z

    On 2022-Sep-28, Amit Kapila wrote:

> On Wed, Sep 28, 2022 at 11:30 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:

> > It looks tome that the errmsg and errordetail are reversed. Isn't the following order common?
> >
> > >        errmsg("schemas cannot be added to or dropped from publication \"%s\".",
> > >                       NameStr(pubform->pubname)),
> > >        errdetail("The publication is defined as FOR ALL TABLES.")));
> >
> 
> This one seems to be matching with the below existing message:
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> NameStr(pubform->pubname)),
> errdetail("Tables cannot be added to or dropped from FOR ALL TABLES
> publications.")));

Well, that suggests we should change both together.  I do agree that
they look suspicious; they should be more similar to this other one, I
think:

                ereport(ERROR,
                        errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                        errmsg("cannot add schema to publication \"%s\"",
                               stmt->pubname),
                        errdetail("Schemas cannot be added if any tables that specify a column list are already part of the publication."));

The errcodes appear not to agree with each other, also.  Maybe that
needs some more thought as well.  I don't think INVALID_PARAMETER_VALUE
is the right thing here, and I'm not sure about
OBJECT_NOT_IN_PREREQUISITE_STATE either.


FWIW, the latter is a whole category which is not defined by the SQL
standard, so I recall Tom got it from DB2.  DB2 chose to subdivide in a
lot of different cases, see
https://www.ibm.com/docs/en/db2/9.7?topic=messages-sqlstate#rsttmsg__code55
for a (current?) table.  Maybe we should define some additional 55Pxx
values -- say 55PR1 INCOMPATIBLE PUBLICATION DEFINITION (R for
"replication"-related matters; the P is what we chose for the
Postgres-specific subcategory).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

  18. Re: A doubt about a newly added errdetail

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-28T09:22:32Z

    At Wed, 28 Sep 2022 13:47:25 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Wed, Sep 28, 2022 at 11:30 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > I'm fine with that. By the way, related to the area, I found the
> > following error messages.
> >
> > >        errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> > >                       NameStr(pubform->pubname)),
> > >        errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES publications.")));
> >
> > It looks tome that the errmsg and errordetail are reversed. Isn't the following order common?
> >
> > >        errmsg("schemas cannot be added to or dropped from publication \"%s\".",
> > >                       NameStr(pubform->pubname)),
> > >        errdetail("The publication is defined as FOR ALL TABLES.")));
> >
> 
> This one seems to be matching with the below existing message:
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> NameStr(pubform->pubname)),
> errdetail("Tables cannot be added to or dropped from FOR ALL TABLES
> publications.")));

Yeah, so I meant that I'd like to propose to chage the both.  I just
wanted to ask people whether that proposal is reasonable or not.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  19. Re: A doubt about a newly added errdetail

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-28T09:41:10Z

    At Wed, 28 Sep 2022 10:46:41 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> On 2022-Sep-28, Amit Kapila wrote:
> 
> > On Wed, Sep 28, 2022 at 11:30 AM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> 
> > > It looks tome that the errmsg and errordetail are reversed. Isn't the following order common?
> > >
> > > >        errmsg("schemas cannot be added to or dropped from publication \"%s\".",
> > > >                       NameStr(pubform->pubname)),
> > > >        errdetail("The publication is defined as FOR ALL TABLES.")));
> > >
> > 
> > This one seems to be matching with the below existing message:
> > ereport(ERROR,
> > (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> > NameStr(pubform->pubname)),
> > errdetail("Tables cannot be added to or dropped from FOR ALL TABLES
> > publications.")));
> 
> Well, that suggests we should change both together.  I do agree that
> they look suspicious; they should be more similar to this other one, I
> think:

Ah, yes, and thanks.

>                 ereport(ERROR,
>                         errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                         errmsg("cannot add schema to publication \"%s\"",
>                                stmt->pubname),
>                         errdetail("Schemas cannot be added if any tables that specify a column list are already part of the publication."));
> 
> The errcodes appear not to agree with each other, also.  Maybe that
> needs some more thought as well.  I don't think INVALID_PARAMETER_VALUE
> is the right thing here, and I'm not sure about
> OBJECT_NOT_IN_PREREQUISITE_STATE either.

The latter seems to fit better than the current one. That being said
if we change the SQLSTATE for exising erros, that may make existing
users confused.

> FWIW, the latter is a whole category which is not defined by the SQL
> standard, so I recall Tom got it from DB2.  DB2 chose to subdivide in a
> lot of different cases, see
> https://www.ibm.com/docs/en/db2/9.7?topic=messages-sqlstate#rsttmsg__code55
> for a (current?) table.  Maybe we should define some additional 55Pxx
> values -- say 55PR1 INCOMPATIBLE PUBLICATION DEFINITION (R for
> "replication"-related matters; the P is what we chose for the
> Postgres-specific subcategory).

I generally agree to this. But we don't have enough time to fully
consider that?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center