Horton

Thread

Commits

  1. Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48.

  1. About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-14T07:26:52Z

    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

  2. Re: About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-14T07:34:37Z

    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

  3. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-14T08:46:29Z

    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.

  4. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-14T11:13:32Z

    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.

  5. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-14T11:45:37Z

    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.

  6. Re: About a recently-added message

    Euler Taveira <euler@eulerto.com> — 2024-02-14T14:20:46Z

    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


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  7. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-15T02:56:18Z

    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.

  8. Re: About a recently-added message

    shveta malik <shveta.malik@gmail.com> — 2024-02-15T03:52:23Z

    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

  9. Re: About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-15T06:19:30Z

    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

  10. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-19T05:40:38Z

    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.

  11. Re: About a recently-added message

    shveta malik <shveta.malik@gmail.com> — 2024-02-19T05:56:44Z

    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

  12. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-20T08:43:41Z

    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.

  13. Re: About a recently-added message

    shveta malik <shveta.malik@gmail.com> — 2024-02-20T09:50:48Z

    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

  14. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-21T09:27:42Z

    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.

  15. Re: About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-22T00:36:43Z

    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.c122:	errmsg("logical decoding requires wal_level >= logical")));

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-22T00:46:36Z

    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

  17. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-22T05:21:07Z

    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.

  18. Re: About a recently-added message

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-22T05:40:05Z

    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

  19. Re: About a recently-added message

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-22T10:52:55Z

    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.

  20. Re: About a recently-added message

    Peter Smith <smithpb2250@gmail.com> — 2024-02-23T04:46:38Z

    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