Thread: About a recently-added message
A recent commit added the following message: > "wal_level" must be >= logical. The use of the term "logical" here is a bit confusing, as it's unclear whether it's meant to be a natural language word or a token. (I believe it to be a token.) On the contrary, we already have the following message: > wal_level must be set to "replica" or "logical" at server start. This has the conflicting policy about quotation of variable names and enum values. I suggest making the quoting policy consistent. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 14 Feb 2024 16:26:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > "wal_level" must be >= logical. .. > > wal_level must be set to "replica" or "logical" at server start. .. > I suggest making the quoting policy consistent. Just after this, I found another inconsistency regarding quotation. > 'dbname' must be specified in "%s". The use of single quotes doesn't seem to comply with our standard. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Feb 14, 2024 at 1:04 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Wed, 14 Feb 2024 16:26:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > "wal_level" must be >= logical. > .. > > > wal_level must be set to "replica" or "logical" at server start. > .. > > I suggest making the quoting policy consistent. > > Just after this, I found another inconsistency regarding quotation. > > > 'dbname' must be specified in "%s". > > The use of single quotes doesn't seem to comply with our standard. > Thanks for the report. I'll look into it after analyzing the BF failure caused by the same commit. -- With Regards, Amit Kapila.
On Wed, Feb 14, 2024 at 1:04 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Just after this, I found another inconsistency regarding quotation.
>
> > 'dbname' must be specified in "%s".
>
> The use of single quotes doesn't seem to comply with our standard.
>
Agreed, I think we have two choices here one is to use dbname without
any quotes ("dbname must be specified in \"%s\".", ...)) or use double
quotes ("\"%s\" must be specified in \"%s\".", "dbname" ...)). I see
messages like: "host name must be specified", so if we want to follow
that earlier makes more sense. Any suggestions?
--
With Regards,
Amit Kapila.
On Wed, Feb 14, 2024 at 12:57 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > A recent commit added the following message: > > > "wal_level" must be >= logical. > > The use of the term "logical" here is a bit confusing, as it's unclear > whether it's meant to be a natural language word or a token. (I > believe it to be a token.) > > On the contrary, we already have the following message: > > > wal_level must be set to "replica" or "logical" at server start. > > This has the conflicting policy about quotation of variable names and > enum values. > > I suggest making the quoting policy consistent. > As per docs [1] (In messages containing configuration variable names, do not include quotes when the names are visibly not natural English words, such as when they have underscores, are all-uppercase, or have mixed case. Otherwise, quotes must be added. Do include quotes in a message where an arbitrary variable name is to be expanded.), I think in this case GUC's name shouldn't have been quoted. I think the patch did this because it's developed parallel to a thread where we were discussing whether to quote GUC names or not [2]. I think it is better not to do as per docs till we get any further clarification. Now, I am less clear about whether to quote "logical" or not in the above message. Do you have any suggestions? [1] - https://www.postgresql.org/docs/devel/error-style-guide.html [2] - https://www.postgresql.org/message-id/CAHut+Psf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA@mail.gmail.com -- With Regards, Amit Kapila.
On Wed, Feb 14, 2024, at 8:45 AM, Amit Kapila wrote:
Now, I am less clear about whether to quote "logical" or not in theabove message. Do you have any suggestions?
The possible confusion comes from the fact that the sentence contains "must be"
in the middle of a comparison expression. For pg_createsubscriber, we are using
publisher requires wal_level >= logical
I suggest to use something like
slot synchronization requires wal_level >= logical
On Wed, Feb 14, 2024 at 7:51 PM Euler Taveira <euler@eulerto.com> wrote: > > On Wed, Feb 14, 2024, at 8:45 AM, Amit Kapila wrote: > > Now, I am less clear about whether to quote "logical" or not in the > above message. Do you have any suggestions? > > > The possible confusion comes from the fact that the sentence contains "must be" > in the middle of a comparison expression. For pg_createsubscriber, we are using > > publisher requires wal_level >= logical > > I suggest to use something like > > slot synchronization requires wal_level >= logical > This sounds like a better idea but shouldn't we directly use this in 'errmsg' instead of a separate 'errhint'? Also, if change this, then we should make a similar change for other messages in the same function. -- With Regards, Amit Kapila.
On Thu, Feb 15, 2024 at 8:26 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Feb 14, 2024 at 7:51 PM Euler Taveira <euler@eulerto.com> wrote: > > > > On Wed, Feb 14, 2024, at 8:45 AM, Amit Kapila wrote: > > > > Now, I am less clear about whether to quote "logical" or not in the > > above message. Do you have any suggestions? > > > > > > The possible confusion comes from the fact that the sentence contains "must be" > > in the middle of a comparison expression. For pg_createsubscriber, we are using > > > > publisher requires wal_level >= logical > > > > I suggest to use something like > > > > slot synchronization requires wal_level >= logical > > > > This sounds like a better idea but shouldn't we directly use this in > 'errmsg' instead of a separate 'errhint'? Also, if change this, then > we should make a similar change for other messages in the same > function. +1 on changing the msg(s) suggested way. Please find the patch for the same. It also removes double quotes around the variable names thanks Shveta
Attachment
At Thu, 15 Feb 2024 09:22:23 +0530, shveta malik <shveta.malik@gmail.com> wrote in > On Thu, Feb 15, 2024 at 8:26 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Wed, Feb 14, 2024 at 7:51 PM Euler Taveira <euler@eulerto.com> wrote: > > > > > > On Wed, Feb 14, 2024, at 8:45 AM, Amit Kapila wrote: > > > > > > Now, I am less clear about whether to quote "logical" or not in the > > > above message. Do you have any suggestions? > > > > > > > > > The possible confusion comes from the fact that the sentence contains "must be" > > > in the middle of a comparison expression. For pg_createsubscriber, we are using > > > > > > publisher requires wal_level >= logical > > > > > > I suggest to use something like > > > > > > slot synchronization requires wal_level >= logical > > > > > > > This sounds like a better idea but shouldn't we directly use this in > > 'errmsg' instead of a separate 'errhint'? Also, if change this, then > > we should make a similar change for other messages in the same > > function. > > +1 on changing the msg(s) suggested way. Please find the patch for the > same. It also removes double quotes around the variable names Thanks for the discussion. With a translator hat on, I would be happy if I could determine whether a word requires translation with minimal background information. In this case, a translator needs to know which values wal_level can take. It's relatively easy in this case, but I'm not sure if this is always the case. Therefore, I would be slightly happier if "logical" were double-quoted. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Feb 15, 2024 at 11:49 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 15 Feb 2024 09:22:23 +0530, shveta malik <shveta.malik@gmail.com> wrote in > > > > +1 on changing the msg(s) suggested way. Please find the patch for the > > same. It also removes double quotes around the variable names > > Thanks for the discussion. > > With a translator hat on, I would be happy if I could determine > whether a word requires translation with minimal background > information. In this case, a translator needs to know which values > wal_level can take. It's relatively easy in this case, but I'm not > sure if this is always the case. Therefore, I would be slightly > happier if "logical" were double-quoted. > I see that we use "logical" in double quotes in various error messages. For example: "wal_level must be set to \"replica\" or \"logical\" at server start". So following that we can use the double quotes here as well. -- With Regards, Amit Kapila.
On Mon, Feb 19, 2024 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 15, 2024 at 11:49 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Thu, 15 Feb 2024 09:22:23 +0530, shveta malik <shveta.malik@gmail.com> wrote in
> > >
> > > +1 on changing the msg(s) suggested way. Please find the patch for the
> > > same. It also removes double quotes around the variable names
> >
> > Thanks for the discussion.
> >
> > With a translator hat on, I would be happy if I could determine
> > whether a word requires translation with minimal background
> > information. In this case, a translator needs to know which values
> > wal_level can take. It's relatively easy in this case, but I'm not
> > sure if this is always the case. Therefore, I would be slightly
> > happier if "logical" were double-quoted.
> >
>
> I see that we use "logical" in double quotes in various error
> messages. For example: "wal_level must be set to \"replica\" or
> \"logical\" at server start". So following that we can use the double
> quotes here as well.
Okay, now since we will have double quotes for logical. So do you
prefer the existing way of giving error msg or the changed one.
Existing:
errmsg("bad configuration for slot synchronization"),
errhint("wal_level must be >= logical."));
errmsg("bad configuration for slot synchronization"),
errhint("%s must be defined.", "primary_conninfo"));
The changed one:
errmsg("slot synchronization requires wal_level >= logical"));
errmsg("slot synchronization requires %s to be defined",
"primary_conninfo"));
thanks
Shveta
On Mon, Feb 19, 2024 at 11:26 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Feb 19, 2024 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Feb 15, 2024 at 11:49 AM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > At Thu, 15 Feb 2024 09:22:23 +0530, shveta malik <shveta.malik@gmail.com> wrote in
> > > >
> > > > +1 on changing the msg(s) suggested way. Please find the patch for the
> > > > same. It also removes double quotes around the variable names
> > >
> > > Thanks for the discussion.
> > >
> > > With a translator hat on, I would be happy if I could determine
> > > whether a word requires translation with minimal background
> > > information. In this case, a translator needs to know which values
> > > wal_level can take. It's relatively easy in this case, but I'm not
> > > sure if this is always the case. Therefore, I would be slightly
> > > happier if "logical" were double-quoted.
> > >
> >
> > I see that we use "logical" in double quotes in various error
> > messages. For example: "wal_level must be set to \"replica\" or
> > \"logical\" at server start". So following that we can use the double
> > quotes here as well.
>
> Okay, now since we will have double quotes for logical. So do you
> prefer the existing way of giving error msg or the changed one.
>
> Existing:
> errmsg("bad configuration for slot synchronization"),
> errhint("wal_level must be >= logical."));
>
> errmsg("bad configuration for slot synchronization"),
> errhint("%s must be defined.", "primary_conninfo"));
>
> The changed one:
> errmsg("slot synchronization requires wal_level >= logical"));
>
> errmsg("slot synchronization requires %s to be defined",
> "primary_conninfo"));
>
I would prefer the changed ones as those clearly explain the problem
without additional information.
--
With Regards,
Amit Kapila.
On Tue, Feb 20, 2024 at 2:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > I would prefer the changed ones as those clearly explain the problem > without additional information. okay, attached v2 patch with changed error msgs and double quotes around logical. thanks Shveta
Attachment
On Tue, Feb 20, 2024 at 3:21 PM shveta malik <shveta.malik@gmail.com> wrote: > > okay, attached v2 patch with changed error msgs and double quotes > around logical. > Horiguchi-San, does this address all your concerns related to translation with these new messages? -- With Regards, Amit Kapila.
At Wed, 21 Feb 2024 14:57:42 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in > On Tue, Feb 20, 2024 at 3:21 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > okay, attached v2 patch with changed error msgs and double quotes > > around logical. > > > > Horiguchi-San, does this address all your concerns related to > translation with these new messages? Yes, I'm happy with all of the changes. The proposed patch appears to cover all instances related to slotsync.c, and it looks fine to me. Thanks! I found that logica.c is also using the policy that I complained about, but it is a separate issue. ./logical.c
At Thu, 22 Feb 2024 09:36:43 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > Yes, I'm happy with all of the changes. The proposed patch appears to > cover all instances related to slotsync.c, and it looks fine to > me. Thanks! I'd like to raise another potential issue outside the patch. The patch needed to change only one test item even though it changed nine messages. This means eigh out of nine messages that the patch changed are not covered by our test. I doubt all of them are worth additional test items; however, I think we want to increase coverage. Do you think some additional tests for the rest of the messages are worth the trouble? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Feb 22, 2024 at 6:16 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 22 Feb 2024 09:36:43 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > Yes, I'm happy with all of the changes. The proposed patch appears to > > cover all instances related to slotsync.c, and it looks fine to > > me. Thanks! > > I'd like to raise another potential issue outside the patch. The patch > needed to change only one test item even though it changed nine > messages. This means eigh out of nine messages that the patch changed > are not covered by our test. I doubt all of them are worth additional > test items; however, I think we want to increase coverage. > > Do you think some additional tests for the rest of the messages are > worth the trouble? > We have discussed this during development and didn't find it worth adding tests for all misconfigured parameters. However, in the next patch where we are planning to add a slot sync worker that will automatically sync slots, we are adding a test for one more parameter. I am not against adding tests for all the parameters but it didn't appeal to add more test cycles for this. -- With Regards, Amit Kapila.
At Thu, 22 Feb 2024 10:51:07 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in > > Do you think some additional tests for the rest of the messages are > > worth the trouble? > > > > We have discussed this during development and didn't find it worth > adding tests for all misconfigured parameters. However, in the next > patch where we are planning to add a slot sync worker that will > automatically sync slots, we are adding a test for one more parameter. > I am not against adding tests for all the parameters but it didn't > appeal to add more test cycles for this. Thanks for the explanation. I'm fine with that. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Feb 22, 2024 at 11:10 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 22 Feb 2024 10:51:07 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in > > > Do you think some additional tests for the rest of the messages are > > > worth the trouble? > > > > > > > We have discussed this during development and didn't find it worth > > adding tests for all misconfigured parameters. However, in the next > > patch where we are planning to add a slot sync worker that will > > automatically sync slots, we are adding a test for one more parameter. > > I am not against adding tests for all the parameters but it didn't > > appeal to add more test cycles for this. > > Thanks for the explanation. I'm fine with that. > Pushed. -- With Regards, Amit Kapila.
On Thu, Feb 22, 2024 at 11:36 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 21 Feb 2024 14:57:42 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> > On Tue, Feb 20, 2024 at 3:21 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > okay, attached v2 patch with changed error msgs and double quotes
> > > around logical.
> > >
> >
> > Horiguchi-San, does this address all your concerns related to
> > translation with these new messages?
>
> Yes, I'm happy with all of the changes. The proposed patch appears to
> cover all instances related to slotsync.c, and it looks fine to
> me. Thanks!
>
> I found that logica.c is also using the policy that I complained
> about, but it is a separate issue.
>
> ./logical.c 122: errmsg("logical decoding requires wal_level >= logical")));
>
Hmm. I have a currently stalled patch-set to simplify the quoting of
all the GUC names by using one rule. The consensus is to *always*
quote them. See [1]. And those patches already are addressing that
logical.c code mentioned above.
IMO it would be good if we could try to get that patch set approved to
fix everything in one go, instead of ad-hoc hunting for and fixing
cases one at a time.
======
[1] https://www.postgresql.org/message-id/CAHut%2BPsf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia