Re: pgsql: With gssencmode='require', check credential cache before connect
At Tue, 9 Apr 2024 08:14:53 +0300, Heikki Linnakangas wrote in > On 09/04/2024 04:46, Kyotaro Horiguchi wrote: > > Hello. > > At Sun, 07 Apr 2024 23:50:08 +, Heikki Linnakangas > > wrote in > >> With gssencmode='require', check credential cache before connecting > > This commit adds the following error message (indentations are > > adjusted): > > + libpq_append_conn_error(conn, > > + "GSSAPI encryption required but it is not supported over a local > > socket)"); > > The closing parenthesis at the end of the message seems to be a > > leftover from editing. > > Fixed, thanks! > > > About the following message: > > + libpq_append_conn_error(conn, "could not set ssl alpn extension: > > %s", err); > > I'm not sure about the policy for writing acronyms in lowercase, but > > other occurrences of ALPN (in backend code) seem to be written in > > uppercase. > > Changed to uppercase. I also changed "ssl" to uppercase, for > consistency with the "could not set SSL Server Name Indication (SNI)" > message earlier. (I didn't consider SSL..) > To be even more consistent, we should perhaps spell out "SSL > Application-Layer Protocol Negotiation (ALPN)", but that's pretty long > and I don't think it really helps the user. It really should not fail, > and there isn't anything the user can really do if that fails. Anyone > who doesn't already know what ALPN is will need to google it anyway. I think so, too. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: With gssencmode='require', check credential cache before connect
Hello.
At Sun, 07 Apr 2024 23:50:08 +, Heikki Linnakangas
wrote in
> With gssencmode='require', check credential cache before connecting
This commit adds the following error message (indentations are adjusted):
+ libpq_append_conn_error(conn,
+ "GSSAPI encryption required but it is not supported over a
local socket)");
The closing parenthesis at the end of the message seems to be a
leftover from editing.
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 4bd523ec6e..e35bdc4036 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2927,7 +2927,7 @@ keep_going:
/* We will come back to here until there is
if (conn->raddr.addr.ss_family
== AF_UNIX)
{
libpq_append_conn_error(conn,
-
"GSSAPI encryption required but it is not supported
over a local socket)");
+
"GSSAPI encryption required but it is not supported
over a local socket");
goto error_return;
}
if (conn->gcred ==
GSS_C_NO_CREDENTIAL)
About the following message:
+ libpq_append_conn_error(conn, "could not set ssl alpn
extension: %s", err);
I'm not sure about the policy for writing acronyms in lowercase, but
other occurrences of ALPN (in backend code) seem to be written in
uppercase.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Enhance libpq encryption negotiation tests with new GUC
At Sun, 07 Apr 2024 23:50:08 +, Heikki Linnakangas
wrote in
> Enhance libpq encryption negotiation tests with new GUC
This commit adds the following messages:
> gettext_noop("Log details of pre-authentication connection handshake."),
Similar to a nearby commit, other messages with a similar context use
the phrase "Logs ". Wouldn't it be better to align this
message with existing ones?
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 83e3a59d7e..4584829992 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1227,7 +1227,7 @@ struct config_bool ConfigureNamesBool[] =
},
{
{"trace_connection_negotiation", PGC_POSTMASTER,
DEVELOPER_OPTIONS,
- gettext_noop("Log details of pre-authentication
connection handshake."),
+ gettext_noop("Logs details of pre-authentication
connection handshake."),
NULL,
GUC_NOT_IN_SAMPLE
},
regards
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Transform OR clauses to ANY expression
At Mon, 08 Apr 2024 14:46:57 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Sun, 07 Apr 2024 22:28:06 +, Alexander Korotkov
> wrote in
> > Transform OR clauses to ANY expression
>
> This commit introduces a message like this:
>
> > gettext_noop("Set the minimum length of the list of OR clauses to attempt
> > the OR-to-ANY transformation."),
>
> Unlike the usual phrasing of similar messages in this context, which
> use the form "Sets the minimum length of...", this message uses an
> imperative form ("Set"). I believe it would be better to align the
> tone of this message with the others by changing "Set" to "Sets".
>
> regards.
>
>
> diff --git a/src/backend/utils/misc/guc_tables.c
> b/src/backend/utils/misc/guc_tables.c
> index 83e3a59d7e..a527ffe0b0 100644
> --- a/src/backend/utils/misc/guc_tables.c
> +++ b/src/backend/utils/misc/guc_tables.c
> @@ -3670,7 +3670,7 @@ struct config_int ConfigureNamesInt[] =
>
> {
> {"or_to_any_transform_limit", PGC_USERSET, QUERY_TUNING_OTHER,
> - gettext_noop("Set the minimum length of the list of OR
> clauses to attempt the OR-to-ANY transformation."),
> + gettext_noop("Sets the minimum length of the list of OR
> clauses to attempt the OR-to-ANY transformation."),
> gettext_noop("Once the limit is reached, the planner
> will try to replace expression like "
>"'x=c1 OR x=c2 ..' to the
> expression 'x = ANY(ARRAY[c1,c2,..])'"),
> GUC_EXPLAIN
Sorry for the sequential posts, but I found the following contruct in
the same patch to be quite untranslatable.
> errmsg("%s bound of partition \"%s\" is %s %s bound of split partition",
> first ? "lower" : "upper",
> relname,
> defaultPart ? (first ? "less than" : "greater than") : "not
> equals to",
> first ? "lower" : "upper"),
> parser_errposition(pstate, datum->location)));
I might be misunderstanding this, but if the expressions are correct,
the message could be divided into four fixed messages based on "first"
and "defaultPart". Alternatively, it might be better to provide
simpler explation of the situation.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Transform OR clauses to ANY expression
At Sun, 07 Apr 2024 22:28:06 +, Alexander Korotkov
wrote in
> Transform OR clauses to ANY expression
This commit introduces a message like this:
> gettext_noop("Set the minimum length of the list of OR clauses to attempt the
> OR-to-ANY transformation."),
Unlike the usual phrasing of similar messages in this context, which
use the form "Sets the minimum length of...", this message uses an
imperative form ("Set"). I believe it would be better to align the
tone of this message with the others by changing "Set" to "Sets".
regards.
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 83e3a59d7e..a527ffe0b0 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3670,7 +3670,7 @@ struct config_int ConfigureNamesInt[] =
{
{"or_to_any_transform_limit", PGC_USERSET, QUERY_TUNING_OTHER,
- gettext_noop("Set the minimum length of the list of OR
clauses to attempt the OR-to-ANY transformation."),
+ gettext_noop("Sets the minimum length of the list of OR
clauses to attempt the OR-to-ANY transformation."),
gettext_noop("Once the limit is reached, the planner
will try to replace expression like "
"'x=c1 OR x=c2 ..' to the
expression 'x = ANY(ARRAY[c1,c2,..])'"),
GUC_EXPLAIN
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: pg_createsubscriber: creates a new logical replica from a standb
Hello.
This commit added the following error message:
pg_createsubscriber.c: 375
> pg_fatal("could not access directory \"%s\": %s",
> datadir,
>strerror(errno));
Although other messages use %s with PQresultErrorMessage(), regarding
this specific message, shouldn't we use %m instead of %s + strerror()?
I'm not sure if that would be better.
pg_createsubscriber.c: 687
> pg_log_error("could not obtain database OID: got %d rows,
> expected %d rows",
>PQntuples(res), 1);
pg_createsubscriber.c: 1652
> pg_log_error("could not obtain subscription OID: got %d rows,
> expected %d rows",
In these messages, the second %d is always written as "1 rows",
whereas a similar message correctly uses "1 row".
pg_createsubscriber.c: 561
> pg_log_error("could not get system identifier: got %d rows,
> expected %d row",
>PQntuples(res), 1);
I think it would be better to change the former instances to "%d row",
or to change both to "1 row". I'd like to go the second way but maybe
we should take the first way following our convention.
pg_createsubscriber.c: 923
> pg_log_error("publisher requires wal_level >= logical");
We discussed this message in relation to commit 801792e528, and
decided to quote "logical" to clarify that it is a string literal. I'd
like to follow the conclusion here, too.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index b8f8269340..a6e0986670 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -372,8 +372,7 @@ check_data_directory(const char *datadir)
if (errno == ENOENT)
pg_fatal("data directory \"%s\" does not exist", datadir);
else
- pg_fatal("could not access directory \"%s\": %s", datadir,
- strerror(errno));
+ pg_fatal("could not access directory \"%s\": %m", datadir);
}
snprintf(versionfile, MAXPGPATH, "%s/PG_VERSION", datadir);
@@ -684,7 +683,7 @@ generate_object_name(PGconn *conn)
if (PQntuples(res) != 1)
{
- pg_log_error("could not obtain database OID: got %d rows, expected %d rows",
+ pg_log_error("could not obtain database OID: got %d rows, expected %d row",
PQntuples(res), 1);
disconnect_database(conn, true);
}
@@ -920,7 +919,7 @@ check_publisher(const struct LogicalRepInfo *dbinfo)
if (strcmp(wal_level, "logical") != 0)
{
- pg_log_error("publisher requires wal_level >= logical");
+ pg_log_error("publisher requires wal_level >= \"logical\"");
failed = true;
}
@@ -1649,7 +1648,7 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
if (PQntuples(res) != 1 && !dry_run)
{
- pg_log_error("could not obtain subscription OID: got %d rows, expected %d rows",
+ pg_log_error("could not obtain subscription OID: got %d rows, expected %d row",
PQntuples(res), 1);
disconnect_database(conn, true);
}
Re: pgsql: Add TAP tests for timeouts
Hello.
At Thu, 14 Mar 2024 11:25:42 +, Alexander Korotkov
wrote in
> Add TAP tests for timeouts
>
> This commit adds new tests to verify that transaction_timeout,
> idle_session_timeout, and idle_in_transaction_session_timeout work as
> expected.
> We introduce new injection points in before throwing a timeout FATAL error
> and check these injection points are reached.
>
> Discussion:
> https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
> Author: Andrey Borodin
> Reviewed-by: Alexander Korotkov
In 005_timeouts.pl, I found the following comment.
> # If we send \q with $psql_session->quit it can get to pump already closed.
> # So \q is in initial script, here we only finish IPC::Run.
> $psql_session->{run}->finish;
I'm not sure if "it can get to pump already closed." makes sense. I
guess that it means "the command can get to be pumped (or "can be
sent") to the session already closed" or something similar?
> # 2. Test of the sidle in transaction timeout
s/sidle/idle/ ?
> # Wait until the backend is in the timeout injection point.
I'm not sure, but it seems that "is in" meant "passes" or something like?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Allow logical decoding on standbys
At Thu, 13 Apr 2023 12:07:19 -0400, Robert Haas wrote
in
> On Wed, Apr 12, 2023 at 1:43=E2=80=AFPM Andres Freund =
> wrote:
> > > And, a nearby commit addds the following message.
> > >
> > > + appendStringInfo(&err_detail, _("Logical decoding=
> on standby requires wal_level to be at least logical on the primary server=
> "));
> > >
> > > This is omitting the indefinite article before "standby". I'm not sure
> > > what to do about it but feel like we don't need it here.
> >
> > I don't think we need it either, but I'm not a native speaker.
>
> I am a native speaker of English and I think it is not required here.
> If we were going to add an article, I think we would want an
> indefinite one ("a standby") not a definite one ("the standby")
> because we're talking about a general truism, not a situation that
> pertains to one specific server. However, I don't think we should add
> that because the current phrasing is more consistent with our general
> practice. It's not the way you would actually talk, but it the way
> that people often write error messages. For example, we say "file not
> found" not "the file was not found," and nobody gets upset or confused
> by that.
Thaks for the clarification!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Allow logical decoding on standbys
At Wed, 12 Apr 2023 11:10:00 -0700, Andres Freund wrote in
> Hi,
>
> On 2023-04-12 10:42:44 -0700, Andres Freund wrote:
> > On 2023-04-11 12:02:53 +0900, Kyotaro Horiguchi wrote:
> > > > Allow logical decoding on standbys
> > >
> > > This adds the following error message.
> > >
> > > + errmsg("logical decoding on a standby requires wal_level to be at least
> > > logical on the primary")));
> > >
> > > We alredy have a nearly identical message as follows.
> > >
> > > > errmsg("logical decoding requires wal_level >= logical")));
> > >
> > > And we used to writte this kind of conditions, like "wal_level >=
> > > logical", using a mathematical operator. Don't we need to unify them?
> >
> > I guess. It doesn't seem particularly important, but why not.
>
> If we do that, we should adjust the errdetail() that you (correctly!) pointed
> out as needing punctuation as well. Pushed those improvements together.
Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
At Tue, 11 Apr 2023 16:53:42 +1200, David Rowley wrote
in
> On Tue, 11 Apr 2023 at 15:02, Kyotaro Horiguchi
> wrote:
> > > postgres=# vacuum (buffer_usage_limit);
> > > ERROR: buffer_usage_limit requires a valid value
> >
> > The error message doesn't really make much sense to me. In the same
> > context, most of the code seems to use ("%s requires a parameter",
> > buffer_usage_limit) without "valid".
>
> The only other option I see that requires a value in this context is
> "parallel", and what you say is not true for that, so I'm not sure I
> follow what you're referring to with "most of the code". Can you quote
> the code you mean?
My point was just that "valid" seems redundant. (Sorry, but..)
However, looking again, many of the "cases" are like "requires a
value", which looks womewhat similar to "requires a valid
value" when the cannot be represented by a word. Some remaining
example of these "caes" are:
./commands/define.cÀ54: errmsg("%s requires a
parameter",
(the "parameter" means the option value for type name)
./libpq/hba.cÀ1319: errmsg("authentication method
\"%s\" requires argument \"%s\" to be set", \
But, since I'm not a native speaker, so I don't stress this further
more.
> > vacuum.c:347
> > >errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM
> > > FULL")));
> >
> > It seems like that the vacuum options are usually spelled in uppercase
> > at least for vacuum and analyze. In any case, shouldn't we need to
> > unify them in a certain area? (For example, command options for
> > CREATE SUBSCRIPTION is shown in lowercase in the documentation. But,
> > I'm not exactly sure what we should do about it.)
>
> I wouldn't object to making things more consistent in this area with
> regards to the casing of the options in the ERROR messages. However,
> it doesn't really seem like there is much consistency to follow that
> this new code is breaking.
...
> I do agree that it's not very good that I pushed the lowercase version
> of buffer_usage_limit and the same in uppercase for the VACUUM FULL
> conflict. I'll hold back from fixing that until we figure out the
> other stuff.
Thanks for the opinion! I agree to you.
Thanks!
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Support invalidating replication slots due to horizon and wal_le
> Support invalidating replication slots due to horizon and wal_level
This adds the following message.
+ appendStringInfo(&err_detail, _("Logical decoding on
standby requires wal_level to be at least logical on the primary server"));
This message is missing a period at the end.
regards.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 41848f0ac6..e73fd2d13b 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1274,7 +1274,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause
cause,
break;
case RS_INVAL_WAL_LEVEL:
- appendStringInfo(&err_detail, _("Logical decoding on
standby requires wal_level to be at least logical on the primary server"));
+ appendStringInfo(&err_detail, _("Logical decoding on
standby requires wal_level to be at least logical on the primary server."));
break;
case RS_INVAL_NONE:
pg_unreachable();
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Allow logical decoding on standbys
> Allow logical decoding on standbys
This adds the following error message.
+ errmsg("logical decoding on a standby requires wal_level to be at least
logical on the primary")));
We alredy have a nearly identical message as follows.
> errmsg("logical decoding requires wal_level >= logical")));
And we used to writte this kind of conditions, like "wal_level >=
logical", using a mathematical operator. Don't we need to unify them?
And, a nearby commit addds the following message.
+ appendStringInfo(&err_detail, _("Logical decoding on
standby requires wal_level to be at least logical on the primary server"));
This is omitting the indefinite article before "standby". I'm not sure
what to do about it but feel like we don't need it here.
diff --git a/src/backend/replication/logical/decode.c
b/src/backend/replication/logical/decode.c
index 5508cc2177..beef399b42 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -177,7 +177,7 @@ xlog_decode(LogicalDecodingContext *ctx, XLogRecordBuffer
*buf)
Assert(RecoveryInProgress());
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-errmsg("logical
decoding on a standby requires wal_level to be at least logical on the
primary")));
+errmsg("logical
decoding on standby requires wal_level >= logical on the primary")));
}
break;
}
diff --git a/src/backend/replication/logical/logical.c
b/src/backend/replication/logical/logical.c
index 82dae95080..7e1f677f7a 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -137,7 +137,7 @@ CheckLogicalDecodingRequirements(void)
if (GetActiveWalLevelOnStandby() < WAL_LEVEL_LOGICAL)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-errmsg("logical decoding on a standby
requires wal_level to be at least logical on the primary")));
+errmsg("logical decoding on standby
requires wal_level >= logical on the primary")));
}
}
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
I apologize for writing mails in such a sporadic manner..
At Tue, 11 Apr 2023 10:23:35 +0900 (JST), Kyotaro Horiguchi
wrote in
> Hello.
>
> > Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
>
> This commit added the following error message.
>
> > errmsg("value: \"%s\": is invalid for buffer_usage_limit",
>
> It looks as the follows on terminal.
>
> postgres=# vacuum (buffer_usage_limit 'x');
> ERROR: value: "x": is invalid for buffer_usage_limit
>
> I'm not sure why the message has two colons. [1] talks about the
> message but doesn't really explain the reason for this.
>
> [1]
> https://www.postgresql.org/message-id/CAApHDvqs%2BFcw9Yrn4ORCAX0xKK1-SiCC0w1j7Dhg%3DG9hrvag7g%40mail.gmail.com
vacuum.c:198
> if (opt->arg == NULL)
> {
>ereport(ERROR,
>(errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("buffer_usage_limit option requires a valid value"),
> parser_errposition(pstate, opt->location)));
> postgres=# vacuum (buffer_usage_limit);
> ERROR: buffer_usage_limit requires a valid value
The error message doesn't really make much sense to me. In the same
context, most of the code seems to use ("%s requires a parameter",
buffer_usage_limit) without "valid".
vacuum.c:347
>errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM
> FULL")));
It seems like that the vacuum options are usually spelled in uppercase
at least for vacuum and analyze. In any case, shouldn't we need to
unify them in a certain area? (For example, command options for
CREATE SUBSCRIPTION is shown in lowercase in the documentation. But,
I'm not exactly sure what we should do about it.)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
Hello.
> Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
This commit added the following error message.
>errmsg("value: \"%s\": is invalid for buffer_usage_limit",
It looks as the follows on terminal.
postgres=# vacuum (buffer_usage_limit 'x');
ERROR: value: "x": is invalid for buffer_usage_limit
I'm not sure why the message has two colons. [1] talks about the
message but doesn't really explain the reason for this.
[1]
https://www.postgresql.org/message-id/CAApHDvqs%2BFcw9Yrn4ORCAX0xKK1-SiCC0w1j7Dhg%3DG9hrvag7g%40mail.gmail.com
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: Only allow returning string types or bytea from json_serialize
At Mon, 11 Jul 2022 12:49:11 +0900, Michael Paquier wrote in > On Fri, Jul 08, 2022 at 07:20:14PM +0900, Michael Paquier wrote: > > Yeah, I think that you are right, so let's fix this. If Andrew does > > not show up, I'll take care of it. > > Okay, this one is done, then. Thanks for the patch, Horiguchi-san. Thanks! (I came to notice that I sent the mail only to -commiters..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Only allow returning string types or bytea from json_serialize
At Thu, 07 Jul 2022 21:46:12 +, Andrew Dunstan wrote
in
> Only allow returning string types or bytea from json_serialize
I noticed that this introcues the following error message.
+ errmsg("cannot use RETURNING type %s in JSON_SERIALIZE"
+
format_type_be(returning->typid)),
However, the same file has the following error message.
>errmsg("cannot use RETURNING type %s in %s",
> format_type_be(returning->typid),
> fname),
So, couldn't we share the format string to reduce translatable
messages?
And, the other messages are
cannot use RETURNING type %s in JSON_SCALAR(), and
cannot use RETURNING type %s in JSON()
So, I think this should not be
cannot use RETURNING type %s in JSON_SERIALIZE
, but should be
cannot use RETURNING type %s in JSON_SERIALIZE()
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 3d62f30c9e112e52190b638569de8fc69e287ddb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Fri, 8 Jul 2022 15:21:05 +0900
Subject: [PATCH] Fix an error message
Fix an error message in accordance to the simlar messages. The
message can share the same format string with an existing message, so
use it to reduce the number of translatable error messages.
---
src/backend/parser/parse_expr.c | 5 +++--
src/test/regress/expected/sqljson.out | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index efcf1cd5ab..1dbdba93da 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -4587,8 +4587,9 @@ transformJsonSerializeExpr(ParseState *pstate, JsonSerializeExpr *expr)
if (typcategory != TYPCATEGORY_STRING)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("cannot use RETURNING type %s in JSON_SERIALIZE",
-format_type_be(returning->typid)),
+ errmsg("cannot use RETURNING type %s in %s",
+format_type_be(returning->typid),
+"JSON_SERIALIZE()"),
errhint("Try returning a string type or bytea")));
}
}
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index be27bce9d3..aae4ba4939 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -316,7 +316,7 @@ SELECT pg_typeof(JSON_SERIALIZE(NULL));
-- only string types or bytea allowed
SELECT JSON_SERIALIZE('{ "a" : 1 } ' RETURNING jsonb);
-ERROR: cannot use RETURNING type jsonb in JSON_SERIALIZE
+ERROR: cannot use RETURNING type jsonb in JSON_SERIALIZE()
HINT: Try returning a string type or bytea
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_SERIALIZE('{}');
QUERY PLAN
--
2.31.1
Re: pgsql: Fix pg_basebackup with in-place tablespaces.
At Wed, 16 Mar 2022 11:13:53 +1300, Thomas Munro wrote
in
> On Wed, Mar 16, 2022 at 10:28 AM Tom Lane wrote:
> > David Steele writes:
> > > On 3/14/22 19:31, Thomas Munro wrote:
> > >> Fix pg_basebackup with in-place tablespaces.
> >
> > > Perhaps I'm being picky, but seems like this logic should be wrapped in:
> > > if (allow_in_place_tablespaces)
> > > {
> > > <...>
> > > }
> > > I worry about strange effects when this GUC is not enabled.
> >
> > What would happen if someone created an in-place tablespace and
> > then turned off the GUC?
>
> Then it would break with unhelpful error messages. I suppose we could
> error out explicitly, "in-place tablespace detected, but
> allow_in_place_tablespaces not enabled". I'm not sure why we should
> suddenly choose to enforce that *here* though.
+1. The GUC is only to prevent non-developer users from accidentally
creating in-place tablespaces that is not officieally suported. We
have done nothing about in-place tablespaces other than the things
needed to perform regression test, and I think we won't do that in
future.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: plperl: windows: Use Perl_setlocale on 5.28+, fixing compile fai
At Sun, 30 Jan 2022 17:35:48 -0800, Andres Freund wrote in > Hi, > > On 2022-01-31 00:44:39 +, Andres Freund wrote: > > plperl: windows: Use Perl_setlocale on 5.28+, fixing compile failure. > > > > For older versions we need our own copy of perl's setlocale(), because it > > was > > not exposed (why we need the setlocale in the first place is explained in > > plperl_init_interp) . The copy stopped working in 5.28, as some of the used > > macros are not public anymore. But Perl_setlocale is available in 5.28, so > > use that. > > > > Author: Victor Wagner > > Reviewed-By: Dagfinn Ilmari Mannsåker > > Discussion: https://postgr.es/m/20200501134711.08750...@antares.wagner.home > > Backpatch: all versions > > > > Branch > > -- > > REL_14_STABLE > > I see this broke on everywhere but master. Looking. Anyways, I can build on windows with this (even only on master). Thanks for the work! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: postgres_fdw: Allow postgres_fdw.application_name to include esc
At Fri, 24 Dec 2021 18:44:06 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Fri, 24 Dec 2021 18:24:39 +0900, Fujii Masao
> wrote in
> >
> >
> > On 2021/12/24 18:00, Kyotaro Horiguchi wrote:
> > > I saw the test has been revertd.
> >
> > Yes, I reverted the added unstable tests not to prevent
> > buildfarm from testing other patches while I'm doing
> > the investigation.
> >
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25
> > >
> > >> NOTICE: identifier
> > >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
> > >> will be truncated to
> > >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"
> > > It's 70 characters long..
> > > application_name: pg_regress/postgres_fdw
> > > user_name : buildfarm
> > > database_name : contrib_regression_postgres_fdw
> > > Source PID : 757365
> > > Maybe we can distribute the placeholders into several sessions.
> >
> > Or probably we don't need to test all escape sequences. How about
> > picking up one or two from them? But even if only one or two are
> > picked up, application_name still can be larger than 63 characters. So
> > probably we also should use substring() in the test query, for that
> > case. For example,
> >
> > SELECT count(*) > 0 FROM pg_stat_activity
> > WHERE application_name = substring(current_setting('application_name')
> > ||
> > CURRENT_USER || current_database() || pg_backend_pid() || '%', 1, 63);
>
> I once thought the same, but that change causes buildfarms *always*
> truncate the resulting application name. I believe the base
> application_name and database_name never grows further longer. So I'd
> like to split the check to %a%u%% and %d%p.
Or if we are convinced that the names won't get longer, we could set
application_name on the local session as something like 'X' so that the
result fits 63 character-long..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: postgres_fdw: Allow postgres_fdw.application_name to include esc
At Fri, 24 Dec 2021 18:24:39 +0900, Fujii Masao
wrote in
>
>
> On 2021/12/24 18:00, Kyotaro Horiguchi wrote:
> > I saw the test has been revertd.
>
> Yes, I reverted the added unstable tests not to prevent
> buildfarm from testing other patches while I'm doing
> the investigation.
>
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25
> >
> >> NOTICE: identifier
> >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
> >> will be truncated to
> >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"
> > It's 70 characters long..
> > application_name: pg_regress/postgres_fdw
> > user_name : buildfarm
> > database_name : contrib_regression_postgres_fdw
> > Source PID : 757365
> > Maybe we can distribute the placeholders into several sessions.
>
> Or probably we don't need to test all escape sequences. How about
> picking up one or two from them? But even if only one or two are
> picked up, application_name still can be larger than 63 characters. So
> probably we also should use substring() in the test query, for that
> case. For example,
>
> SELECT count(*) > 0 FROM pg_stat_activity
> WHERE application_name = substring(current_setting('application_name')
> ||
> CURRENT_USER || current_database() || pg_backend_pid() || '%', 1, 63);
I once thought the same, but that change causes buildfarms *always*
truncate the resulting application name. I believe the base
application_name and database_name never grows further longer. So I'd
like to split the check to %a%u%% and %d%p.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: postgres_fdw: Allow postgres_fdw.application_name to include esc
I saw the test has been revertd. At Fri, 24 Dec 2021 17:20:18 +0900, Fujii Masao wrote in > Hmm... some buildfarm animals reported a failure. So I will > investigate the issue. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25 > NOTICE: identifier > "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%" > will be truncated to > "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw" It's 70 characters long.. application_name: pg_regress/postgres_fdw user_name : buildfarm database_name : contrib_regression_postgres_fdw Source PID : 757365 Maybe we can distribute the placeholders into several sessions. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Allow pg_receivewal to stream from a slot's restart LSN
At Tue, 26 Oct 2021 00:47:44 +, Michael Paquier wrote in > Allow pg_receivewal to stream from a slot's restart LSN ... > Details > --- > https://git.postgresql.org/pg/commitdiff/f61e1dd2cee6b1a1da75c2bb0ca3bc72f18748c1 Ouch.. sorry, it's a bit late. I noticed a typo in the change. + If a starting point cannot not be calculated with the previous method, The "not" is a duplicate. reagrds. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml index d3c7488293..9fde2fd2ef 100644 --- a/doc/src/sgml/ref/pg_receivewal.sgml +++ b/doc/src/sgml/ref/pg_receivewal.sgml @@ -90,7 +90,7 @@ PostgreSQL documentation - If a starting point cannot not be calculated with the previous method, + If a starting point cannot be calculated with the previous method, and if a replication slot is used, an extra READ_REPLICATION_SLOT command is issued to retrieve the slot's restart_lsn to use as starting point.
Re: pgsql: ecpg: Remove trailing period from error message.
At Wed, 25 Aug 2021 00:57:25 +, Fujii Masao wrote in > ecpg: Remove trailing period from error message. > > This commit improves the ecpg's error message that commit f576de1db1 updated, > so that it gets rid of trailing period and uppercases the command name > in the error message. > > Author: Kyotaro Horiguchi > Reviewed-by: Fujii Masao > Discussion: > https://postgr.es/m/20210819.170315.1413060634876301811.horikyota@gmail.com > > Branch > -- > master REL_14_STABLE has the same commit as e2d6da0708, (which I found at the first time). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Fix handling of WAL segments ready to be archived during crash r
At Fri, 24 Apr 2020 10:14:37 +0900, Michael Paquier wrote in > On Fri, Apr 24, 2020 at 09:59:29AM +0900, Michael Paquier wrote: > > And this visibly comes down to the fact that we don't take care enough > > of the timing between the restartpoints done, the startup process > > doing its recycling work and the archiver. The rest of the test > > relies on the reports of pg_stat_archiver a points to wait at as > > published by the archiver process. So there are two things we could > > do here: > > 1) Just remove the unstable parts of the tests (the three ones above), > > and keep coverage based on everything we have using pg_stat_archiver. > > 2) Remove the test entirely, though I would rather have us keep some > > coverage, particularly for primaries as this got broken. > > > > I'd rather do 2), any thoughts? > > Oops, sorry. I sent this message too quickly. I would rather > actually do 1) and keep the major parts of the tests. All the > buildfarm failures are just around the three checks mentioned > upthread. Thanks for your trouble fixing the failures. I think we can reimplement them by waiting pg_stat_archiver.last_failed_wal at least for archive_mode=always case. I'm not sure about the case where archive_mode=on, though. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Skip WAL for new relfilenodes, under wal_level=minimal.
At Fri, 24 Apr 2020 16:29:41 +0900, Michael Paquier wrote in > Hi Noah, > > On Sat, Apr 04, 2020 at 08:40:43PM +, Noah Misch wrote: > > Skip WAL for new relfilenodes, under wal_level=minimal. > > > > Until now, only selected bulk operations (e.g. COPY) did this. If a > > given relfilenode received both a WAL-skipping COPY and a WAL-logged > > operation (e.g. INSERT), recovery could lose tuples from the COPY. See > > src/backend/access/transam/README section "Skipping WAL for New > > RelFileNode" for the new coding rules. Maintainers of table access > > methods should examine that section. > > > > To maintain data durability, just before commit, we choose between an > > fsync of the relfilenode and copying its contents to WAL. A new GUC, > > wal_skip_threshold, guides that choice. If this change slows a workload > > that creates small, permanent relfilenodes under wal_level=minimal, try > > adjusting wal_skip_threshold. Users setting a timeout on COMMIT may > > need to adjust that timeout, and log_min_duration_statement analysis > > will reflect time consumption moving to COMMIT from commands like COPY. > > + /* > +* Records other than SWITCH_WAL must have content. We use an integer 0 to > +* follow the restriction. > +*/ > This commit has added the following comment, but I guess you meant > XLOG_SWITCH instead? Oops, I'm the one to blame on it. Actually it is a mistake of XLOG_SWITCH. By the way, the "xlog-switch" in the following comment might be better to be XLOG_SWITCH. > /* All (non xlog-switch) records should contain data. */ There are several other occurances of xlog-switch. ragards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Remove testing for precise LSN/reserved bytes in new TAP test
At Thu, 09 Apr 2020 17:31:59 -0400, Tom Lane wrote in > Kyotaro Horiguchi writes: > >> Sorry for the wrong test file. > >> Checking in lower precision would be sufficient. > > > I noticed that prailiedog failed in another mode. > > Yeah. We have at least four different buildfarm members complaining > about this test case. I took this patch and further lobotomized the > tests by removing *all* dependencies on restart_lsn and > pg_current_wal_lsn(). If anybody wants to put any of that back, > the burden of proof will be on them to show why we should believe > the results will be stable, not for the buildfarm to demonstrate > that they're not. I think the significant part of the test is wal_status. So I'm not eager to get it back. Thanks! -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgsql: Remove testing for precise LSN/reserved bytes in new TAP test
At Wed, 08 Apr 2020 14:44:52 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Wed, 08 Apr 2020 03:33:04 +, Alvaro Herrera
> wrote in
> > Remove testing for precise LSN/reserved bytes in new TAP test
> >
> > Trying to ensure that a slot's restart_lsn or amount of reserved bytes
> > exactly match some specific values seems unnecessary, and fragile as
> > shown by failures in multiple buildfarm members.
>
> Sorry for the wrong test file.
> Checking in lower precision would be sufficient.
I noticed that prailiedog failed in another mode.
not ok 5 - check that max_slot_wal_keep_size is working
# Failed test 'check that max_slot_wal_keep_size is working'
# at t/019_replslot_limit.pl line 89.
# got: '0/D00198|normal|5120 kB'
# expected: '0/D00164|normal|5120 kB'
The first colomn in the result is restart_lsn and it's not essential
to the test. It is removed in the attached test script. Precise LSN is
not reffered in tht test script.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
# Test for replication slot limit
# Ensure that max_slot_wal_keep_size limits the number of WAL files to
# be kept by replication slots.
use strict;
use warnings;
use TestLib;
use PostgresNode;
use File::Path qw(rmtree);
use Test::More tests => 13;
use Time::HiRes qw(usleep);
$ENV{PGDATABASE} = 'postgres';
# Initialize master node, setting wal-segsize to 1MB
my $node_master = get_new_node('master');
$node_master->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
$node_master->append_conf('postgresql.conf', qq(
min_wal_size = 2MB
max_wal_size = 4MB
log_checkpoints = yes
));
$node_master->start;
$node_master->safe_psql('postgres', "SELECT
pg_create_physical_replication_slot('rep1')");
# The slot state and remain should be null before the first connection
my $result = $node_master->safe_psql('postgres', "SELECT restart_lsn is NULL,
wal_status is NULL, min_safe_lsn is NULL FROM pg_replication_slots WHERE
slot_name = 'rep1'");
is($result, "t|t|t", 'check the state of non-reserved slot is "unknown"');
# Take backup
my $backup_name = 'my_backup';
$node_master->backup($backup_name);
# Create a standby linking to it using the replication slot
my $node_standby = get_new_node('standby_1');
$node_standby->init_from_backup($node_master, $backup_name, has_streaming => 1);
$node_standby->append_conf('postgresql.conf', "primary_slot_name = 'rep1'");
$node_standby->start;
# Wait until standby has replayed enough data
my $start_lsn = $node_master->lsn('write');
$node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
# Stop standby
$node_standby->stop;
# Preparation done, the slot is the state "normal" now
$result = $node_master->safe_psql('postgres', "SELECT wal_status, min_safe_lsn
is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "normal|t", 'check the catching-up state');
# Advance WAL by five segments (= 5MB) on master
advance_wal($node_master, 1);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when fitting max_wal_size
$result = $node_master->safe_psql('postgres', "SELECT wal_status, min_safe_lsn
is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "normal|t", 'check that it is safe if WAL fits in max_wal_size');
advance_wal($node_master, 4);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when max_slot_wal_keep_size is not set
$result = $node_master->safe_psql('postgres', "SELECT wal_status, min_safe_lsn
is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "normal|t", 'check that slot is working');
# The standby can reconnect to master
$node_standby->start;
$start_lsn = $node_master->lsn('write');
$node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
$node_standby->stop;
# Set max_slot_wal_keep_size on master
my $max_slot_wal_keep_size_mb = 6;
$node_master->append_conf('postgresql.conf', qq(
max_slot_wal_keep_size = ${max_slot_wal_keep_size_mb}MB
));
$node_master->reload;
# The slot is in safe state. The distance from the min_safe_lsn should
# be as almost (max_slot_wal_keep_size - 1) times large as the segment
# size
$result = $node_master->safe_psql('postgres', "SELECT wal_status,
floor((pg_current_wal_lsn() - min_safe_lsn) / 1024 / 1024) FROM
pg_replication_slots WHERE slot_name = &
Re: pgsql: Remove testing for precise LSN/reserved bytes in new TAP test
At Wed, 08 Apr 2020 03:33:04 +, Alvaro Herrera
wrote in
> Remove testing for precise LSN/reserved bytes in new TAP test
>
> Trying to ensure that a slot's restart_lsn or amount of reserved bytes
> exactly match some specific values seems unnecessary, and fragile as
> shown by failures in multiple buildfarm members.
Sorry for the wrong test file.
Checking in lower precision would be sufficient.
--
Kyotaro Horiguchi
NTT Open Source Software Center
# Test for replication slot limit
# Ensure that max_slot_wal_keep_size limits the number of WAL files to
# be kept by replication slots.
use strict;
use warnings;
use TestLib;
use PostgresNode;
use File::Path qw(rmtree);
use Test::More tests => 13;
use Time::HiRes qw(usleep);
$ENV{PGDATABASE} = 'postgres';
# Initialize master node, setting wal-segsize to 1MB
my $node_master = get_new_node('master');
$node_master->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
$node_master->append_conf('postgresql.conf', qq(
min_wal_size = 2MB
max_wal_size = 4MB
log_checkpoints = yes
));
$node_master->start;
$node_master->safe_psql('postgres', "SELECT
pg_create_physical_replication_slot('rep1')");
# The slot state and remain should be null before the first connection
my $result = $node_master->safe_psql('postgres', "SELECT restart_lsn is NULL,
wal_status is NULL, min_safe_lsn is NULL FROM pg_replication_slots WHERE
slot_name = 'rep1'");
is($result, "t|t|t", 'check the state of non-reserved slot is "unknown"');
# Take backup
my $backup_name = 'my_backup';
$node_master->backup($backup_name);
# Create a standby linking to it using the replication slot
my $node_standby = get_new_node('standby_1');
$node_standby->init_from_backup($node_master, $backup_name, has_streaming => 1);
$node_standby->append_conf('postgresql.conf', "primary_slot_name = 'rep1'");
$node_standby->start;
# Wait until standby has replayed enough data
my $start_lsn = $node_master->lsn('write');
$node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
# Stop standby
$node_standby->stop;
# Preparation done, the slot is the state "normal" now
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status,
min_safe_lsn is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "$start_lsn|normal|t", 'check the catching-up state');
# Advance WAL by five segments (= 5MB) on master
advance_wal($node_master, 1);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when fitting max_wal_size
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status,
min_safe_lsn is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "$start_lsn|normal|t", 'check that restart_lsn is in max_wal_size');
advance_wal($node_master, 4);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when max_slot_wal_keep_size is not set
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status,
min_safe_lsn is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "$start_lsn|normal|t", 'check that slot is working');
# The standby can reconnect to master
$node_standby->start;
$start_lsn = $node_master->lsn('write');
$node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
$node_standby->stop;
# Set max_slot_wal_keep_size on master
my $max_slot_wal_keep_size_mb = 6;
$node_master->append_conf('postgresql.conf', qq(
max_slot_wal_keep_size = ${max_slot_wal_keep_size_mb}MB
));
$node_master->reload;
# The slot is in safe state. The distance from the min_safe_lsn should
# be as almost (max_slot_wal_keep_size - 1) times large as the segment
# size
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status,
floor((pg_current_wal_lsn() - min_safe_lsn) / 1024 / 1024) FROM
pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "$start_lsn|normal|5", 'check that max_slot_wal_keep_size is
working');
# Advance WAL again then checkpoint, reducing remain by 2 MB.
advance_wal($node_master, 2);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# The slot is still working
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status,
floor((pg_current_wal_lsn() - min_safe_lsn) / 1024 / 1024) FROM
pg_replication_slots WHERE slot_name = 'rep1'&qu
Re: pgsql: walreceiver uses a temporary replication slot by default
At Thu, 13 Feb 2020 16:48:21 +0900, Michael Paquier wrote in > On Wed, Feb 12, 2020 at 06:11:06PM +0900, Fujii Masao wrote: > > On 2020/02/12 7:53, Jehan-Guillaume de Rorthais wrote: > >> In my humble opinion, I prefer the previous behavior, streaming without > >> temporary slot, for one reason: primary availability. > > > > +1 > > > >> With temp slot created by default, if one standby lag far behind, it can > >> make > >> the primary unavailable. We have nothing yet to forbid a slot to fill the > >> pg_wal partition. How new users creating their first cluster would react > >> in such > >> situation? I suppose the original discussion was mostly targeting them? > >> Recovering from this is way more scary than building a standby. > >> > >> So the default behavior might not be desirable and maybe > >> wal_receiver_create_temp_slot might be off by default? > >> > >> Note that Kyotaro HORIGUCHI is working on a patch to restricting maximum > >> keep > >> segments by repslots: > >> > >> https://www.postgresql.org/message-id/flat/20190627162256.4f4872b8%40firost#6cba1177f766e7ffa5237789e748da38 > > > > Yeah, I think it's better to disable this option until something like > > Horiguchi-san's proposal will have been committed, i.e., until > > the upper limit on the number (or size) of WAL files that remain > > for slots become configurable. > > Even with that, are we sure this extra feature would be a reason > sufficient to change the default value of this option to be enabled? I think the feature (slot limit) is not going to be an reason to enable it (tmp slot). In the first place I think we cannot determine the default value generally workable.. > I am not sure about that either. My opinion is that this option is > useful to have and that it is not really a problem if you have slot > monitoring on the primary (or a standby for cascading). And I'd like > to believe that it is a common practice lately for base backups, > archivers based on pg_receivewal or even logical decoding, but it > could be surprising for some users who do not do that yet. So > Jehan-Guillaume's arguments sound also sensible to me (he also > maintains an automatic failover solution called PAF). > > From what I can see nobody really likes the current state of things > for this option, and that does not come down only to its default > value. The default GUC value and the way the parameter is loaded by > the WAL sender are problematic, still easy enough to fix. How do we > move on from here? I could post a patch based on what Sergei Kornilov > has sent around [1], but that's Peter's feature. Any opinions? > > [1]: https://www.postgresql.org/message-id/20200122055510.gh174...@paquier.xyz regards. -- Kyotaro Horiguchi NTT Open Source Software Center
