Thread: A doubt about a newly added errdetail
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
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/
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.
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/
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.
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
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
Attachment
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.
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
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)
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
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)
Attachment
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)
Attachment
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.
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
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.
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
thepublication."));
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)
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
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
ofthe 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