About a recently-added message

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]https://www.postgresql.org/docs/devel/error-style-guide.html (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]/messages/by-id/CAHut+Psf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA@mail.gmail.com. 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]: /messages/by-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 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/

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

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

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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]/messages/by-id/CAHut+Psf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA@mail.gmail.com. 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]: /messages/by-id/CAHut+Psf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia