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]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 usingpublisher 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 usingpublisher 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
Attachments:
v1-0001-Fix-quotation-of-variable-names.patchapplication/octet-stream; name=v1-0001-Fix-quotation-of-variable-names.patchDownload
From d15d7d9a33bfec7c4764fe9601822d69dd386b5e Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.malik@gmail.com>
Date: Wed, 14 Feb 2024 17:29:25 +0530
Subject: [PATCH v1] Fix quotation of variable names.
This patch corrects the usage of double quotes for variable
names and values added by commit ddd5f4f54a.
---
src/backend/replication/logical/slotsync.c | 29 +++++++++----------
.../t/040_standby_failover_slots_sync.pl | 2 +-
2 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 0aa1bf1ad2..5e82c02484 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -729,12 +729,12 @@ validate_remote_info(WalReceiverConn *wrconn)
ereport(ERROR,
errmsg("could not fetch primary_slot_name \"%s\" info from the primary server: %s",
PrimarySlotName, res->err),
- errhint("Check if \"primary_slot_name\" is configured correctly."));
+ errhint("Check if primary_slot_name is configured correctly."));
tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
elog(ERROR,
- "failed to fetch tuple for the primary server slot specified by \"primary_slot_name\"");
+ "failed to fetch tuple for the primary server slot specified by primary_slot_name");
remote_in_recovery = DatumGetBool(slot_getattr(tupslot, 1, &isnull));
Assert(!isnull);
@@ -750,9 +750,9 @@ validate_remote_info(WalReceiverConn *wrconn)
if (!primary_slot_valid)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
+ errmsg("slot synchronization requires valid primary_slot_name"),
/* translator: second %s is a GUC variable name */
- errdetail("The replication slot \"%s\" specified by \"%s\" does not exist on the primary server.",
+ errdetail("The replication slot \"%s\" specified by %s does not exist on the primary server.",
PrimarySlotName, "primary_slot_name"));
ExecClearTuple(tupslot);
@@ -778,8 +778,7 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be defined.", "primary_slot_name"));
+ errmsg("slot synchronization requires %s to be defined", "primary_slot_name"));
/*
* hot_standby_feedback must be enabled to cooperate with the physical
@@ -790,15 +789,14 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be enabled.", "hot_standby_feedback"));
+ errmsg("slot synchronization requires %s to be enabled",
+ "hot_standby_feedback"));
/* Logical slot sync/creation requires wal_level >= logical. */
if (wal_level < WAL_LEVEL_LOGICAL)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"wal_level\" must be >= logical."));
+ errmsg("slot synchronization requires wal_level >= logical"));
/*
* The primary_conninfo is required to make connection to primary for
@@ -808,8 +806,8 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be defined.", "primary_conninfo"));
+ errmsg("slot synchronization requires %s to be defined",
+ "primary_conninfo"));
/*
* The slot synchronization needs a database connection for walrcv_exec to
@@ -820,12 +818,11 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/*
- * translator: 'dbname' is a specific option; %s is a GUC variable
- * name
+ * translator: dbname is a specific option; %s is a GUC variable name
*/
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("'dbname' must be specified in \"%s\".", "primary_conninfo"));
+ errmsg("slot synchronization requires dbname to be specified in %s",
+ "primary_conninfo"));
}
/*
diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl
index 9634a50b3e..e9b66a5a41 100644
--- a/src/test/recovery/t/040_standby_failover_slots_sync.pl
+++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl
@@ -305,7 +305,7 @@ $standby1->reload;
($result, $stdout, $stderr) =
$standby1->psql('postgres', "SELECT pg_sync_replication_slots();");
ok( $stderr =~
- /HINT: 'dbname' must be specified in "primary_conninfo"/,
+ /ERROR: slot synchronization requires dbname to be specified in primary_conninfo/,
"cannot sync slots if dbname is not specified in primary_conninfo");
##################################################
--
2.34.1
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 usingpublisher 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 namesThanks 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 namesThanks 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 namesThanks 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
Attachments:
v2-0001-Fix-quotation-of-variable-names.patchapplication/octet-stream; name=v2-0001-Fix-quotation-of-variable-names.patchDownload
From 15d6d7571168d2a532b06552fed0454077fc2e0f Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.malik@gmail.com>
Date: Wed, 14 Feb 2024 17:29:25 +0530
Subject: [PATCH v2] Fix quotation of variable names.
This patch corrects the usage of double quotes for variable
names and values added by commit ddd5f4f54a.
---
src/backend/replication/logical/slotsync.c | 29 +++++++++----------
.../t/040_standby_failover_slots_sync.pl | 2 +-
2 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 4cab7b7101..8196e25bfd 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -743,12 +743,12 @@ validate_remote_info(WalReceiverConn *wrconn)
ereport(ERROR,
errmsg("could not fetch primary_slot_name \"%s\" info from the primary server: %s",
PrimarySlotName, res->err),
- errhint("Check if \"primary_slot_name\" is configured correctly."));
+ errhint("Check if primary_slot_name is configured correctly."));
tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
elog(ERROR,
- "failed to fetch tuple for the primary server slot specified by \"primary_slot_name\"");
+ "failed to fetch tuple for the primary server slot specified by primary_slot_name");
remote_in_recovery = DatumGetBool(slot_getattr(tupslot, 1, &isnull));
Assert(!isnull);
@@ -764,9 +764,9 @@ validate_remote_info(WalReceiverConn *wrconn)
if (!primary_slot_valid)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
+ errmsg("slot synchronization requires valid primary_slot_name"),
/* translator: second %s is a GUC variable name */
- errdetail("The replication slot \"%s\" specified by \"%s\" does not exist on the primary server.",
+ errdetail("The replication slot \"%s\" specified by %s does not exist on the primary server.",
PrimarySlotName, "primary_slot_name"));
ExecClearTuple(tupslot);
@@ -792,8 +792,7 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be defined.", "primary_slot_name"));
+ errmsg("slot synchronization requires %s to be defined", "primary_slot_name"));
/*
* hot_standby_feedback must be enabled to cooperate with the physical
@@ -804,15 +803,14 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be enabled.", "hot_standby_feedback"));
+ errmsg("slot synchronization requires %s to be enabled",
+ "hot_standby_feedback"));
/* Logical slot sync/creation requires wal_level >= logical. */
if (wal_level < WAL_LEVEL_LOGICAL)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"wal_level\" must be >= logical."));
+ errmsg("slot synchronization requires wal_level >= \"logical\""));
/*
* The primary_conninfo is required to make connection to primary for
@@ -822,8 +820,8 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/* translator: %s is a GUC variable name */
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("\"%s\" must be defined.", "primary_conninfo"));
+ errmsg("slot synchronization requires %s to be defined",
+ "primary_conninfo"));
/*
* The slot synchronization needs a database connection for walrcv_exec to
@@ -834,12 +832,11 @@ ValidateSlotSyncParams(void)
ereport(ERROR,
/*
- * translator: 'dbname' is a specific option; %s is a GUC variable
- * name
+ * translator: dbname is a specific option; %s is a GUC variable name
*/
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("bad configuration for slot synchronization"),
- errhint("'dbname' must be specified in \"%s\".", "primary_conninfo"));
+ errmsg("slot synchronization requires dbname to be specified in %s",
+ "primary_conninfo"));
}
/*
diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl
index 2755c3fc84..0f2f819f53 100644
--- a/src/test/recovery/t/040_standby_failover_slots_sync.pl
+++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl
@@ -319,7 +319,7 @@ $standby1->reload;
($result, $stdout, $stderr) =
$standby1->psql('postgres', "SELECT pg_sync_replication_slots();");
ok( $stderr =~
- /HINT: 'dbname' must be specified in "primary_conninfo"/,
+ /ERROR: slot synchronization requires dbname to be specified in primary_conninfo/,
"cannot sync slots if dbname is not specified in primary_conninfo");
##################################################
--
2.34.1
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