Re: [Proposal] Add foreign-server health checks infrastructure
On 2021-11-29 21:36, Zhihong Yu wrote:
On Mon, Nov 29, 2021 at 12:51 AM kuroda.hay...@fujitsu.com
wrote:
Dear Zhihong,
Thank you for giving comments! I'll post new patches later.
+#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()
(CheckingRemoteServersHoldoffCount++)
The macro contains only one operation. Can the macro be removed
(with `CheckingRemoteServersHoldoffCount++` inlined) ?
Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and
HOLD_CANCEL_INTERRUPTS():
```
#define HOLD_INTERRUPTS() (InterruptHoldoffCount++)
#define RESUME_INTERRUPTS() \
do { \
Assert(InterruptHoldoffCount > 0); \
InterruptHoldoffCount--; \
} while(0)
#define HOLD_CANCEL_INTERRUPTS() (QueryCancelHoldoffCount++)
#define RESUME_CANCEL_INTERRUPTS() \
do { \
Assert(QueryCancelHoldoffCount > 0); \
QueryCancelHoldoffCount--; \
} while(0)
#define START_CRIT_SECTION() (CritSectionCount++)
#define END_CRIT_SECTION() \
do { \
Assert(CritSectionCount > 0); \
CritSectionCount--; \
} while(0)
```
So I want to keep the current style. Could you tell me if you have
any other reasons?
+ if (CheckingRemoteServersTimeoutPending &&
CheckingRemoteServersHoldoffCount != 0)
+ {
+ /*
+* Skip checking foreign servers while reading messages.
+*/
+ InterruptPending = true;
+ }
+ else if (CheckingRemoteServersTimeoutPending)
Would the code be more readable if the above two if blocks be
moved inside one enclosing if block (factoring the common
condition)?
+ if (CheckingRemoteServersTimeoutPending)
+1. Will fix.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Hi,
It is Okay to keep the macros.
Thanks
Hi, Kuroda-san. Sorry for late reply.
Even for local-only transaction, I thought it useless to execute
CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I
make it so that it determines outside whether it contains SQL to the
remote or not?
The following points are minor.
1. In foreign.c, some of the lines are too long and should be broken.
2. In pqcomm.c, the newline have been removed, what is the intention of
this?
3. In postgres.c,
3-1. how about inserting a comment between lines 2713 and 2714, similar
to line 2707?
3-2. the line breaks in line 3242 and line 3243 should be aligned.
3-3. you should change
/*
* Skip checking foreign servers while reading messages.
*/
to
/*
* Skip checking foreign servers while reading messages.
*/
4. In connection.c, There is a typo in line 1684, so "fucntion" should
be changed to "function".
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Emit a warning if the extension's GUC is set incorrectly
Thank you for the review and sorry for the late reply.
On 2021-11-16 19:25, Bharath Rupireddy wrote:
> I observed an odd behaviour:
> 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
> 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
> contrib module, I created the extension, have seen the following
> warning:
> 2021-11-15 06:02:31.198 UTC [2018111] WARNING: unrecognized
> configuration parameter "postgres_fdw.XXX"
> 3) I further did, "alter system set
> postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
> pg_reload_conf();", it silently accepts.
>
> postgres=# create extension postgres_fdw ;
> WARNING: unrecognized configuration parameter "postgres_fdw.XXX"
> CREATE EXTENSION
> postgres=# alter system set
> postgres_fdw.XXX='I_further_messed_up_conf_file';
> ALTER SYSTEM
> postgres=# select pg_reload_conf();
> pg_reload_conf
>
> t
> (1 row)
I have made changes to achieve the above.
However, it also gives me an error when
---
postgres=# SET abc.a TO on;
SET
postgres=# SET abc.b TO on;
2021-12-16 16:22:20.351 JST [2489236] ERROR: unrecognized configuration
parameter "abc.a"
2021-12-16 16:22:20.351 JST [2489236] STATEMENT: SET abc.b TO on;
ERROR: unrecognized configuration parameter "abc.a"
---
I know that some people do not think this is good.
Therefore, can I leave this problem alone or is there another better
way?
For backward compatibility you can retain the function
EmitWarningsOnPlaceholders as-is and have another similar function
that emits an error:
In guc.c, have something like below:
static void
check_conf_params(const char *className, bool emit_error)
{
/* have the existing code of EmitWarningsOnPlaceholders here */
ereport(emit_error ? ERROR : WARNING,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("unrecognized configuration parameter
\"%s\"",
var->name)));
}
void
EmitErrorOnPlaceholders(const char *className)
{
check_conf_params(className, true);
}
void
EmitWarningsOnPlaceholders(const char *className)
{
check_conf_params(className, false);
}
Thank you for your advise.
According to [1], we used the same function name, but the warning level
was INFO.
Therefore, I think it is OK to use the same function name.
[1]
https://www.postgresql.org/message-id/flat/200901051634.n05GYNr06169%40momjian.us#1d045374f014494e4b40a4862a000723
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e91d5a3cfd..d136ca12ea 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5431,11 +5431,19 @@ valid_custom_variable_name(const char *name)
{
bool saw_sep = false;
bool name_start = true;
+ int placeholder_length = 0;
for (const char *p = name; *p; p++)
{
+ placeholder_length++;
if (*p == GUC_QUALIFIER_SEPARATOR)
{
+ /* Check invalid placeholder and custom parameter*/
+ char *placeholder = malloc(sizeof(char) * (placeholder_length - 1));
+ strncpy(placeholder, name, placeholder_length - 1);
+ placeholder[placeholder_length -1] = '\0';
+ EmitErrorsOnPlaceholders(placeholder);
+
if (
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-12-17 01:55, Fujii Masao wrote:
On 2021/12/16 16:31, Shinya Kato wrote:
Thank you for the review and sorry for the late reply.
On 2021-11-16 19:25, Bharath Rupireddy wrote:
> I observed an odd behaviour:
> 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
> 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
> contrib module, I created the extension, have seen the following
> warning:
> 2021-11-15 06:02:31.198 UTC [2018111] WARNING: unrecognized
> configuration parameter "postgres_fdw.XXX"
> 3) I further did, "alter system set
> postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
> pg_reload_conf();", it silently accepts.
>
> postgres=# create extension postgres_fdw ;
> WARNING: unrecognized configuration parameter "postgres_fdw.XXX"
> CREATE EXTENSION
> postgres=# alter system set
> postgres_fdw.XXX='I_further_messed_up_conf_file';
> ALTER SYSTEM
> postgres=# select pg_reload_conf();
> pg_reload_conf
>
> t
> (1 row)
I have made changes to achieve the above.
IMO this behavior change is not good. For example, because it seems to
break at least the following case. I think that these are the valid
steps, but with the patch, the server fails to start up at the step #2
because pg_trgm's custom parameters are treated as invalid ones.
1. Add the following two pg_trgm parameters to postgresql.conf
- pg_trgm.similarity_threshold
- pg_trgm.strict_word_similarity_threshold
2. Start the server
3. Run "CREATE EXTENSION pg_trgm" and then use pg_trgm
It can happen because the placeholder "pg_trgm" is not already
registered.
I think too this behavior is not good.
I need to consider an another implementation or to allow undefined GUCs
to be set.
When I compiled PostgreSQL with the patch, I got the following
compilation failure.
guc.c:5453:4: error: implicit declaration of function
'EmitErrorsOnPlaceholders' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
EmitErrorsOnPlaceholders(placeholder);
^
- ereport(WARNING,
+ ereport(ERROR,
Thanks! There was a correction omission, so I fixed it.
I'm still not sure why you were thinking that ERROR is more proper
here.
Since I get an ERROR when I set the wrong normal GUCs, I thought I
should also get an ERROR when I set the wrong extension's GUCs.
However, there are likely to be harmful effects, and I may need to think
again.
For now, I'v attached the patch that fixed the compilation error.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e91d5a3cfd..c7c0179c5c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5431,11 +5431,19 @@ valid_custom_variable_name(const char *name)
{
bool saw_sep = false;
bool name_start = true;
+ int placeholder_length = 0;
for (const char *p = name; *p; p++)
{
+ placeholder_length++;
if (*p == GUC_QUALIFIER_SEPARATOR)
{
+ /* Check invalid placeholder and custom parameter*/
+ char *placeholder = malloc(sizeof(char) * (placeholder_length - 1));
+ strncpy(placeholder, name, placeholder_length - 1);
+ plac
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-12-17 15:42, Peter Eisentraut wrote:
On 17.12.21 03:25, Shinya Kato wrote:
For now, I'v attached the patch that fixed the compilation error.
I think it would be good if you could split the uncontroversial new
EmitErrorsOnPlaceholders() calls into a separate patch. And please
add an explanation what exactly the rest of the patch changes.
Thank you for the comment!
I splitted the patch.
- v6-01-Add-EmitWarningsOnPlaceholders.patch
We should use EmitWarningsOnPlaceholders when we use
DefineCustomXXXVariable.
I don't think there is any room for debate.
- v6-0002-Change-error-level-of-EmitWarningsOnPlaceholders.patch
This is a patch to change the error level of EmitWarningsOnPlaceholders
from WARNING to ERROR.
I think it's OK to emit ERROR as well as when the wrong GUC is set for
non-extensions, or since it does not behave abnormally, it can be left
as WARNING.
Thought?
- v6-0003-Emit-error-when-invalid-extensions-GUCs-are-set.patch
This is a patch to emit error when invalid extension's GUCs are set.
No test changes have been made, so the regression test will fail.
I have created a patch, but I don't think this is necessary because of
the previous discussion.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index e11837559d..1389aa0943 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -474,6 +474,8 @@ _PG_init(void)
PGC_SUSET, 0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("pltclu");
+
pltcl_pm_init_done = true;
}
diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c
index b3d0841ba8..8ec623ac52 100644
--- a/src/test/modules/delay_execution/delay_execution.c
+++ b/src/test/modules/delay_execution/delay_execution.c
@@ -91,6 +91,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("delay_execution");
+
/* Install our hook */
prev_planner_hook = planner_hook;
planner_hook = delay_execution_planner;
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
index 6b0a3db104..3ba33e501c 100644
--- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -48,6 +48,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("ssl_passphrase");
+
if (ssl_passphrase)
openssl_tls_init_hook = set_rot13;
}
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 0b6246676b..adb02d8cb8 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -322,6 +322,8 @@ _PG_init(void)
0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("worker_spi");
+
/* set up common data for all our workers */
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 7b03046301..5e9407c34a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9333,6 +9333,13 @@ DefineCustomEnumVariable(const char *name,
define_custom_variable(&var->gen);
}
+/*
+ * For historical context, this function name is EmitWarningsOnPlac
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-12-22 02:23, Tom Lane wrote: Kyotaro Horiguchi writes: At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato wrote in We should use EmitWarningsOnPlaceholders when we use DefineCustomXXXVariable. I don't think there is any room for debate. Unfortunately, pltcl.c defines variables both in pltcl and pltclu but the patch adds the warning only for pltclu. Right. The rest of 0001 looks fine, so pushed with that correction. I concur that 0002 is unacceptable. The result of it will be that a freshly-loaded extension will fail to hook into the system as it should, because it will fail to complete its initialization. That will cause user frustration and bug reports. (BTW, I wonder if EmitWarningsOnPlaceholders should be using LOG rather than WARNING when it's running in the postmaster. Use of WARNING is moderately likely to result in nothing getting printed at all.) 0003 looks to me like it was superseded by 75d22069e. I do not particularly like that patch though; it seems both inefficient and ill-designed. 0003 is adding a check in an equally bizarre place. Why isn't add_placeholder_variable the place to deal with this? Also, once we know that a prefix is reserved, why don't we throw an error not just a warning for attempts to set some unknown variable under that prefix? We don't allow you to set unknown non-prefixed variables. regards, tom lane Thank you for pushing! Thank you all for the reviews, I think I will take down 0002 and 0003. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021/12/22 3:33, Tom Lane wrote: I wrote: 0003 looks to me like it was superseded by 75d22069e. I do not particularly like that patch though; it seems both inefficient and ill-designed. 0003 is adding a check in an equally bizarre place. Why isn't add_placeholder_variable the place to deal with this? Also, once we know that a prefix is reserved, why don't we throw an error not just a warning for attempts to set some unknown variable under that prefix? We don't allow you to set unknown non-prefixed variables. Concretely, I think we should do the attached, which removes much of 75d22069e and does the check at the point of placeholder creation. (After looking closer, add_placeholder_variable's caller is the function that is responsible for rejecting bad names, not add_placeholder_variable itself.) This also fixes an indisputable bug in 75d22069e, namely that it did prefix comparisons incorrectly and would throw warnings for cases it shouldn't. Reserving "plpgsql" shouldn't have the effect of reserving "pl" as well. Thank you for creating the patch. This is exactly what I wanted to create. It looks good to me. I'm tempted to propose that we also rename EmitWarningsOnPlaceholders to something like MarkGUCPrefixReserved, to more clearly reflect what it does now. (We could provide the old name as a macro alias to avoid breaking extensions needlessly.) +1 -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [Proposal] Add foreign-server health checks infrastructure
Thank you for the new patch! On 2021-12-15 15:40, kuroda.hay...@fujitsu.com wrote: Dear Kato-san, Thank you for giving comments! And sorry for late reply. I rebased my patches. Even for local-only transaction, I thought it useless to execute CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I make it so that it determines outside whether it contains SQL to the remote or not? Yeah, remote-checking timeout will be enable even ifa local transaction is opened. In my understanding, postgres cannot distinguish whether opening transactions are using only local object or not. My first idea was that turning on the timeout when GetFdwRoutineXXX functions were called, but in some cases FDWs may not use remote connection even if they call such a handler function. e.g. postgresExplainForeignScan(). Hence I tried another approach that unregister all checking callback the end of each transactions. Only FDWs can notice that transactions are remote or local, so they must register callback when they really connect to other database. This implementation will avoid calling remote checking in the case of local transaction, but multiple registering and unregistering may lead another overhead. I attached which implements that. It certainly incurs another overhead, but I think it's better than the previous one. So far, I haven't encountered any problems, but I'd like other people's opinions. The following points are minor. 1. In foreign.c, some of the lines are too long and should be broken. 2. In pqcomm.c, the newline have been removed, what is the intention of this? 3. In postgres.c, 3-1. how about inserting a comment between lines 2713 and 2714, similar to line 2707? 3-2. the line breaks in line 3242 and line 3243 should be aligned. 3-3. you should change /* * Skip checking foreign servers while reading messages. */ to /* * Skip checking foreign servers while reading messages. */ 4. In connection.c, There is a typo in line 1684, so "fucntion" should be changed to "function". Maybe all of them were fixed. Thanks! Thank you, and it looks good to me. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion
On 2021-10-14 14:30, katouknl wrote:
It is very good, but it seems to me that there are some tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON
I think these tab-comletion also can be improved and it's a good
timing for that.
Thank you for the comments!
I fixed where you pointed out.
Thank you for the update!
I tried "COMMENT ON OPERATOR ...", and an operator seemed to be
complemented with double quotation marks.
However, it caused the COMMENT command to fail.
---
postgres=# COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
ERROR: syntax error at or near "("
LINE 1: COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
postgres=# COMMENT ON OPERATOR + (integer, integer) IS 'test_success';
COMMENT
---
So, I think as with \do command, you do not need to complete the
operators.
Do you think?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
On 2021-10-21 03:40, Mark Dilger wrote: These patches have been split off the now deprecated monolithic "Delegating superuser tasks to new security roles" thread at [1]. The purpose of these patches is to fix the CREATEROLE escalation attack vector misfeature. (Not everyone will see CREATEROLE that way, but the perceived value of the patch set likely depends on how much you see CREATEROLE in that light.) Hi! Thank you for the patch. I too think that CREATEROLE escalation attack is problem. I have three comments. 1. Is there a function to check the owner of a role, it would be nice to be able to check with \du or pg_roles view. 2. Is it correct that REPLICATION/BYPASSRLS can be granted even if you are not a super user, but have CREATEROLE and REPLICATION/BYPASSRLS? 3. I think it would be better to have an "DROP ROLE [ IF EXISTS ] name [, ...] [CASCADE | RESTRICT]" like "DROP TABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]". What do you think? -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion
On 2021-10-15 17:49, Ken Kato wrote:
2021-10-15 13:29 に Shinya Kato さんは書きました:
On 2021-10-14 14:30, katouknl wrote:
It is very good, but it seems to me that there are some
tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON
I think these tab-comletion also can be improved and it's a good
timing for that.
Thank you for the comments!
I fixed where you pointed out.
Thank you for the update!
I tried "COMMENT ON OPERATOR ...", and an operator seemed to be
complemented with double quotation marks.
However, it caused the COMMENT command to fail.
---
postgres=# COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
ERROR: syntax error at or near "("
LINE 1: COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
postgres=# COMMENT ON OPERATOR + (integer, integer) IS 'test_success';
COMMENT
---
So, I think as with \do command, you do not need to complete the
operators.
Do you think?
Thank you for the further comments!
I fixed so that it doesn't complete the operators anymore.
It only completes with CLASS and FAMILY.
Also, I updated TEXT SEARCH.
It completes object names for each one of CONFIGURATION, DICTIONARY,
PARSER, and TEMPLATE.
Thank you for update!
The patch looks good to me. I applied cosmetic changes to it.
Attached is the updated version of the patch.
Barring any objection, I will change status to Ready for Committer.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index ecae9df8ed..4c67b6842b 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -494,7 +494,6 @@ static const SchemaQuery Query_for_list_of_partitioned_indexes = {
.result = "pg_catalog.quote_ident(c.relname)",
};
-
/* All relations */
static const SchemaQuery Query_for_list_of_relations = {
.catname = "pg_catalog.pg_class c",
@@ -513,11 +512,46 @@ static const SchemaQuery Query_for_list_of_partitioned_relations = {
.result = "pg_catalog.quote_ident(c.relname)",
};
+static const SchemaQuery Query_for_list_of_operator_classes = {
+ .catname = "pg_catalog.pg_opclass o",
+ .viscondition = "pg_catalog.pg_opclass_is_visible(o.oid)",
+ .namespace = "o.opcnamespace",
+ .result = "pg_catalog.quote_ident(o.opcname)",
+};
+
static const SchemaQuery Query_for_list_of_operator_families = {
- .catname = "pg_catalog.pg_opfamily c",
- .viscondition = "pg_catalog.pg_opfamily_is_visible(c.oid)",
- .namespace = "c.opfnamespace",
- .result = "pg_catalog.quote_ident(c.opfname)",
+ .catname = "pg_catalog.pg_opfamily o",
+ .viscondition = "pg_catalog.pg_opfamily_is_visible(o.oid)",
+ .namespace = "o.opfnamespace",
+ .result = "pg_catalog.quote_ident(o.opfname)",
+};
+
+static const SchemaQuery Query_for_list_of_text_search_configurations = {
+ .catname = "pg_catalog.pg_ts_config t",
+ .viscondition = "pg_catalog.pg_ts_config_is_visible(t.oid)",
+ .namespace = "t.cfgnamespace",
+ .result = "pg_catalog.quote_ident(t.cfgname)",
+};
+
+static const SchemaQuery Query_for_list_of_text_search_dictionaries = {
+ .catname = "pg_catalog.pg_ts_dict t",
+ .viscondition = "pg_catalog.pg_ts_dict_is_visible(t.oid)",
+ .namespace = "t.dictnamespace",
+ .result = "pg_catalog.quote_ident(t.dictname)",
+};
+
+static const SchemaQuery Query_for_list_of_text_search_parsers = {
+ .catname = "pg_catalog.pg_ts_parser t",
+ .viscondition = "pg_catalog.pg_ts_parser_is_visible(t.oid)",
+ .namespace = "t.prsnamespace",
+ .result = "pg_catalog.quote_ident(t.prsname)",
+};
+
+static const SchemaQuery Query_for_list_of_text_search_templates = {
+ .catname = "pg_catalog.pg_ts_template t",
+ .viscondition = "pg_catalog.pg_ts_template_is_visible(t.oid)",
+ .namespace = "t.tmplnamespace",
+ .result = "pg_catalog.quote_ident(t.tmplname)",
};
/* Relations supporting INSERT, UPDATE or DELETE */
@@ -628,7 +662,6 @@ static const SchemaQuery Query_for_list_of_collations = {
.result = "pg_catalog.quote_ident(c.collname)",
};
-
/*
* Queries to get lists of names of various kinds of things, possibly
* restricted to names matching a partially entered name. In these queries,
@@ -767,6 +800,22 @@ static const SchemaQuery Query_for_list_of_collations = {
" UNION ALL SELECT 'CURRENT_USER'"\
" UNION ALL SELECT 'SESSION_USER'"
+#define Query_for_list_of_operator_class_index_methods \
+"SELECT pg_catalog.qu
Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion
On 2021-10-27 14:45, Michael Paquier wrote:
On Tue, Oct 26, 2021 at 05:04:24PM +0900, Shinya Kato wrote:
Barring any objection, I will change status to Ready for Committer.
+ else if (Matches("COMMENT", "ON", "PROCEDURAL"))
+ COMPLETE_WITH("LANGUAGE");
+ else if (Matches("COMMENT", "ON", "PROCEDURAL", "LANGUAGE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_languages);
I don't think that there is much point in being this picky either with
the usage of PROCEDURAL, as we already complete a similar and simpler
grammar with LANGUAGE. I would just remove this part of the patch.
In my opinion, it is written in the documentation, so tab-completion of
"PROCEDURAL"is good.
How about a completion with "LANGUAGE" and "PROCEDURAL LANGUAGE", like
"PASSWORD" and "ENCRYPTED PASSWORD" in CREATE ROLE?
+ else if (Matches("COMMENT", "ON", "OPERATOR"))
+ COMPLETE_WITH("CLASS", "FAMILY");
Isn't this one wrong? Operators can have comments, and we'd miss
them. This is mentioned upthread, but it seems to me that we'd better
drop this part of the patch if the operator naming part cannot be
solved easily.
As you said, it may be misleading.
I agree to drop it.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
On 2021-10-28 07:21, Mark Dilger wrote:
On Oct 25, 2021, at 10:09 PM, Shinya Kato
wrote:
Hi! Thank you for the patch.
I too think that CREATEROLE escalation attack is problem.
I have three comments.
1. Is there a function to check the owner of a role, it would be nice
to be able to check with \du or pg_roles view.
No, but that is a good idea.
These two ideas are implemented in v2. Both \du and pg_roles show the
owner information.
Thank you. It seems good to me.
By the way, I got the following execution result.
I was able to add the membership of a role with a different owner.
In brief, "a" was able to change the membership of owner "shinya".
Is this the correct behavior?
---
postgres=# CREATE ROLE a LOGIN;
CREATE ROLE
postgres=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
postgres=# CREATE ROLE b;
CREATE ROLE
postgres=# \du a
List of roles
Role name | Owner | Attributes | Member of
---+++-
a | shinya || {pg_execute_server_program}
postgres=# \du b
List of roles
Role name | Owner | Attributes | Member of
---++--+---
b | shinya | Cannot login | {}
postgres=# \c - a
You are now connected to database "postgres" as user "a".
postgres=> GRANT pg_execute_server_program TO b;
GRANT ROLE
postgres=> \du b
List of roles
Role name | Owner | Attributes | Member of
---++--+-
b | shinya | Cannot login | {pg_execute_server_program}
---
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
On 2021-10-29 01:14, Tom Lane wrote: Mark Dilger writes: The only intentional backward compatibility break in this patch set is the the behavior of CREATEROLE. The general hope is that such a compatibility break will help far more than it hurts, as CREATEROLE does not appear to be a well adopted feature. I would expect that breaking the behavior of the WITH ADMIN OPTION feature would cause a lot more pain. Even more to the point, WITH ADMIN OPTION is defined by the SQL standard. The only way you get to mess with that is if you can convince people we mis-implemented the standard. Thank you for the detailed explanation. I now understand what you said. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
On 2021-10-28 07:21, Mark Dilger wrote:
On Oct 25, 2021, at 10:09 PM, Shinya Kato
wrote:
Hi! Thank you for the patch.
I too think that CREATEROLE escalation attack is problem.
I have three comments.
1. Is there a function to check the owner of a role, it would be nice
to be able to check with \du or pg_roles view.
No, but that is a good idea.
These two ideas are implemented in v2. Both \du and pg_roles show the
owner information.
The current solution is to run REASSIGN OWNED in each database where
the role owns objects before running DROP ROLE. At that point, the
CASCADE option (not implemented) won't be needed. Of course, I need
to post the next revision of this patch set addressing the
deficiencies that Nathan pointed out upthread to make that work.
REASSIGN OWNED and ALTER ROLE..OWNER TO now work in v2.
When ALTER ROLE with the privilege of REPLICATION, only the superuser is
checked.
Therefore, we have a strange situation where we can create a role but
not change it.
---
postgres=> SELECT current_user;
current_user
--
test
(1 row)
postgres=> \du test
List of roles
Role name | Owner |Attributes| Member of
---++--+---
test | shinya | Create role, Replication | {}
postgres=> CREATE ROLE test2 REPLICATION;
CREATE ROLE
postgres=> ALTER ROLE test2 NOREPLICATION;
2021-11-04 14:24:02.687 JST [2615016] ERROR: must be superuser to alter
replication roles or change replication attribute
2021-11-04 14:24:02.687 JST [2615016] STATEMENT: ALTER ROLE test2
NOREPLICATION;
ERROR: must be superuser to alter replication roles or change
replication attribute
---
Wouldn't it be better to check if the role has CREATEROLE and
REPLICATION?
The same is true for BYPASSRLS.
By the way, is this thread registered to CommitFest?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Emit a warning if the extension's GUC is set incorrectly
Hi hackers,
If wrong GUCs of auth_delay, pg_trgm, postgres_fdw and sepgsql are
described in postgresql.conf, a warning is not emitted unlike
pg_stat_statements, auto_explain and pg_prewarm.
So, I improved it by adding EmitWarningsOnPlaceholders.
An example output is shown below.
---
2021-11-14 18:18:16.486 JST [487067] WARNING: unrecognized
configuration parameter "auth_delay.xxx"
---
What do you think?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-11-15 04:50, Daniel Gustafsson wrote:
Seems reasonable on a quick skim, commit da2c1b8a2 did a similar
roundup back
in 2009 but at the time most of these didn't exist (pg_trgm did but
didn't have
custom option back then). There is one additional callsite defining
custom
variables in src/pl/tcl/pltcl.c which probably should get this
treatment as
well, it would align it with the pl/perl counterpart.
I'll have a closer look and test tomorrow.
Thank you for the review!
I have missed src/pl/tcl/pltcl.c, so I created the new patch.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index e11837559d..1389aa0943 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -474,6 +474,8 @@ _PG_init(void)
PGC_SUSET, 0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("pltclu");
+
pltcl_pm_init_done = true;
}
Re: [Proposal] Add foreign-server health checks infrastructure
Hi, Thank you for the patch! On 2021-10-30 12:50, kuroda.hay...@fujitsu.com wrote: ## Background Currently there is no way to check the status of an foreign server in PostgreSQL. If an foreign server's postmaster goes down, or if the network between servers is lost, the backend process will only detect these when it uses the connection corresponding to that foreign server. Consider a workload that updates data on an foreign server only at the beginning of a transaction, and runs a lot of local SQLs. Even if the network is disconnected immediately after accessing the foreign server, the backend process will continue to execute local SQLs without realizing it. The process will eventually finish to execute SQLs and try to commit. Only then will it realize that the foreign server cannot be connect and will abort the transaction. This situation should be detected as soon as possible because it is impossible to commit a transaction when the foreign server goes down. This can be more of a problem if you have system-wide downtime requirements. That's why I want to implement the health-check feature to postgres. It's a good idea. I also think such a situation should be avoided. ## Further work As the next step I have a plan to implement the callback function to postgres_fdw. I already made a PoC, but it deeply depends on the following thread: https://commitfest.postgresql.org/35/3098/ I also want you to review the postgres_fdw part, but I think it should not be attached because cfbot cannot understand such a dependency and will throw build error. Do you know how to deal with them in this case? I don't know how to deal with them, but I hope you will attach the PoC, as it may be easier to review. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-11-15 15:14, Bharath Rupireddy wrote:
Do we need to add them in the following too?
delay_execution/delay_execution.c
ssl_passphrase_func.c
worker_spi.c
Especially, worker_spi.c is important as it works as a template for
the bg worker code.
Thank you for pointing this out! I added it.
I'm not sure if we have "how to create an extension/a bg worker?" in
the core docs, if we have, it is a good idea to make note of using
EmitWarningsOnPlaceholders so that it will not be missed in the future
modules.
I cannot find any such docs, so I add a comment to
src/backend/utils/misc/guc.c.
I observed an odd behaviour:
1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
contrib module, I created the extension, have seen the following
warning:
2021-11-15 06:02:31.198 UTC [2018111] WARNING: unrecognized
configuration parameter "postgres_fdw.XXX"
3) I further did, "alter system set
postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
pg_reload_conf();", it silently accepts.
postgres=# create extension postgres_fdw ;
WARNING: unrecognized configuration parameter "postgres_fdw.XXX"
CREATE EXTENSION
postgres=# alter system set
postgres_fdw.XXX='I_further_messed_up_conf_file';
ALTER SYSTEM
postgres=# select pg_reload_conf();
pg_reload_conf
t
(1 row)
My point is, why should the "create extension" succeed with a WARNING
when a wrong parameter related to it is set in the postgresql.conf
file so that we don't see further problems as shown in (3). I think
EmitWarningsOnPlaceholders should emit an error so that the extension
will not get created in the first place if a wrong configuration
parameter is set in the postgresql.conf file. Many times, users will
not have access to server logs so it is good to fail the "create
extension" statement.
Thoughts?
postgres=# create extension postgres_fdw ;
ERROR: unrecognized configuration parameter "postgres_fdw.XXX"
postgres=#
I confirmed it too, and I think that's definitely a problem.
I also thought it would be a good idea to emit an ERROR as well as when
an invalid normal GUC is set.
I didn't change the function name because it would affect many third
party extensions.
I plan to change to emit an error when an invalid custom GUC is set in
the SET or ALTER SYSTEM SET commands, but I haven't tackled this yet.
The patch as of now is attached.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e91d5a3cfd..07647a0d84 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9322,6 +9322,13 @@ DefineCustomEnumVariable(const char *name,
define_custom_variable(&var->gen);
}
+/*
+ * For historical context, this function name is EmitWarningsOnPlaceholders,
+ * but it emits an error when an invalid custom GUC is set.
+ *
+ * If we use DefineCustomXXXVariable, it is recommended that this function is
+ * added.
+ */
void
EmitWarningsOnPlaceholders(const char *className)
{
@@ -9336,7 +9343,7 @@ EmitWarningsOnPlaceholders(const char *className)
strncmp(className, var->name, classLen) == 0 &&
var->name[classLen] == GUC_QUALIFIER_SEPARATOR)
{
- ereport(WARNING,
+ ere
Re: [Proposal] Add foreign-server health checks infrastructure
On 2021-11-18 21:43, kuroda.hay...@fujitsu.com wrote:
Dear Kato-san,
Thank you for your interest!
> I also want you to review the postgres_fdw part,
> but I think it should not be attached because cfbot cannot understand
> such a dependency
> and will throw build error. Do you know how to deal with them in this
> case?
I don't know how to deal with them, but I hope you will attach the
PoC,
as it may be easier to review.
OK, I attached the PoC along with the dependent patches. Please see
the zip file.
add_helth_check_... patch is written by me, and other two patches are
just copied from [1].
In the new callback function ConnectionHash is searched sequentially
and
WaitEventSetWait() is performed for WL_SOCKET_CLOSED socket event.
This event is added by the dependent ones.
Thank you for sending the patches!
I confirmed that they can be compiled and tested successfully on CentOS
8.
I haven't looked at the core of the code yet, but I took a quick look at
it.
+ {
+ {"remote_servers_connection_check_interval", PGC_USERSET,
CONN_AUTH_SETTINGS,
+ gettext_noop("Sets the time interval between checks for
disconnection of remote servers."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &remote_servers_connection_check_interval,
+ 0, 0, INT_MAX,
+ },
If you don't use check_hook, assign_hook and show_hook, you should
explicitly write "NULL, NULL, NULL", as show below.
+ {
+ {"remote_servers_connection_check_interval", PGC_USERSET,
CONN_AUTH_SETTINGS,
+ gettext_noop("Sets the time interval between checks for
disconnection of remote servers."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &remote_servers_connection_check_interval,
+ 0, 0, INT_MAX,
+ NULL, NULL, NULL
+ },
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("Postgres foreign server %s might be
down.",
+ server->servername));
According to [1], error messages should start with a lowercase letter
and not use a period.
Also, along with the rest of the code, it is a good idea to enclose the
server name in double quotes.
I'll get back to you once I've read all the code.
[1] https://www.postgresql.org/docs/devel/error-style-guide.html
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Fix bugs not to discard statistics when changing stats_fetch_consistency
Hi, hackers
There is below description in docs for stats_fetch_consistency.
"Changing this parameter in a transaction discards the statistics
snapshot."
However, I wonder if changes stats_fetch_consistency in a transaction,
statistics is not discarded in some cases.
Example:
--
* session 1
=# SET stats_fetch_consistency TO snapshot;
=# BEGIN;
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-+-+---
23592 | 628 | 5939027
(1 row)
* session 2
=# CREATE TABLE test (i int); -- generate WAL records
=# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-+-+---
23631 | 644 | 6023411
(1 row)
* session 1
=*# -- snapshot is not discarded, it is right
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-+-+---
23592 | 628 | 5939027
(1 row)
=*# SET stats_fetch_consistency TO cache;
=*# -- snapshot is not discarded, it is not right
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-+-+---
23592 | 628 | 5939027
(1 row)
--
I can see similar cases in pg_stat_archiver, pg_stat_bgwriter,
pg_stat_checkpointer, pg_stat_io, and pg_stat_slru.
Is it a bug? I fixed it, and do you think?
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATIONdiff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 125ca97b18..40ef6ef321 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1714,3 +1714,11 @@ assign_stats_fetch_consistency(int newval, void *extra)
if (pgstat_fetch_consistency != newval)
force_stats_snapshot_clear = true;
}
+
+
+void
+pgstat_clear_snapshot_if_needed(void)
+{
+ if (force_stats_snapshot_clear)
+ pgstat_clear_snapshot();
+}
diff --git a/src/backend/utils/activity/pgstat_archiver.c b/src/backend/utils/activity/pgstat_archiver.c
index 66398b20e5..d4979e9fd0 100644
--- a/src/backend/utils/activity/pgstat_archiver.c
+++ b/src/backend/utils/activity/pgstat_archiver.c
@@ -57,6 +57,7 @@ pgstat_report_archiver(const char *xlog, bool failed)
PgStat_ArchiverStats *
pgstat_fetch_stat_archiver(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_ARCHIVER);
return &pgStatLocal.snapshot.archiver;
diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 7d2432e4fa..931d0f17e7 100644
--- a/src/backend/utils/activity/pgstat_bgwriter.c
+++ b/src/backend/utils/activity/pgstat_bgwriter.c
@@ -70,6 +70,7 @@ pgstat_report_bgwriter(void)
PgStat_BgWriterStats *
pgstat_fetch_stat_bgwriter(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_BGWRITER);
return &pgStatLocal.snapshot.bgwriter;
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index 30a8110e38..ac700072ed 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -79,6 +79,7 @@ pgstat_report_checkpointer(void)
PgStat_CheckpointerStats *
pgstat_fetch_stat_checkpointer(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_CHECKPOINTER);
return &pgStatLocal.snapshot.checkpointer;
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 43c393d6fe..13712aa4af 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -156,6 +156,7 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
PgStat_IO *
pgstat_fetch_stat_io(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_IO);
return &pgStatLocal.snapshot.io;
diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c
index 56ea1c3378..7f8a97e8ca 100644
--- a/src/backend/utils/activity/pgstat_slru.c
+++ b/src/backend/utils/activity/pgstat_slru.c
@@ -104,6 +104,7 @@ pgstat_count_slru_truncate(int slru_idx)
PgStat_SLRUStats *
pgstat_fetch_slru(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_SLRU);
return pgStatLocal.snapshot.slru;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index 1a3c0e1a66..f8fcf909f0 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -66,6 +66,7 @@ pgstat_report_wal(bool force)
PgStat_WalStats *
pgstat_fetch_stat_wal(void)
{
+ pgstat_clear_snapshot_if_needed();
pgstat_snapshot_fixed(PGSTAT_KIND_WAL);
return &pgStatLocal.snapshot.wal;
Re: Fix bugs not to discard statistics when changing stats_fetch_consistency
On 2024-02-01 17:33, Michael Paquier wrote: On Thu, Jan 11, 2024 at 06:18:38PM +0900, Shinya Kato wrote: Hi, hackers (Sorry for the delay, this thread was on my TODO list for some time.) There is below description in docs for stats_fetch_consistency. "Changing this parameter in a transaction discards the statistics snapshot." However, I wonder if changes stats_fetch_consistency in a transaction, statistics is not discarded in some cases. Yep, you're right. This is inconsistent with the documentation where we need to clean up the cached data when changing this GUC. I was considering a few options regarding the location of the extra pgstat_clear_snapshot(), but at the end I see the point in doing it in a path even if it creates a duplicate with pgstat_build_snapshot() when pgstat_fetch_consistency is using the snapshot mode. A location better than your patch is pgstat_snapshot_fixed(), though, so as new stats kinds will be able to get the call. I have been banging my head on my desk for a bit when thinking about a way to test that in a predictible way, until I remembered that these stats are only flushed at commit, so this requires at least two sessions, with one of them having a transaction opened while manipulating stats_fetch_consistency. TAP would be one option, but I'm not really tempted about spending more cycles with a background_psql just for this case. If I'm missing something, feel free. Thank you for the review and pushing! I think it is better and more concise than my implementation. -- Regards, Shinya Kato NTT DATA GROUP CORPORATION
Remove duplicates of membership from results of \du
Hi, hackers
When executing \du, you can see duplicates of the same role in 'member of'.
This happens when admin | inherit | set options are granted by another role.
---
postgres=# create role role_a login createrole;
CREATE ROLE
postgres=# \du
List of roles
Role name | Attributes | Member of
---++---
role_a | Create role
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=# set role role_a;
SET
postgres=> create role role_b;
CREATE ROLE
postgres=> \du
List of roles
Role name | Attributes | Member of
---++---
role_a | Create role
| {role_b}
role_b | Cannot login
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=> grant role_b to role_a;
GRANT ROLE
postgres=> \du
List of roles
Role name | Attributes | Member of
---++-
role_a | Create role
| {role_b,role_b}
role_b | Cannot login
| {}
shinya | Superuser, Create role, Create DB, Replication, Bypass RLS
| {}
postgres=> select rolname, oid from pg_roles where rolname = 'role_b';
rolname | oid
-+---
role_b | 16401
(1 row)
postgres=> select * from pg_auth_members where roleid = 16401;
oid | roleid | member | grantor | admin_option | inherit_option |
set_option
---+++-+--++
16402 | 16401 | 16400 | 10 | t | f | f
16403 | 16401 | 16400 | 16400 | f | t | t
(2 rows)
---
Attached patch resolves this issue.
Do you think?
Regards,
Shinya Kato
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 058e41e749..8aeb669100 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3632,7 +3632,7 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
"SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
" r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
" r.rolconnlimit, r.rolvaliduntil,\n"
- " ARRAY(SELECT b.rolname\n"
+ " ARRAY(SELECT DISTINCT b.rolname\n"
"FROM pg_catalog.pg_auth_members m\n"
"JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
"WHERE m.member = r.oid) as memberof");
Add --{no-,}bypassrls flags to createuser
Hi,
Add --{no-,}bypassrls flags to createuser.
The following is an example of execution.
--
$ createuser a --bypassrls
$ psql -c "\du a"
List of roles
Role name | Attributes | Member of
---++---
a | Bypass RLS | {}
--
Do you think?
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..6c2ee1e0c6 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -290,6 +290,28 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will have the BYPASSRLS privilege,
+which is described more fully in the documentation for .
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not have the BYPASSRLS
+privilege, which is described more fully in the documentation for .
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..5b363b6f54 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -48,6 +48,8 @@ main(int argc, char *argv[])
{"replication", no_argument, NULL, 1},
{"no-replication", no_argument, NULL, 2},
{"interactive", no_argument, NULL, 3},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
@@ -76,7 +78,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -165,6 +168,12 @@ main(int argc, char *argv[])
case 3:
interactive = true;
break;
+ case 4:
+bypassrls = TRI_YES;
+break;
+ case 5:
+bypassrls = TRI_NO;
+break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -304,6 +313,10 @@ main(int argc, char *argv[])
appendPQExpBufferStr(&sql, " REPLICATION");
if (replication == TRI_NO)
appendPQExpBufferStr(&sql, " NOREPLICATION");
+ if (bypassrls == TRI_YES)
+ appendPQExpBufferStr(&sql, " BYPASSRLS");
+ if (bypassrls == TRI_NO)
+ appendPQExpBufferStr(&sql, " NOBYPASSRLS");
if (conn_limit >= -1)
appendPQExpBuffer(&sql, " CONNECTION LIMIT %d", conn_limit);
if (roles.head != NULL)
@@ -366,6 +379,8 @@ help(const char *progname)
"than using defaults\n"));
printf(_(" --replication role can initiate replication\n"));
printf(_(" --no-replication role cannot initiate replication\n"));
+ printf(_(" --bypassrls role can bypass row-level security (RLS) policy\n"));
+ printf(_(" --no-bypassrlsrole cannot bypass row-level security (RLS) policy\n"));
printf(_(" -?, --helpshow this help, then exit\n"));
printf(_("\nConnection options:\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
Re: Add --{no-,}bypassrls flags to createuser
On 2022-04-13 17:35, Kyotaro Horiguchi wrote:
At Wed, 13 Apr 2022 16:10:01 +0900, Michael Paquier
wrote in
On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:
> It is sensible to rig createuser command with full capability of
> CREATE ROLE is reasonable.
>
> Only --replication is added by commit 9b8aff8c19 (2010) since
> 8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
> 491c029dbc (2014) but it seems to have forgotten to add the
> corresponding createuser options.
>
> By a quick search, found a few other CREATE ROLE optinos that are not
> supported by createuser.
My question is: is BYPASSRLS common enough to justify having a switch
to createuser? As the development cycle of 15 has just finished and
that we are in feature freeze, you may want to hold on new patches for
a bit. The next commit fest is planned for July.
I don't think there's a definitive criteria (other than feasibility)
for whether each CREATE ROLE option should have the correspondent
option in the createuser command. I don't see a clear reason why
createuser command should not have the option.
Thank you for the review!
I created a new patch containing 'VALID UNTIL', 'ADMIN', and 'ROLE'.
To add the ROLE clause, the originally existing --role option
(corresponding to the IN ROLE clause) is changed to the --in-role
option. Would this not be good from a backward compatibility standpoint?
As far as schedules are concerned, I don't think this has anything to
do with 15.
I have registered this patch for the July commit fest.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..fc477605f6 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+ The new role will be added immediately as a member with admin option
+ of this role.
+ Multiple roles to which new role will be added as a member with admin
+ option can be specified by writing multiple
+ -a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -132,7 +146,7 @@ PostgreSQL documentation
-g role
- --role=role
+ --in-role=role
Indicates role to which this role will be added immediately as a new
@@ -143,6 +157,19 @@ PostgreSQL documentation
+
+ -G role
+ --role=role
+
+
+ The new role will be added immediately as a member of this role.
+ Multiple roles to which new role will be added as a member
+ can be specified by writing multiple
+ -G switches.
+
+
+
+
-i
--inherit
@@ -258,6 +285,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a timestamp after which the role's password is no longer valid.
+The default is to set no expiration.
+
+
+
+
-V
--version
@@ -290,6 +328,28 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will have the BYPASSRLS privilege,
+which is described more fully in the documentation for .
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not have the BYPASSRLS
+privilege, which is described more fully in the documentation for .
+
+
+
+
-?
--help
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7309ebddea..3e8d84de36 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -587,7 +587,7 @@ ok(-f "$tempdir/backuponserver/base.tar", 'backup tar was created');
rmtree("$tempdir/backuponserver");
$node->command_ok(
- [qw(createuser --replication --role=pg_write_server_files backupuser)],
+ [qw(createuser --replication --in-role=pg_write_server_files backupuser)],
'create backup user');
$node->command_ok(
[ @pg_basebackup_defs, '-U', 'backupuser', '--target', "server:$tempdir/backuponserver", '-X', 'none' ],
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..0a31593055 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -31,7 +31,7 @@ main(int argc, char *argv[])
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
Re: Add --{no-,}bypassrls flags to createuser
On 2022-04-14 18:57, Daniel Gustafsson wrote:
On 14 Apr 2022, at 09:42, Shinya Kato
wrote:
To add the ROLE clause, the originally existing --role option
(corresponding to the IN ROLE clause) is changed to the --in-role
option. Would this not be good from a backward compatibility
standpoint?
- printf(_(" -g, --role=ROLE new role will be a member of
this role\n"));
+ printf(_(" -g, --in-role=ROLEnew role will be a member of
this role\n"));
+ printf(_(" -G, --role=ROLE this role will be a member of
new role\n"));
Won't this make existing scripts to behave differently after an
upgrade? That
seems like something we should avoid at all costs.
I understand. For backward compatibility, I left the ROLE clause option
as it is and changed the IN ROLE clause option to --membership option.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..20c60092c0 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+ The new role will be added immediately as a member with admin option
+ of this role.
+ Multiple roles to which new role will be added as a member with admin
+ option can be specified by writing multiple
+ -a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,19 @@ PostgreSQL documentation
+
+ -m role
+ --membership=role
+
+
+ The new role will be added immediately as a member of this role.
+ Multiple roles to which new role will be added as a member
+ can be specified by writing multiple
+ -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +285,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a timestamp after which the role's password is no longer valid.
+The default is to set no expiration.
+
+
+
+
-V
--version
@@ -290,6 +328,28 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will have the BYPASSRLS privilege,
+which is described more fully in the documentation for .
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not have the BYPASSRLS
+privilege, which is described more fully in the documentation for .
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..f7ec842cb7 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"membership", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -69,6 +74,9 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ SimpleStringList memberships = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
+ char *timestamp = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PEv:m:a:",
long_options, &optindex)) != -1)
{
switch (c)
@@ -165,6 +174,21 @@ main(int argc, char *argv[])
case 3:
interactive = true;
break;
+ case 4:
+bypassrls = TRI_YES;
+break;
+ case 5:
+bypassrls = TRI_NO;
+break;
+ case 'v':
+timestamp = pg_strdup(optarg);
+break;
+ case 'm':
+simple_string_list_append(&memberships, optarg);
+break;
+ case 'a':
Re: Add --{no-,}bypassrls flags to createuser
Thank you for the reviews!
On 2022-04-26 05:19, Nathan Bossart wrote:
- printf(_(" -g, --role=ROLE new role will be a member of
this role\n"));
+ printf(_(" -g, --role=ROLEnew role will be a member of this
role\n"));
This looks lik an unexpected change.
I fixed it.
I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later
meaning,
at least for me, the collective).
That -m doesn't match --role-to is no worse than -g not matching
--role, a
short option seems worthwhile, and the -m (membership) mnemonic should
be
simple to pick-up.
I don't see the addition of "-name" to the option name being
beneficial.
Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking
that
liberty for consistency here is very appealing and there isn't another
SQL
clause that it would be confused with.
+1 for "member". It might not be perfect, but IMO it's the clearest
option.
Thanks! I changed the option "--membership" to "--member".
For now, I also think "-m / --member" is the best choice, although it is
ambiguous:(
I'd like to hear others' opinions.
regards
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..189ca5bb67 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+ The new role will be added immediately as a member with admin option
+ of this role.
+ Multiple roles to which new role will be added as a member with admin
+ option can be specified by writing multiple
+ -a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,19 @@ PostgreSQL documentation
+
+ -m role
+ --member=role
+
+
+ The new role will be added immediately as a member of this role.
+ Multiple roles to which new role will be added as a member
+ can be specified by writing multiple
+ -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +285,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a timestamp after which the role's password is no longer valid.
+The default is to set no expiration.
+
+
+
+
-V
--version
@@ -290,6 +328,28 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will have the BYPASSRLS privilege,
+which is described more fully in the documentation for .
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not have the BYPASSRLS
+privilege, which is described more fully in the documentation for .
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..73cd1b479e 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"member", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -69,6 +74,9 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ SimpleStringList members = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
+ char *timestamp = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PEv:m:a:",
long_options, &am
Re: Set log_lock_waits=on by default
On 2023-12-22 20:00, Laurenz Albe wrote: My point is that in the vast majority of cases, long lock waits indicate a problem that you would like to know about, so the parameter should default to "on". +1. I always set log_lock_waits=on, so I agree with this. Just a random idea but what if we separated log_lock_waits from deadlock_timeout? Say, it becomes time-valued rather than Boolean-valued, but it has to be >= deadlock_timeout? Because I'd probably be more interested in hearing about a lock wait that was more than say 10 seconds, but I don't necessarily want to wait 10 seconds for the deadlock detector to trigger. That is an appealing thought, but as far as I know, "log_lock_waits" is implemented by the deadlock detector, which is why it is tied to "deadlock_timeout". So if we want that, we'd need a separate "live lock detector". I don't know if we want to go there. Personally, I thought it was a good idea to separate log_lock_waits and deadlock_timeout, but I have not checked how that is implemented. -- Regards, Shinya Kato NTT DATA GROUP CORPORATION
Re: Fix bug in VACUUM and ANALYZE docs
On 2023-09-19 17:59, Ryoga Yoshida wrote: Hi, Issue1: VACUUM and ANALYZE docs explain that the parameter of BUFFER_USAGE_LIMIT is optional as follows. But this is not true. The argument, size, is required for BUFFER_USAGE_LIMIT. So the docs should be fixed this issue. BUFFER_USAGE_LIMIT [ size ] https://www.postgresql.org/docs/devel/sql-vacuum.html https://www.postgresql.org/docs/devel/sql-analyze.html Issue2: Sizes may also be specified as a string containing the numerical size followed by any one of the following memory units: kB (kilobytes), MB (megabytes), GB (gigabytes), or TB (terabytes). VACUUM and ANALYZE docs explain that the argument of BUFFER_USAGE_LIMIT accepts the units like kB (kilobytes), MB (megabytes), GB (gigabytes), or TB (terabytes). But it also actually accepts B(bytes) as an unit. So the docs should include "B(bytes)" as an unit that the argument of BUFFER_USAGE_LIMIT can accept. You can see the patch in the attached file. Thanks for the patch. You're right. It looks good to me. -- Regards, Shinya Kato NTT DATA GROUP CORPORATION
Re: Add --{no-,}bypassrls flags to createuser
Thanks for reviews and comments!
On 2022-05-06 07:08, Przemysław Sztoch wrote:
Thanks for the new patch! Would you mind adding some tests for the new
options?
I created a new patch to test the new options!
However, not all option tests exist, so it may be necessary to consider
whether to actually add this test.
Too bad there's no --comment parameter to do COMMENT ON ROLE name IS
'Comment';
As you already make such changes in createuser, I would like to ask
for an additional --comment parameter
that will allow sysadmins to set a comment with additional information
about the new DB user.
psql is scary for some. :-)
Since the createuser command is a wrapper for the CREATE ROLE command, I
do not think it is appropriate to add options that the CREATE ROLE
command does not have.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..189ca5bb67 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+ The new role will be added immediately as a member with admin option
+ of this role.
+ Multiple roles to which new role will be added as a member with admin
+ option can be specified by writing multiple
+ -a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,19 @@ PostgreSQL documentation
+
+ -m role
+ --member=role
+
+
+ The new role will be added immediately as a member of this role.
+ Multiple roles to which new role will be added as a member
+ can be specified by writing multiple
+ -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +285,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a timestamp after which the role's password is no longer valid.
+The default is to set no expiration.
+
+
+
+
-V
--version
@@ -290,6 +328,28 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will have the BYPASSRLS privilege,
+which is described more fully in the documentation for .
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not have the BYPASSRLS
+privilege, which is described more fully in the documentation for .
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..73cd1b479e 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"member", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -69,6 +74,9 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ SimpleStringList members = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
+ char *timestamp = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PEv:m:a:",
long_options, &optindex)) != -1)
{
switch (c)
@@ -165,6 +174,21 @@ main(int argc, char *argv[])
case 3:
interactive = true;
break;
+ case 4:
+bypassrls = TRI_YES;
+break;
+ case 5:
+bypassrls = TRI_NO;
+break;
+ case 'v':
+timestamp = pg_strdup(optarg);
+break;
+ case 'm':
+simple_string_list_append(&members, optarg);
+break;
+ case 'a':
+simple_string_list_append(&admins, optarg);
+break;
default:
/* ge
Re: Add --{no-,}bypassrls flags to createuser
On 2022-05-21 06:45, Nathan Bossart wrote: On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote: I created a new patch to test the new options! Thanks for the new patch! I attached a new version with a few small changes. What do you think? Thanks for updating the patch! It looks good to me. On 2022-05-23 10:32, Kyotaro Horiguchi wrote: Anyway, after fixing that issue we will modify the createrole command so that it uses the new SQL feature. I find no hard obstacles in reaching there in the 16 cycle. +1. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add --{no-,}bypassrls flags to createuser
On 2022-05-23 16:29, Michael Paquier wrote:
+$node->issues_sql_like(
+ [ 'createuser', 'regress_role2', '-a', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_role2 NOSUPERUSER NOCREATEDB
NOCREATEROLE INHERIT LOGIN ADMIN regress_user1;/,
+ 'add a role as a member with admin option of the newly created
role');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_role3', '-m', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_role3 NOSUPERUSER NOCREATEDB
NOCREATEROLE INHERIT LOGIN ROLE regress_user1;/,
+ 'add a role as a member of the newly created role');
May I ask for the addition of tests when one specifies multiple
switches for --admin and --member? This would check the code path
where you build a list of role names. You could check fancier string
patterns, while on it, to look after the use of fmtId(), say with
role names that include whitespaces or such.
Thanks!
I changed to the test that describes multiple "-m".
It seems to be working without any problems, how about it?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..27efebbfe2 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+Indicates role that will be immediately added as a member of the new
+role with admin option, giving it the right to grant membership in the
+new role to others. Multiple roles to add as members (with admin
+option) of the new role can be specified by writing multiple
+-a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,18 @@ PostgreSQL documentation
+
+ -m role
+ --member=role
+
+
+Indicates role that will be immediately added as a member of the new
+role. Multiple roles to add as members of the new role can be specified
+by writing multiple -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +284,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a date and time after which the role's password is no longer valid.
+The default is to set no password expiry date.
+
+
+
+
-V
--version
@@ -290,6 +327,25 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will bypass every row-level security (RLS) policy.
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not bypass row-level security (RLS) policies. This is
+the default.
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..7e3e61a9c8 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"member", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -62,6 +67,8 @@ main(int argc, char *argv[])
char *port = NULL;
char *username = NULL;
SimpleStringList roles = {NULL, NULL};
+ SimpleStringList members = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
enum trivalue prompt_password = TRI_DEFAULT;
ConnParams cparams;
bool echo = false;
@@ -69,6 +76,7 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ char *pwexpiry = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSr
Re: Add --{no-,}bypassrls flags to createuser
On 2022-05-24 11:09, Michael Paquier wrote:
On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
Michael also requested a test for multiple -a switches and for fancier
string patterns. Once that is taken care of, I think this can be
marked as
ready-for-committer.
Looking at v7, this means to extend the tests to process lists for
--admin with more name patterns. And while on it, we could do the
same for the existing command for --role, but this one is on me, being
overly-pedantic while looking at the patch :)
Thanks! I fixed it.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..27efebbfe2 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+Indicates role that will be immediately added as a member of the new
+role with admin option, giving it the right to grant membership in the
+new role to others. Multiple roles to add as members (with admin
+option) of the new role can be specified by writing multiple
+-a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,18 @@ PostgreSQL documentation
+
+ -m role
+ --member=role
+
+
+Indicates role that will be immediately added as a member of the new
+role. Multiple roles to add as members of the new role can be specified
+by writing multiple -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +284,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a date and time after which the role's password is no longer valid.
+The default is to set no password expiry date.
+
+
+
+
-V
--version
@@ -290,6 +327,25 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will bypass every row-level security (RLS) policy.
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not bypass row-level security (RLS) policies. This is
+the default.
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..7e3e61a9c8 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"member", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -62,6 +67,8 @@ main(int argc, char *argv[])
char *port = NULL;
char *username = NULL;
SimpleStringList roles = {NULL, NULL};
+ SimpleStringList members = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
enum trivalue prompt_password = TRI_DEFAULT;
ConnParams cparams;
bool echo = false;
@@ -69,6 +76,7 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ char *pwexpiry = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PEv:m:a:",
long_options, &optindex)) != -1)
{
switch (c)
@@ -165,6 +174,21 @@ main(int argc, char *argv[])
case 3:
interactive = true;
break;
+ case 4:
+bypassrls = TRI_YES;
+break;
+ case 5:
+bypassrls = TRI_NO;
+break;
+ case 'v':
+pwexpiry = pg_strdup(optarg);
+break;
+ case 'm':
+simple_string_list_append(&members, optarg);
+break;
+ case 'a':
+simple_string_list_append(&admins, optarg);
+break;
default:
/* getopt_long already emitted
Re: Add --{no-,}bypassrls flags to createuser
On 2022-05-25 12:47, Michael Paquier wrote:
On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
I reproduced the same failure at my hand and identified the
cause. Windows' version of getopt_long seems to dislike that
non-optional parameters precedes options.
Tweaking the list of arguments in some commands kicked by the TAP
tests to satisfy our implementation of getopt_long() has been the
origin of a couple of portability fixes, like ffd3980.
Thanks! I fixed it.
On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
wrote in
We're still missing some "fancier" string patterns in the tests, but
we
might just be nitpicking at this point.
Such "fancier" strings should be properly handled by FmtId() and
appendStringLiteralConn. If this is a privilege escalating command,
we should have ones but this is not.
Sorry, I didn't quite understand the "fancier" pattern. Is a string like
this patch correct?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 17579e50af..27efebbfe2 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -76,6 +76,20 @@ PostgreSQL documentation
+
+ -a role
+ --admin=role
+
+
+Indicates role that will be immediately added as a member of the new
+role with admin option, giving it the right to grant membership in the
+new role to others. Multiple roles to add as members (with admin
+option) of the new role can be specified by writing multiple
+-a switches.
+
+
+
+
-c number
--connection-limit=number
@@ -204,6 +218,18 @@ PostgreSQL documentation
+
+ -m role
+ --member=role
+
+
+Indicates role that will be immediately added as a member of the new
+role. Multiple roles to add as members of the new role can be specified
+by writing multiple -m switches.
+
+
+
+
-P
--pwprompt
@@ -258,6 +284,17 @@ PostgreSQL documentation
+
+ -v timestamp
+ --valid-until=timestamp
+
+
+Set a date and time after which the role's password is no longer valid.
+The default is to set no password expiry date.
+
+
+
+
-V
--version
@@ -290,6 +327,25 @@ PostgreSQL documentation
+
+ --bypassrls
+
+
+The new user will bypass every row-level security (RLS) policy.
+
+
+
+
+
+ --no-bypassrls
+
+
+The new user will not bypass row-level security (RLS) policies. This is
+the default.
+
+
+
+
-?
--help
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index bfba0d09d1..7e3e61a9c8 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -51,6 +51,11 @@ main(int argc, char *argv[])
{"connection-limit", required_argument, NULL, 'c'},
{"pwprompt", no_argument, NULL, 'P'},
{"encrypted", no_argument, NULL, 'E'},
+ {"bypassrls", no_argument, NULL, 4},
+ {"no-bypassrls", no_argument, NULL, 5},
+ {"valid-until", required_argument, NULL, 'v'},
+ {"member", required_argument, NULL, 'm'},
+ {"admin", required_argument, NULL, 'a'},
{NULL, 0, NULL, 0}
};
@@ -62,6 +67,8 @@ main(int argc, char *argv[])
char *port = NULL;
char *username = NULL;
SimpleStringList roles = {NULL, NULL};
+ SimpleStringList members = {NULL, NULL};
+ SimpleStringList admins = {NULL, NULL};
enum trivalue prompt_password = TRI_DEFAULT;
ConnParams cparams;
bool echo = false;
@@ -69,6 +76,7 @@ main(int argc, char *argv[])
int conn_limit = -2; /* less than minimum valid value */
bool pwprompt = false;
char *newpassword = NULL;
+ char *pwexpiry = NULL;
/* Tri-valued variables. */
enum trivalue createdb = TRI_DEFAULT,
@@ -76,7 +84,8 @@ main(int argc, char *argv[])
createrole = TRI_DEFAULT,
inherit = TRI_DEFAULT,
login = TRI_DEFAULT,
-replication = TRI_DEFAULT;
+replication = TRI_DEFAULT,
+bypassrls = TRI_DEFAULT;
PQExpBufferData sql;
@@ -89,7 +98,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "createuser", help);
- while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE",
+ while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PEv:m:a:",
long_options, &optindex)) != -1)
{
switch (c)
@@ -165,6 +174,21 @@ main(int argc, char *argv
Fix inconsistencies GUC categories
Hi,
It appears that config.sgml and pg_settings have not been updated, even
though a new subcategory was added in 249d649. a55a984 may have been
missed in the cleaning.
--
Category is 'CONNECTIONS AND AUTHENTICATION' and subcategory is
'Connection Settings' at config.sgml.
Category is 'CONNECTIONS AND AUTHENTICATION' and subcategory is
'Connection Settings' at pg_settings.
Category is 'CONNECTIONS AND AUTHENTICATION' and subcategory is 'TCP
settings' at postgresql.conf.sample.
--
I would like to unify the following with config.sgml as in a55a984.
--
Category is 'REPORTING AND LOGGING' and subcategory is 'PROCESS TITLE'
at config.sgml.
Category is 'REPORTING AND LOGGING' and subcategory is 'PROCESS TITLE'
at pg_settings.
Category is 'PROCESS TITLE' and subcategory is none at
postgresql.conf.sample.
--
Trivial changes were made to the following short_desc.
--
recovery_prefetch
enable_group_by_reordering
stats_fetch_consistency
--
I've attached a patch.
Thoghts?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e2d728e0c4..2522f4c8c5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -896,6 +896,13 @@ include_dir 'conf.d'
+
+
+
+
+ TCP Settings
+
+
tcp_keepalives_idle (integer)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index af4a1c3068..5db5df6285 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -778,6 +778,8 @@ const char *const config_group_names[] =
gettext_noop("File Locations"),
/* CONN_AUTH_SETTINGS */
gettext_noop("Connections and Authentication / Connection Settings"),
+ /* CONN_AUTH_TCP */
+ gettext_noop("Connections and Authentication / TCP Settings"),
/* CONN_AUTH_AUTH */
gettext_noop("Connections and Authentication / Authentication"),
/* CONN_AUTH_SSL */
@@ -1216,7 +1218,7 @@ static struct config_bool ConfigureNamesBool[] =
},
{
{"enable_group_by_reordering", PGC_USERSET, QUERY_TUNING_METHOD,
- gettext_noop("enable reordering of GROUP BY key"),
+ gettext_noop("Enable reordering of GROUP BY key."),
NULL,
GUC_EXPLAIN
},
@@ -3460,7 +3462,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"tcp_keepalives_idle", PGC_USERSET, CONN_AUTH_SETTINGS,
+ {"tcp_keepalives_idle", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("Time between issuing TCP keepalives."),
gettext_noop("A value of 0 uses the system default."),
GUC_UNIT_S
@@ -3471,7 +3473,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"tcp_keepalives_interval", PGC_USERSET, CONN_AUTH_SETTINGS,
+ {"tcp_keepalives_interval", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("Time between TCP keepalive retransmits."),
gettext_noop("A value of 0 uses the system default."),
GUC_UNIT_S
@@ -3493,7 +3495,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"tcp_keepalives_count", PGC_USERSET, CONN_AUTH_SETTINGS,
+ {"tcp_keepalives_count", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("Maximum number of TCP keepalive retransmits."),
gettext_noop("This controls the number of consecutive keepalive retransmits that can be "
"lost before a connection is considered dead. A value of 0 uses the "
@@ -3595,7 +3597,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"tcp_user_timeout", PGC_USERSET, CONN_AUTH_SETTINGS,
+ {"tcp_user_timeout", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("TCP user timeout."),
gettext_noop("A value of 0 uses the system default."),
GUC_UNIT_MS
@@ -3640,7 +3642,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"client_connection_check_interval", PGC_USERSET, CONN_AUTH_SETTINGS,
+ {"client_connection_check_interval", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("Sets the time interval between checks for disconnection while running queries."),
NULL,
GUC_UNIT_MS
@@ -4953,7 +4955,7 @@ static struct config_enum ConfigureNamesEnum[] =
{
{"stats_fetch_consistency", PGC_USERSET, STATS_CUMULATIVE,
- gettext_noop("Sets the consistency of accesses to statistics data"),
+ gettext_noop("Sets the consistency of accesses to statistics data."),
NULL
},
&pgstat_fetch_consistency,
@@ -5044,7 +5046,7 @@ static struct config_enum ConfigureNamesEnum[] =
{
{"recovery_prefetch", PGC_SIGHUP, WAL_RECOVERY,
- gettext_noop("Prefetch referenced b
Re: (LOCK TABLE options) “ONLY” and “NOWAIT” are not yet implemented
2021-09-28 17:06 に bt21tanigaway さんは書きました: 2021-09-28 17:03 に bt21tanigaway さんは書きました: 2021-09-28 16:36 に Fujii Masao さんは書きました: On 2021/09/28 16:13, bt21tanigaway wrote: Hi, (LOCK TABLE options) “ONLY” and “NOWAIT” are not yet implemented in tab-complete. I made a patch for these options. Thanks for the patch! The patch seems to forget to handle the tab-completion for "LOCK ONLY IN". Thanks for your comment! I attach a new patch fixed to this mail. Regards, Koyu Tanigawa Sorry, I forgot to attach patch file. "fix-tab-complete2.patch" is fixed! Regards, Koyu Tanigawa Thank you for your patch. I have two comments. 1. When I executed git apply, an error occured. --- $ git apply ~/Downloads/fix-tab-complete2.patch /home/penguin/Downloads/fix-tab-complete2.patch:14: indent with spaces. COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, " UNION SELECT 'TABLE'" " UNION SELECT 'ONLY'"); warning: 1 line adds whitespace errors. --- 2. The command "LOCK TABLE a, b;" can be executed, but tab-completion doesn't work properly. Is it OK? -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion
On 2021-10-07 17:14, katouknl wrote: Hi, Thank you for the review. I wasn't quite sure where to start counting the characters, but I used pgindent to properly format it, so hopefully everything is okay. Also, I sorted them in alphabetical order just to make it look prettier. * The below change crosses the 80-character limit in a line. Please maintain the same. - "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "ROLE"); + "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "TRANSFORM FOR", "ROLE"); I made sure there is no whitespaces this time. * There are trailing whitespaces after COMPLETE_WITH_QUERY(Query_for_list_of_languages);. Remove these extra whitespaces. surajkhamkar@localhost:tab_comment$ git apply fix_tab_complete_comment.patch fix_tab_complete_comment.patch:38: trailing whitespace. COMPLETE_WITH_QUERY(Query_for_list_of_languages); warning: 1 line adds whitespace errors. Thank you for the patch! It is very good, but it seems to me that there are some tab-completion missing in COMMENT command. For example, - CONSTRAINT ... ON DOMAIN - OPERATOR CLASS - OPERATOR FAMILY - POLICY ... ON - [PROCEDURAL] - RULE ... ON - TRIGGER ... ON I think these tab-comletion also can be improved and it's a good timing for that. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fix typo in func.sgml
Hi hackers, I've found a duplicate "a a" in func.sgml and fixed it. Patch is attached. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 8dd63c0455..f87afefeae 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -18033,7 +18033,7 @@ FROM or array, but if it is CONDITIONAL it will not be applied to a single array or object. UNCONDITIONAL is the default. -If the result is a a scalar string, by default the value returned will have +If the result is a scalar string, by default the value returned will have surrounding quotes making it a valid JSON value. However, this behavior is reversed if OMIT QUOTES is specified. The ON ERROR and ON EMPTY
Re: Fix typo in func.sgml
On 2022-08-24 20:47, David Rowley wrote: On Wed, 24 Aug 2022 at 22:44, Shinya Kato wrote: I've found a duplicate "a a" in func.sgml and fixed it. Patch is attached. Thanks. Pushed. David Thanks for pushing! -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fix japanese translation of log messages
Hi hackers, I've found typos in ja.po, and fixed them. The patch is attached. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONdiff --git a/src/backend/po/ja.po b/src/backend/po/ja.po index 0925465d22..189000792c 100644 --- a/src/backend/po/ja.po +++ b/src/backend/po/ja.po @@ -1492,7 +1492,7 @@ msgstr "ãã©ã¬ã«ã¯ã¼ã«ã®åæåã«å¤±æãã¾ãã" #: access/transam/parallel.c:719 access/transam/parallel.c:838 #, c-format msgid "More details may be available in the server log." -msgstr "詳細ãªæ å ±ãã¯ãµã¼ããã°ã«ããããããã¾ããã" +msgstr "詳細ãªæ å ±ã¯ãµã¼ããã°ã«ããããããã¾ããã" #: access/transam/parallel.c:899 #, c-format @@ -1841,7 +1841,7 @@ msgid "" "Stop the postmaster and vacuum that database in single-user mode.\n" "You might also need to commit or roll back old prepared transactions, or drop stale replication slots." msgstr "" -"postmaster ãåæ¢å¾ãã·ã³ã°ã«ã¦ã¼ã¶ã¢ã¼ãã§ãã¼ã¿ãã¼ã¹ãVACUUMãå®è¡ãã¦ãã ããã\n" +"postmaster ãåæ¢å¾ãã·ã³ã°ã«ã¦ã¼ã¶ã¢ã¼ãã§ãã¼ã¿ãã¼ã¹ã«VACUUMãå®è¡ãã¦ãã ããã\n" "å¤ãæºåæ¸ã¿ãã©ã³ã¶ã¯ã·ã§ã³ã®ã³ãããã¾ãã¯ãã¼ã«ããã¯ããããã¯å¤ãã¬ããªã±ã¼ã·ã§ã³ã¹ãããã®åé¤ãå¿ è¦ããããã¾ããã" #: access/transam/varsup.c:136 @@ -12183,7 +12183,7 @@ msgstr "失æããè¡ã¯%sãå«ã¿ã¾ã" #: executor/execMain.c:1970 #, c-format msgid "null value in column \"%s\" of relation \"%s\" violates not-null constraint" -msgstr "ãªã¬ã¼ã·ã§ã³\"%2$s\"ã®å\"%1$s\"ã®NULLå¤ããéNULLå¶ç´ã«éåãã¦ãã¾ã" +msgstr "ãªã¬ã¼ã·ã§ã³\"%2$s\"ã®å\"%1$s\"ã®NULLå¤ãéNULLå¶ç´ã«éåãã¦ãã¾ã" #: executor/execMain.c:2021 #, c-format @@ -17583,7 +17583,7 @@ msgstr "CREATE TABLE ã§ã¯æ¢åã®ã¤ã³ããã¯ã¹ã使ãã¾ãã" #: parser/parse_utilcmd.c:2261 #, c-format msgid "index \"%s\" is already associated with a constraint" -msgstr "ã¤ã³ããã¯ã¹\"%s\"ã¯ãã§ã«1ã¤ã®å¶ç´ã«å²ãå½ã¦ããããã¾ã" +msgstr "ã¤ã³ããã¯ã¹\"%s\"ã¯ãã§ã«1ã¤ã®å¶ç´ã«å²ãå½ã¦ããã¦ãã¾ã" #: parser/parse_utilcmd.c:2276 #, c-format @@ -25012,7 +25012,7 @@ msgstr "è¨å®å\"%s\"ãNULLã«ãããã¨ã¯ã§ãã¾ãã" #: utils/adt/tsvector_op.c:2665 #, c-format msgid "text search configuration name \"%s\" must be schema-qualified" -msgstr "ããã¹ãæ¤ç´¢è¨å®å称\"%s\"ã¯ã¹ãã¼ã修飾ããªãããããã¾ãã" +msgstr "ããã¹ãæ¤ç´¢è¨å®å称\"%s\"ã¯ã¹ãã¼ã修飾ããªããã°ãªãã¾ãã" #: utils/adt/tsvector_op.c:2690 #, c-format @@ -26503,7 +26503,7 @@ msgstr "åãåããã®ãã¼ã¹ããªã¼ããã°ã«è¨é²ãã¾ãã" #: utils/misc/guc.c:1498 msgid "Logs each query's rewritten parse tree." -msgstr "ãªã©ã¤ãå¾ã®åãåããã®ãã¼ã¹ããªã¼ããã°ãè¨é²ãã¾ãã" +msgstr "ãªã©ã¤ãå¾ã®åãåããã®ãã¼ã¹ããªã¼ããã°ã«è¨é²ãã¾ãã" #: utils/misc/guc.c:1507 msgid "Logs each query's execution plan." @@ -28443,7 +28443,7 @@ msgstr "åºå®ããããã¼ã¿ã«\"%s\"ã¯åé¤ã§ãã¾ãã" #: utils/mmgr/portalmem.c:488 #, c-format msgid "cannot drop active portal \"%s\"" -msgstr "ã¢ã¯ãã¤ããªãã¼ã¿ã«\"%s\"ãåé¤ã§ãã¾ãã" +msgstr "ã¢ã¯ãã£ããªãã¼ã¿ã«\"%s\"ãåé¤ã§ãã¾ãã" #: utils/mmgr/portalmem.c:739 #, c-format
Re: Fix japanese translation of log messages
On 2022-08-26 14:07, Kyotaro Horiguchi wrote: At Fri, 26 Aug 2022 10:23:01 +0900, Shinya Kato wrote in I've found typos in ja.po, and fixed them. The patch is attached. (This is not for -hackers but I'm fine with it being posted here;p) Sorry, I didn't know there was an pgsql-translators. Thanks for the report! Pushed to 10 to 15 of translation repository with some minor changes. They will be reflected in the code tree some time later. Thanks! msgid "More details may be available in the server log." -msgstr "詳細な情報がはサーバログにあるかもしれません。" +msgstr "詳細な情報はサーバログにあるかもしれません。" I prefer "詳細な情報が" than "詳細な情報は" here. (The existnce of the details is unknown here.) msgid "cannot drop active portal \"%s\"" -msgstr "アクテイブなポータル\"%s\"を削除できません" +msgstr "アクティブなポータル\"%s\"を削除できません" I canged it to "アクティブなポータル\"%s\"は削除できません". (It describes state facts, not telling the result of an action.) Thanks, LGTM. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH] Tab completion for SET COMPRESSION
On 2022-08-22 21:48, Aleksander Alekseev wrote: Hi hackers, The proposed patch adds the missing tab completion for 'ALTER TABLE ... SET COMPRESSION ...' syntax. Thanks, LGTM. In addition, why not take this opportunity to create a tab completion for "ALTER TABLE OF " and "ALTER TABLE NOT OF"? -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH] Tab completion for SET COMPRESSION
On 2022-09-06 17:28, Aleksander Alekseev wrote:
In addition, why not take this opportunity to create a tab completion
for
"ALTER TABLE OF " and "ALTER TABLE NOT OF"?
Thanks for reviewing, Shinya. Let's fix this too. The patch is
attached.
Thanks for the new patch!
A minor modification has been made so that the composite type is also
completed after "ALTER TABLE OF".
Thought?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 62a39779b9..053c0ea75c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2240,7 +2240,8 @@ psql_completion(const char *text, int start, int end)
"ENABLE", "INHERIT", "NO", "RENAME", "RESET",
"OWNER TO", "SET", "VALIDATE CONSTRAINT",
"REPLICA IDENTITY", "ATTACH PARTITION",
- "DETACH PARTITION", "FORCE ROW LEVEL SECURITY");
+ "DETACH PARTITION", "FORCE ROW LEVEL SECURITY",
+ "OF", "NOT OF");
/* ALTER TABLE xxx ADD */
else if (Matches("ALTER", "TABLE", MatchAny, "ADD"))
{
@@ -2469,6 +2470,10 @@ psql_completion(const char *text, int start, int end)
else if (Matches("ALTER", "TABLE", MatchAny, "DETACH", "PARTITION", MatchAny))
COMPLETE_WITH("CONCURRENTLY", "FINALIZE");
+ /* ALTER TABLE OF */
+ else if (TailMatches("ALTER", "TABLE", MatchAny, "OF"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_composite_datatypes);
+
/* ALTER TABLESPACE with RENAME TO, OWNER TO, SET, RESET */
else if (Matches("ALTER", "TABLESPACE", MatchAny))
COMPLETE_WITH("RENAME TO", "OWNER TO", "SET", "RESET");
Re: [PATCH] Tab completion for SET COMPRESSION
On 2022-09-06 20:57, Aleksander Alekseev wrote: Hi Shinya, A minor modification has been made so that the composite type is also completed after "ALTER TABLE OF". LGTM. Here is v3 created with `git format-path`. Unlike v2 it also includes the commit message. Thanks! I marked it as ready for committer. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022-09-09 11:18, bt22kawamotok wrote:
I created a patch for improving MARGE tab completion.
Currently there is a problem with "MERGE INTO dst as d Using src as s
ON d.key = s.key WHEN " is typed, "MATCHED" and "NOT MATCHED" is
not completed.
There is also a problem that typing "MERGE INTO a AS " completes
"USING".
This patch solves the above problems.
Thanks for the patch!
else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
else if (TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny,
"WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
else if (TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
"WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
I thought it would be better to describe this section as follows,
summarizing the conditions
else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN") ||
TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny,
"WHEN") ||
TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
"WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
There are similar redundancies in the tab completion of MERGE statement,
so why not fix that as well?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022-09-12 15:53, bt22kawamotok wrote:
else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
else if (TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny, "WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
else if (TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
"WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
I thought it would be better to describe this section as follows,
summarizing the conditions
else if (TailMatches("USING", MatchAny, "ON", MatchAny, "WHEN") ||
TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny,
"WHEN") ||
TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
"WHEN"))
COMPLETE_WITH("MATCHED", "NOT MATCHED");
There are similar redundancies in the tab completion of MERGE
statement, so why not fix that as well?
Thanks for your review.
A new patch has been created to reflect the changes you indicated.
Thanks for updating!
Compile errors have occurred, so can you fix them?
And I think we can eliminate similar redundancies in MERGE tab
completion and I would like you to fix them.
For example,
else if (TailMatches("WHEN", "MATCHED"))
COMPLETE_WITH("THEN", "AND");
else if (TailMatches("WHEN", "NOT", "MATCHED"))
COMPLETE_WITH("THEN", "AND");
above statement can be converted to the statement below.
else if (TailMatches("WHEN", "MATCHED") ||
TailMatches("WHEN", "NOT", "MATCHED"))
COMPLETE_WITH("THEN", "AND");
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022-09-12 18:20, bt22kawamotok wrote:
Other than this correction, the parts that can be put together in OR
were corrected in fix_tab_completion_merge_v4.patch.
When I tried to apply this patch, I got the following warning, please
fix it.
Other than that, I think everything is fine.
$ git apply fix_tab_completion_merge_v4.patch
fix_tab_completion_merge_v4.patch:38: trailing whitespace.
else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
fix_tab_completion_merge_v4.patch:39: indent with spaces.
TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
fix_tab_completion_merge_v4.patch:40: indent with spaces.
TailMatches("USING", MatchAny, MatchAny, "ON",
MatchAny))
fix_tab_completion_merge_v4.patch:53: trailing whitespace.
else if (TailMatches("WHEN", "MATCHED") ||
warning: 4 lines add whitespace errors.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022-09-14 18:12, bt22kawamotok wrote:
I fixed it in v6.
Thanks for updating.
+ COMPLETE_WITH("UPDATE", "DELETE", "DO NOTHING");
"UPDATE" is always followed by "SET", so why not complement it with
"UPDATE SET"?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Set AUTOCOMMIT to on in script output by pg_dump
Hi hackers! When SQL scripts created with pg_dump/pg_dumpall/pg_restore are executed in psql with AUTOCOMMIT turned off, they will not succeed in many cases. This is because the script contains SQL statements that cannot be executed within a transaction block. If you simply add set AUTOCOMMIT on to the scripts created by pg_dump/pg_dumpall/pg_restore, they will work fine. A patch is attached No documentation has been added as we could not find any documentation on the details in the script. Do you think? Regards, Shinya Kato NTT DATA GROUP CORPORATIONFrom e5b4a6ab95ab5c798ef8c7f7062fb764a74de37a Mon Sep 17 00:00:00 2001 From: Shinya Kato Date: Wed, 9 Oct 2024 10:28:31 +0900 Subject: [PATCH v1] Set AUTOCOMMIT to on in script output by pg_dump --- src/bin/pg_dump/pg_backup_archiver.c | 7 +++ src/bin/pg_dump/pg_dumpall.c | 6 ++ 2 files changed, 13 insertions(+) diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 8c20c263c4..b1077a5521 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -472,6 +472,13 @@ RestoreArchive(Archive *AHX) ahprintf(AH, "BEGIN;\n\n"); } + /* + * Set AUTOCOMMIT to on when dumping a plain-text file to prevent errors + * with SQL statements that cannot be executed inside transaction block. + */ + if (AH->format == archNull || ropt->filename != NULL) + ahprintf(AH, "\\set AUTOCOMMIT on\n"); + /* * Establish important parameter values right away. */ diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index e3ad8fb295..006d0201bc 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -553,6 +553,12 @@ main(int argc, char *argv[]) * database we're connected to at the moment is fine. */ + /* + * Set AUTOCOMMIT to on to prevent errors with SQL statements that cannot be + * executed inside transaction block. + */ + fprintf(OPF, "\\set AUTOCOMMIT on\n\n"); + /* Restore will need to write to the target cluster */ fprintf(OPF, "SET default_transaction_read_only = off;\n\n"); -- 2.43.0
Re: Set AUTOCOMMIT to on in script output by pg_dump
On 2024-10-10 14:56, Shinya Kato wrote: A new patch is attached. I am not a native English, so corrections to the texts are welcome. I created a commit fest entry. https://commitfest.postgresql.org/50/5306/ -- Regards, Shinya Kato NTT DATA GROUP CORPORATION
Re: Set AUTOCOMMIT to on in script output by pg_dump
Thank you all for the comments! On 2024-10-09 15:15, Yugo Nagata wrote: On Tue, 8 Oct 2024 21:37:38 -0700 "David G. Johnston" wrote: On Tuesday, October 8, 2024, Tom Lane wrote: > "David G. Johnston" writes: > > On Tuesday, October 8, 2024, Yugo Nagata wrote: > >> On Wed, 09 Oct 2024 11:10:37 +0900 > >> Shinya Kato wrote: > >>> When SQL scripts created with pg_dump/pg_dumpall/pg_restore are > executed > >>> in psql with AUTOCOMMIT turned off, they will not succeed in many > cases. > > > Agreed. If we aren’t already outputting psql-only stuff I am a strong -1 > > for making this the first such case. > > I really doubt that this is the only way in which you can break a > pg_dump script by executing it in a non-default psql environment. > We'd likely be better advised to spend some documentation effort > recommending that pg_dump scripts be executed under "psql --no-psqlrc". +1 Reinforcing that our output script basically assumes a default execution environment seems worth mentioning even if it seems self-evident once it’s said. +1 While adding to the documentation is sufficient if users use it correctly, users often behave unexpectedly. My intention was to implement it in a way that works without issues even if misused. However, since the prevailing opinion seems to favor simply updating the documentation, I will proceed with that approach. A new patch is attached. I am not a native English, so corrections to the texts are welcome. -- Regards, Shinya Kato NTT DATA GROUP CORPORATIONFrom f2ac5156b8fc02b6b529d3aeef16d80f21bbe08c Mon Sep 17 00:00:00 2001 From: Shinya Kato Date: Thu, 10 Oct 2024 14:50:39 +0900 Subject: [PATCH v2] doc: Add --no-psqlrc in pg_dump/pg_dumpall docs --- doc/src/sgml/backup.sgml | 16 +--- doc/src/sgml/ref/pg_dump.sgml| 9 - doc/src/sgml/ref/pg_dumpall.sgml | 11 ++- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index e4e4c56cf1..21ad34f4e0 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -106,10 +106,10 @@ pg_dump dbname > Text files created by pg_dump are intended to -be read in by the psql program. The -general command form to restore a dump is +be read in by the psql program with its default +settings. The general command form to restore a dump is -psql dbname < dumpfile +psql --no-psqlrc dbname < dumpfile where dumpfile is the file output by the pg_dump command. The database dbname < template0 before executing psql (e.g., with createdb -T template0 dbname). psql +class="parameter">dbname). To use +psql with its default settings, use the +--no-psqlrc option. psql supports options similar to pg_dump for specifying the database server to connect to and the user name to use. See the reference page for more information. @@ -141,7 +143,7 @@ psql dbname < psql exit with an exit status of 3 if an SQL error occurs: -psql --set ON_ERROR_STOP=on dbname < dumpfile +psql --no-psqlrc --set ON_ERROR_STOP=on dbname < dumpfile Either way, you will only have a partially restored database. Alternatively, you can specify that the whole dump should be @@ -160,7 +162,7 @@ psql --set ON_ERROR_STOP=on dbname < write to or read from pipes makes it possible to dump a database directly from one server to another, for example: -pg_dump -h host1 dbname | psql -h host2 dbname +pg_dump -h host1 dbname | psql --no-psqlrc -h host2 dbname @@ -205,7 +207,7 @@ pg_dumpall > dumpfile The resulting dump can be restored with psql: -psql -f dumpfile postgres +psql --no-psqlrc -f dumpfile postgres (Actually, you can specify any existing database name to start from, but if you are loading into an empty cluster then postgres diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index ffc29b04fb..b6c33c3c2f 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -1636,6 +1636,13 @@ CREATE DATABASE foo WITH TEMPLATE template0; option will be automatically enabled by the subscriber if the subscription had been originally created with two_phase = true option. + + + It is generally recommended to use the --no-psqlrc option + when restoring a database from a pg_dump script + to ensure a clean restore process and prevent potential conflicts with + existing psql configurations. + @@ -1653,7 +1660,7 @@ CREATE DATABASE foo WITH TEMPLATE template0; newdb: -$ psql -d newdb -f db.sql +$ psql --no-psqlrc -d newdb -f db.sql diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml index 4d7c046468..f8db63326a 100644 --- a/doc/src/sgml/ref/pg_dumpall.sg
Re: Set AUTOCOMMIT to on in script output by pg_dump
Hi, thank you for the reviews. On 2025-01-18 00:45, Robert Treat wrote: This looks pretty good to me. I think there are a couple of minor grammar changes that could be made, and I think the pg_dumpall section could use a couple tweaks, specifically 1) not all incantations of pg_dumpall emit psql meta commands (-g comes to mind quickly) and ISTR some non-psql clients honor psql meta commands, so I would lessen the language around incompatibility, and 2) I think adding an explicit -f for the database name in the pg_dumpall is clearer, and mirrors the pg_dump example. Thanks, I had no reason to oppose it, so I reflected that in the patch. On 2025-01-18 05:11, Tom Lane wrote: Robert Treat writes: suggested diffs attached, let me know if you would like a consolidated patch Sadly, the cfbot is now confused since it doesn't understand the idea of an incremental patch. Somebody please post a consolidated patch. I've attached new consolidated patch. For myself, I'd suggest writing the examples with -X not --no-psqlrc. -X is shorter, it's more likely to be what people would actually type, and it's not jarringly inconsistent with the adjacent use of -h and other short-form switches. We can write the added paragraphs like It is generally recommended to use the -X (--no-psqlrc) option when restoring a database ... to provide clarity about what the switch does. I agree to it and fixed the patch. -- Regards, Shinya Kato NTT DATA GROUP CORPORATIONFrom 664f89f8467f356024fe6e4e4ac0369b65238d41 Mon Sep 17 00:00:00 2001 From: Shinya Kato Date: Wed, 22 Jan 2025 21:53:19 +0900 Subject: [PATCH v3] doc: Add --no-psqlrc in pg_dump pg_dumpall docs --- doc/src/sgml/backup.sgml | 16 +--- doc/src/sgml/ref/pg_dump.sgml| 10 +- doc/src/sgml/ref/pg_dumpall.sgml | 14 -- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index e4e4c56cf1..dd38a8b220 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -106,10 +106,10 @@ pg_dump dbname > Text files created by pg_dump are intended to -be read in by the psql program. The -general command form to restore a dump is +be read in by the psql program using its default +settings. The general command form to restore a dump is -psql dbname < dumpfile +psql -X dbname < dumpfile where dumpfile is the file output by the pg_dump command. The database dbname < template0 before executing psql (e.g., with createdb -T template0 dbname). psql +class="parameter">dbname). To use +psql with its default settings, use the +-X (--no-psqlrc) option. psql supports options similar to pg_dump for specifying the database server to connect to and the user name to use. See the reference page for more information. @@ -141,7 +143,7 @@ psql dbname < psql exit with an exit status of 3 if an SQL error occurs: -psql --set ON_ERROR_STOP=on dbname < dumpfile +psql -X --set ON_ERROR_STOP=on dbname < dumpfile Either way, you will only have a partially restored database. Alternatively, you can specify that the whole dump should be @@ -160,7 +162,7 @@ psql --set ON_ERROR_STOP=on dbname < write to or read from pipes makes it possible to dump a database directly from one server to another, for example: -pg_dump -h host1 dbname | psql -h host2 dbname +pg_dump -h host1 dbname | psql -X -h host2 dbname @@ -205,7 +207,7 @@ pg_dumpall > dumpfile The resulting dump can be restored with psql: -psql -f dumpfile postgres +psql -X -f dumpfile postgres (Actually, you can specify any existing database name to start from, but if you are loading into an empty cluster then postgres diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index d66e901f51..8b572cdec0 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -1636,6 +1636,14 @@ CREATE DATABASE foo WITH TEMPLATE template0; option will be automatically enabled by the subscriber if the subscription had been originally created with two_phase = true option. + + + It is generally recommended to use the -X + (--no-psqlrc) option when restoring a database from a + pg_dump script to ensure a clean restore process + and prevent potential conflicts with existing psql + configurations. + @@ -1653,7 +1661,7 @@ CREATE DATABASE foo WITH TEMPLATE template0; newdb: -$ psql -d newdb -f db.sql +$ psql -X -d newdb -f db.sql diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml index 014f279258..bfbe305b08 100644 --- a/doc/src/sgml/ref/pg_dumpall.sgml +++ b/doc/src/sgml/ref/pg_dumpall.sgml @@ -817,6 +817,16 @@ exclude database PATTERN database creation will fail for databases in non-default locations. + + + It is
Re: Set AUTOCOMMIT to on in script output by pg_dump
On 2025-01-26 02:45, Tom Lane wrote: Robert Treat writes: On Wed, Jan 22, 2025 at 8:02 AM Shinya Kato wrote: I agree to it and fixed the patch. LGTM LGTM too. Pushed with a couple of very minor tweaks. regards, tom lane Thank you for pushing! -- Regards, Shinya Kato NTT DATA GROUP CORPORATION
Re: [PATCH] New predefined role pg_manage_extensions
On Thu, Jan 16, 2025 at 3:31 PM Michael Banck wrote: I agree with this idea. I think it is natural to delegate a part of superuser privileges to another role because superuser privilege is too strong. > > In general, this concept is rather dubious. Why should we have such a > > dangerous pre-defined role? > > Well, I would say pg_execute_server_program could be regarded as a > precedent. Exactly. pg_execute_server_program can escalate to superuser privileges, so pg_manage_extensions is not the only dangerous pre-defined role. > I do think having a whitelist of allowed-to-be-installed extensions > (similar/like https://github.com/dimitri/pgextwlist) makes sense > additionally in today's container/cloud word where the local Postgres > admin might not have control over which packages get installed but wants > to have control over which extension the application admins (or whoever) > may create, but that is another topic I think. To use a certain extension, you may need to install the postgresql-contrib package. In that case, is there a way to restrict extensions other than the required one? Or is it unnecessary to impose such restrictions? Regards, Shinya Kato
Add log_autovacuum_{vacuum|analyze}_min_duration
Hi hackers,
I am proposing the introduction of two new GUC parameters,
log_autovacuum_{vacuum|analyze}_min_duration, to replace the existing
log_autovacuum_min_duration.
Motivation:
Currently, log_autovacuum_min_duration controls the logging threshold
for both autovacuum and autoanalyze activities. However, autoanalyze
operations typically have a much shorter duration than autovacuum
operations. Consequently, if log_autovacuum_min_duration is set to a
value appropriate for capturing longer-running autovacuum tasks,
shorter autoanalyze activities are often not logged. This can make it
difficult to monitor and troubleshoot autoanalyze behavior
effectively.
By providing separate GUCs, administrators can set distinct logging
thresholds for autovacuum and autoanalyze, ensuring that both types of
activities can be logged appropriately based on their typical
execution times.
Example Usage:
The following SQL demonstrates how these new parameters would allow
for more granular logging. In this example, autoanalyze is configured
to always log (duration set to 0), while autovacuum is set to log only
if it runs for a very long time.
```
=# CREATE TABLE t (i int, d text) WITH (
-- autoanalyze settings
autovacuum_analyze_threshold = 1,
autovacuum_analyze_scale_factor = 0,
log_autovacuum_analyze_min_duration = 0,
-- autovacuum settings
autovacuum_vacuum_threshold = 1,
autovacuum_vacuum_scale_factor = 0,
log_autovacuum_vacuum_min_duration = 100_000_000
);
=# INSERT INTO t VALUES (1, 'a');
=# DELETE FROM t WHERE i = 1;
2025-06-03 15:15:39.608 JST [401368] LOG: automatic analyze of table
"postgres.public.t"
avg read rate: 18.229 MB/s, avg write rate: 0.000 MB/s
buffer usage: 155 hits, 7 reads, 0 dirtied
WAL usage: 1 records, 0 full page images, 530 bytes, 0 buffers full
system usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
```
Two patches are attached.
In v1-0001-Rename-log_autovacuum_min_duration.patch, just renamed
log_autovacuum_min_duration to log_autovacuum_vacuum_min_duration.
In v1-0002-Add-log_autovacuum_analyze_min_duration.patch, add the new
GUC parameter log_autovacuum_analyze_min_duration.
Do you think?
Best regards,
Shinya Kato
NTT OSS Center
v1-0002-Add-log_autovacuum_analyze_min_duration.patch
Description: Binary data
v1-0001-Rename-log_autovacuum_min_duration.patch
Description: Binary data
Re: Add log_autovacuum_{vacuum|analyze}_min_duration
Thank you for the comment!
On Tue, Jun 3, 2025 at 4:42 PM Michael Banck wrote:
>
> Hi,
>
> On Tue, Jun 03, 2025 at 03:35:20PM +0900, Shinya Kato wrote:
> > I am proposing the introduction of two new GUC parameters,
> > log_autovacuum_{vacuum|analyze}_min_duration, to replace the existing
> > log_autovacuum_min_duration.
>
> How about adding log_autoanalyze_min_duration instead? That would still
> slightly retcon the log_autovacuum_min_duration meaning/semantics by no
> longer logging autoanalyze unless the new GUC is set, but at least not
> rename the GUC and make both shorter while still being comprehensible
> IMO. Not sure what others think?
I surely think adding log_autoanalyze_min_duration is simpler and
shorter, but the reason I chose this GUC name is for consistency with
other autovacuum parameters. Existing autovacuum parameters that have
separate settings for vacuum and analyze operations follow the pattern
autovacuum_{vacuum|analyze}_*.
https://www.postgresql.org/docs/devel/runtime-config-vacuum.html#RUNTIME-CONFIG-AUTOVACUUM
Shinya Kato
NTT OSS Center
Re: Add log_autovacuum_{vacuum|analyze}_min_duration
On Wed, Jun 11, 2025 at 1:49 PM Shinya Kato wrote: > On Thu, Jun 5, 2025 at 3:53 AM Sami Imseih wrote: > >> > Approach 2: >> > - log_autovacuum_min_duration: Changed behavior, which controls only >> > autovacuum logging. >> > - log_autoanalyze_min_duration: New parameter, which controls >> > autoanalyze logging. >> >> My vote is for this approach. It is probably OK to change the behavior of >> log_autovacuum_min_duration, as the new GUC will have the same default >> value. >> > > Thank you for voting. I also think this approach is reasonable to > implement. > A new patch is attached. Thoughts? -- Best regards, Shinya Kato NTT OSS Center v2-0001-Add-log_autoanalyze_min_duration.patch Description: Binary data
Re: Extend COPY FROM with HEADER to skip multiple lines
> > However, a similar proposal was made earlier [1], and seemingly > > some hackers weren't in favor of it. It's probably worth reading > > that thread to understand the previous concerns. > > > > [1] > > https://postgr.es/m/calay4q8ngsxp0p5uf56vn-md7rewqzp5k6ps1cgum26x4fs...@mail.gmail.com > > Oh, I missed it. I will check it soon. I read it. There are clear differences from the earlier proposal. My sole motivation is to skip multiple headers, and I don't believe a feature for skipping footers is necessary. To be clear, I think it's best to simply extend the current HEADER option. Regarding the concern about adding ETL-like functionality, this feature is already implemented in other RDBMSs, which is why I believe it is also necessary for PostgreSQL. Honestly, I haven't implemented it yet, so I'm not sure about the performance. However, I don't expect it to be significantly different from the current HEADER option that skips a single line. > I think the earlier proposal went rather further than this one, which I > suspect can be implemented fairly cheaply. That's probably it, exactly. > I don't have terribly strong feelings about it, but matching a feature > implemented elsewhere has some attraction if it can be done easily. > > OTOH I'm a bit curious to know what software produces multi-line CSV > headers. Both Pandas and R can create CSV files with multi-line headers (although I don't personally think this is desirable). Furthermore, various systems sometimes generate reports as CSV files that unexpectedly contain multiple header lines. -- Best regards, Shinya Kato NTT OSS Center
Re: Extend COPY FROM with HEADER to skip multiple lines
On Tue, Jun 10, 2025 at 2:34 PM Fujii Masao wrote: > > There are clear differences from the earlier proposal. My sole > > motivation is to skip multiple headers, and I don't believe a feature > > for skipping footers is necessary. To be clear, I think it's best to > > simply extend the current HEADER option. > > Sounds ok to me. Thank you. > > Regarding the concern about adding ETL-like functionality, this > > feature is already implemented in other RDBMSs, which is why I believe > > it is also necessary for PostgreSQL. > > > > Honestly, I haven't implemented it yet, so I'm not sure about the > > performance. However, I don't expect it to be significantly different > > from the current HEADER option that skips a single line. > > So it seems better for you to implement the patch at first and then > check the performance overhead etc if necessary. Thank you for your advice. I will create a patch. -- Best regards, Shinya Kato NTT OSS Center On Tue, Jun 10, 2025 at 2:34 PM Fujii Masao wrote: > > > > On 2025/06/10 9:43, Shinya Kato wrote: > >>> However, a similar proposal was made earlier [1], and seemingly > >>> some hackers weren't in favor of it. It's probably worth reading > >>> that thread to understand the previous concerns. > >>> > >>> [1] > >>> https://postgr.es/m/calay4q8ngsxp0p5uf56vn-md7rewqzp5k6ps1cgum26x4fs...@mail.gmail.com > >> > >> Oh, I missed it. I will check it soon. > > > > I read it. > > > > There are clear differences from the earlier proposal. My sole > > motivation is to skip multiple headers, and I don't believe a feature > > for skipping footers is necessary. To be clear, I think it's best to > > simply extend the current HEADER option. > > Sounds ok to me. > > > > Regarding the concern about adding ETL-like functionality, this > > feature is already implemented in other RDBMSs, which is why I believe > > it is also necessary for PostgreSQL. > > > > Honestly, I haven't implemented it yet, so I'm not sure about the > > performance. However, I don't expect it to be significantly different > > from the current HEADER option that skips a single line. > > So it seems better for you to implement the patch at first and then > check the performance overhead etc if necessary. > > Regards, > > -- > Fujii Masao > NTT DATA Japan Corporation > -- Best regards, Shinya Kato NTT OSS Center
Re: Extend COPY FROM with HEADER to skip multiple lines
> > So it seems better for you to implement the patch at first and then > > check the performance overhead etc if necessary. > > Thank you for your advice. I will create a patch. I created a patch. As you can see from the patch, I believe the performance impact is negligible. The only changes were to modify how the HEADER option is stored and to add a loop that skips the specified number of header lines when parsing the options. The design is such that if a HEADER value larger than the number of lines in the file is specified, the command will complete with zero rows loaded and will not return an error. This is the same behavior as specifying HEADER true for a CSV file that has zero rows. And I will add this patch for the next CF. Thoughts? -- Best regards, Shinya Kato NTT OSS Center v1-0001-Add-support-for-multi-line-header-skipping-in-COP.patch Description: Binary data
Re: Extend COPY FROM with HEADER to skip multiple lines
On Thu, Jun 26, 2025 at 4:36 PM Fujii Masao
wrote:
> On 2025/06/26 14:35, Shinya Kato wrote:
> >
> > > > So it seems better for you to implement the patch at first and then
> > > > check the performance overhead etc if necessary.
> > >
> > > Thank you for your advice. I will create a patch.
> >
> > I created a patch.
>
> Thanks for the patch!
>
Thank you for your review.
When I compiled with the patch applied, I got the following warning:
>
> copyfromparse.c:834:20: error: ‘done’ may be used uninitialized
> [-Werror=maybe-uninitialized]
>834 | if (done)
>|^
>
Oh, sorry. I missed it.
I fixed it to initialize done = false.
> + lines are discarded. An integer value greater than 1 is only valid
> for
> + COPY FROM commands.
>
>
> This might be slightly misleading since 0 is also a valid value for COPY
> FROM.
>
That's certainly true. I fixed it below:
+ lines are discarded. An integer value greater than 1 is not allowed
for
+ COPY TO commands.
> + for (int i = 0; i < cstate->opts.header.skip_lines; i++)
> + {
> + cstate->cur_lineno++;
> + done = CopyReadLine(cstate, is_csv);
> + }
>
> If "done" is true, shouldn't we break out of the loop immediately?
> Otherwise, with a large HEADER value, we could end up wasting a lot of
> cycles unnecessarily.
Exactly, fixed.
> +defGetCopyHeaderOption(DefElem *def, bool is_from)
> {
> + CopyHeaderOption option;
>
> To avoid repeating the same code like "option.match = false" in every case,
> how about initializing the struct like "option = {false, 1}"?
>
Exactly, fixed.
>
>
> +
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +errmsg("HEADER
> must be non-negative integer")));
>
> This message could be confusing since HEADER can also accept a Boolean or
> "match".
> Maybe it's better to use the same message as "%s requires a Boolean value,
> integer value,
> or \"match\"",? "integer value"? If so, it's also better to use
> "non-negative integer"
> instead of just "integer".
>
Yes, I fixed it to use the same message which replaced "integer" to
"non-negative integer".
Original error message "%s requires a Boolean value, integer value, or
\"match\"" should also be fixed to "non-negative integer"?
--
Best regards,
Shinya Kato
NTT OSS Center
v2-0001-Add-support-for-multi-line-header-skipping-in-COP.patch
Description: Binary data
Re: Extend COPY FROM with HEADER to skip multiple lines
On Fri, Jun 27, 2025 at 12:03 AM Fujii Masao
wrote:
>
>
> On 2025/06/26 19:12, Shinya Kato wrote:
> > On Thu, Jun 26, 2025 at 4:36 PM Fujii Masao <mailto:masao.fu...@oss.nttdata.com>> wrote:
> >
> > On 2025/06/26 14:35, Shinya Kato wrote:
> > >
> > > > > So it seems better for you to implement the patch at first
> and then
> > > > > check the performance overhead etc if necessary.
> > > >
> > > > Thank you for your advice. I will create a patch.
> > >
> > > I created a patch.
> >
> > Thanks for the patch!
> >
> >
> > Thank you for your review.
> >
> > When I compiled with the patch applied, I got the following warning:
> >
> > copyfromparse.c:834:20: error: ‘done’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> > 834 | if (done)
> > |^
> >
> >
> > Oh, sorry. I missed it.
> > I fixed it to initialize done = false.
> >
> > + lines are discarded. An integer value greater than 1 is only
> valid for
> > + COPY FROM commands.
> >
> >
> > This might be slightly misleading since 0 is also a valid value for
> COPY FROM.
> >
> > That's certainly true. I fixed it below:
> > + lines are discarded. An integer value greater than 1 is not
> allowed for
> > + COPY TO commands.
>
> Thanks for fixing that! However, it's odd that the description for COPY TO
> appears
> under the section starting with "On input." Shouldn't it be under the "On
> output"
> section instead?
>
> Also, I think the entire HEADER option section could use a clearer
> structure.
> How about revising it like this?
>
> -
>
>
> - Specifies that the file contains a header line with the names of
> each
> - column in the file. On output, the first line contains the column
> - names from the table. On input, the first line is discarded when
> this
> - option is set to true (or equivalent Boolean
> value).
> - If this option is set to MATCH, the number and
> names
> - of the columns in the header line must match the actual column
> names of
> - the table, in order; otherwise an error is raised.
> + On output, if this option is set to true
> + (or an equivalent Boolean value), the first line of the output will
> + contain the column names from the table.
> + Integer values 0 and 1 are
> + accepted as Boolean values, but other integers are not allowed for
> + COPY TO commands.
> +
> +
> + On input, if this option is set to true
> + (or an equivalent Boolean value), the first line of the input is
> + discarded. If it is set to a non-negative integer, that number of
> + lines are discarded. If the option is set to
> MATCH,
> + the number and names of the columns in the header line must exactly
> + match those of the table, in order; otherwise an error is raised.
> + The MATCH value is only valid for
> + COPY FROM commands.
> +
> +
> This option is not allowed when using binary
> format.
> - The MATCH option is only valid for COPY
> - FROM commands.
>
>
> -
>
Yes, your documentation is clearer than mine.
>
> Also, similar to the existing "boolean" type explanation in the COPY docs,
> we should add a corresponding entry for integer, now that it's accepted by
> the HEADER option. The VACUUM docs has a good example of how to phrase
> this.
>
Exactly.
> > Original error message "%s requires a Boolean value, integer value, or
> \"match\"" should also be fixed to "non-negative integer"?
>
> Yes, I think that the message
>
> "%s requires a Boolean value, a non-negative integer, or the string
> \"match\""
>
> is clearer and more accurate.
Thank you, you're right.
> -typedef enum CopyHeaderChoice
> +typedef struct CopyHeaderOption
> {
> - COPY_HEADER_FALSE = 0,
> - COPY_HEADER_TRUE,
> - COPY_HEADER_MATCH,
> -} CopyHeaderChoice;
> + boolmatch; /* header line must match actual names? */
> + int skip_lines; /* number of lines to skip before
> data */
> +} CopyHeaderOption;
>
> - CopyHeaderChoice header_line; /* header line? */
> + CopyHeaderOption header;/* header line? */
>
Re: Partitioned tables and [un]loggedness
On Wed, Jun 4, 2025 at 6:42 PM Michael Paquier wrote: > > On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote: > > I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is > > the remaining piece. > > And the second piece is now applied as of e2bab2d79204. > -- > Michael Hi, Should we consider preventing tab completion for PARTITION BY immediately after CREATE TABLE name (...)? Or is it fine to leave it as is, given that it's syntactically correct? -- Best regards, Shinya Kato NTT OSS Center
Re: Partitioned tables and [un]loggedness
On Wed, Jun 4, 2025 at 6:55 PM Shinya Kato wrote: > > On Wed, Jun 4, 2025 at 6:42 PM Michael Paquier wrote: > > > > On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote: > > > I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is > > > the remaining piece. > > > > And the second piece is now applied as of e2bab2d79204. > > -- > > Michael > > Hi, > > Should we consider preventing tab completion for PARTITION BY > immediately after CREATE TABLE name (...)? Or is it fine to leave it > as is, given that it's syntactically correct? Sorry. CREATE TABLE name (...) -> CREATE UNLOGGED TABLE name (...) -- Best regards, Shinya Kato NTT OSS Center
Re: Add log_autovacuum_{vacuum|analyze}_min_duration
Thank you all for the comments!
On Wed, Jun 4, 2025 at 10:30 AM wenhui qiu wrote:
>
> HI
> I vote log_autovacuum_{vacuum|analyze}_min_duration. Then don't remove
> log_autovacuum_min_duration so easily!
>
> On Wed, Jun 4, 2025 at 7:16 AM Fujii Masao
> wrote:
>>
>>
>>
>> On 2025/06/04 4:32, Sami Imseih wrote:
>> >> On Tue, Jun 03, 2025 at 10:57:11AM +0200, Michael Banck wrote:
>> >>> On Tue, Jun 03, 2025 at 05:25:40PM +0900, Shinya Kato wrote:
>> >>>> I surely think adding log_autoanalyze_min_duration is simpler and
>> >>>> shorter, but the reason I chose this GUC name is for consistency with
>> >>>> other autovacuum parameters. Existing autovacuum parameters that have
>> >>>> separate settings for vacuum and analyze operations follow the pattern
>> >>>> autovacuum_{vacuum|analyze}_*.
>> >>>> https://www.postgresql.org/docs/devel/runtime-config-vacuum.html#RUNTIME-CONFIG-AUTOVACUUM
>> >>>
>> >>> Right, but the GUCs that directly affect either vacuum or autovacuum
>> >>> behaviour need the qualification (and then vacuum/analyze on top of it).
>> >>> I think we have less constraints with the logging GUC and do not need to
>> >>> mirror the behaviorial GUCs at all costs. But again, that is just my two
>> >>> cents.
>> >>
>> >> I lean towards log_autovacuum_{vacuum|analyze}_min_duration. If
>> >> log_autovacuum_min_duration didn't exist, that's probably the naming
>> >> scheme
>> >> we'd go with. However, I'm not sure we can get away with renaming
>> >> log_autovacuum_min_duration. Presumably we'd need to at least keep it
>> >> around as a backward-compatibility GUC, and its behavior would probably
>> >> change, too
>> >
>> > I think deprecating a GUC like log_autovacuum_min_duration would be quite
>> > difficult.
>>
>> Also deprecating the log_autovacuum_min_duration reloption might be tricky.
>> If we remove support for it in v19, how should pg_dump handle tables with
>> this option set from older versions? Should it translate it into both
>> log_autovacuum_vacuum_min_duration and log_autovacuum_analyze_min_duration
>> during dump? Would pg_upgrade run into the same issue?
>>
I understand it's hard to deprecate log_autovacuum_min_duration. I
think there are three approaches that reflect your comments.
Approach 1:
- log_autovacuum_min_duration: Same behavior, which controls
autovacuum and autoanalyze logging.
- log_autoanalyze_min_duration: New parameter, which controls
autoanalyze logging.
Approach 2:
- log_autovacuum_min_duration: Changed behavior, which controls only
autovacuum logging.
- log_autoanalyze_min_duration: New parameter, which controls
autoanalyze logging.
Approach 3:
- log_autovacuum_min_duration: Retained for backward compatibility.
- log_autovacuum_{vacuum,analyze}_min_duration: New parameter.
Thoughts?
And I added a new entry for the next commitfest.
https://commitfest.postgresql.org/patch/5797/
--
Best regards,
Shinya Kato
NTT OSS Center
Re: Partitioned tables and [un]loggedness
On Thu, Jun 5, 2025 at 9:23 AM Michael Paquier wrote: > Agreed to not suggest the PARTITION BY clause in the tab completion as > it is not supported by the backend for unlogged tables. > tab-complete.in.c has some handling for CREATE UNLOGGED TABLE around > line 3667, so we could just have an extra case for it, like in the > attached patch. A split already exists for temporary tables to handle > the ON COMMIT clause after the attribute list. > > Thoughts? Thank you. It looks good to me. -- Best regards, Shinya Kato NTT OSS Center
Re: Extend COPY FROM with HEADER to skip multiple lines
On Tue, Jun 10, 2025 at 7:05 PM Dagfinn Ilmari Mannsåker wrote: > Andrew Dunstan writes: > > > OTOH I'm a bit curious to know what software produces multi-line CSV > > headers. > > AWS CloudFront access logs are stored in S3 as TSV files (one per hour > per CF node) with a two-line header comment where the first line is the > version and the second lists the fields (but not in a form useful for > HEADER MATCH). > Thank you for providing that example. > Although not useful for the above format, and not intended to derail or > bloat the proposal in this thread, would it be useful to have a mode > that combines skip and match? I.e. skip N lines, then check the fields > in the one after that against the target columns. > I think it would be useful, but the target columns are not always at the bottom of the header. For example, the target columns could be on the first line, with explanations or sub-columns on the lines that follow. Considering this, the patch would become too complicated, so I'd like to keep this out of scope. What do you think? -- Best regards, Shinya Kato NTT OSS Center
Re: Add log_autovacuum_{vacuum|analyze}_min_duration
On Thu, Jun 5, 2025 at 3:53 AM Sami Imseih wrote: > > Approach 2: > > - log_autovacuum_min_duration: Changed behavior, which controls only > > autovacuum logging. > > - log_autoanalyze_min_duration: New parameter, which controls > > autoanalyze logging. > > My vote is for this approach. It is probably OK to change the behavior of > log_autovacuum_min_duration, as the new GUC will have the same default > value. > Thank you for voting. I also think this approach is reasonable to implement. log_autoanalyze_min_duration makes sense, especially since > "autoanalyze" is the term we already use in system views (e.g., > pg_stat_all_tables.last_autoanalyze). I do not think we need to worry > about consistency with other autovacuum parameters (e.g., > autovacuum_[vacuum|analyze]_threshold, etc.), because in this case we are > only talking about logging, so we have more flexibility in naming. > +1. > Initially, I was not sure if there is a use case in which someone would > want > to turn off autovacuum logging but keep autoanalyze logging (or vice > versa), > but there may be, and this will be more flexible. > My concern is less about turning autovacuum and autoanalyze logs on or off individually, and more about the fact that setting a large value for log_autovacuum_min_duration prevents autoanalyze logs from being recorded. -- Best regards, Shinya Kato NTT OSS Center
Extend COPY FROM with HEADER to skip multiple lines
Hi hackers, I'd like to propose a new feature for the COPY FROM command to allow skipping multiple header lines when loading data. This enhancement would enable files with multi-line headers to be loaded without any preprocessing, which would significantly improve usability. In real-world scenarios, it's common for data files to contain multiple header lines, such as file descriptions or column explanations. Currently, the COPY command cannot load these files directly, which requires users to preprocess them with tools like sed or tail. Although you can use "COPY t FROM PROGRAM 'tail -n +3 /path/to/file'", some environments do not have the tail command available. Additionally, this approach requires superuser privileges or membership in the pg_execute_server_program role. This feature also has precedent in other major RDBMS: - MySQL: LOAD DATA ... IGNORE N LINES [1] - SQL Server: BULK INSERT … WITH (FIRST ROW=N) [2] - Oracle SQL*Loader: sqlldr … SKIP=N [3] I have not yet created a patch, but I am willing to implement an extension for the HEADER option. I would like to discuss the specification first. The specification I have in mind is as follows: - Command: COPY FROM - Formats: text and csv - Option syntax: HEADER [ boolean | integer | MATCH] (Extend the HEADER option to accept an integer value in addition to the existing boolean and MATCH keywords.) - Behavior: Let N be the specified integer. - If N < 0, raise an error. - If N = 0 or 1, same behavior when boolean is specified. - If N > 1, skip the first N rows. Thoughts? [1] https://dev.mysql.com/doc/refman/8.4/en/load-data.html#load-data-field-line-handling [2] https://learn.microsoft.com/en-us/sql/t-sql/statements/bulk-insert-transact-sql?view=sql-server-ver17#firstrow--first_row [3] https://docs.oracle.com/en/database/oracle/oracle-database/23/sutil/oracle-sql-loader-commands.html#SUTIL-GUID-84244C46-6AFD-412D-9240-BEB0B5C2718B -- Best regards, Shinya Kato NTT OSS Center
Re: Extend COPY FROM with HEADER to skip multiple lines
2025年6月9日(月) 17:27 Fujii Masao : > I generally like the idea. Thanks. > However, a similar proposal was made earlier [1], and seemingly > some hackers weren't in favor of it. It's probably worth reading > that thread to understand the previous concerns. > > [1] > https://postgr.es/m/calay4q8ngsxp0p5uf56vn-md7rewqzp5k6ps1cgum26x4fs...@mail.gmail.com Oh, I missed it. I will check it soon. -- Best regards, Shinya Kato NTT OSS Center
Re: vacuumlazy: Modernize count_nondeletable_pages
Hi!
On Fri, Jun 27, 2025 at 8:34 PM Matthias van de Meent
wrote:
>
> Hi,
>
> Recently I had to debug an issue with VACUUM's truncate stage taking
> an AE lock for a long time, which caused an unpleasant outage due to
> blocked queries on a replica. That, and remembering a thread about
> this over at [0] got me looking at the code that truncates the
> relation.
>
> After looking at the code, I noticed that it has hand-rolled
> prefetching of pages, and in a rather peculiar way. Now that we have
> the read_stream.c API it is much more efficient to make use that
> system, if only to combine IOs and be able to use async IO handling.
>
> While making that change, I also noticed that the current coding does
> not guarantee progress when the relation's AE-lock is heavily
> contended and the process takes long enough to get started:
> When count_nondeletable_pages exits due to lock contention, then
> blockno%32==0. In that case the next iteration will start with that
> same blockno, and this may cause the scan to make no progress at all
> if the time from INSTR_TIME_SET_CURRENT(starttime) to the first
> INSTR_TIME_SET_CURRENT(currenttime) is >20ms; while unlikely this does
> seem possible on a system with high load.
With the previous logic (blockno % 32 == 0), I believe a process
waiting for a lock had to wait for an average of 16 pages to be
processed. With this change, it will now have to wait for 32 pages,
correct? I am not sure if this change is reasonable.
Additionally, if blockno % 32 == 0 in the next loop and there are
still processes waiting for a lock, it seems appropriate to prioritize
those waiting processes and skip the VACUUM TRUNCATE.
> Attached is a patch which improves the status quo for 1, and fixes
> item 2 by increasing the minimum number of pages processed before
> releasing the lock to 31.
Thank you for the patch!
+typedef struct CountNonDeletablePagesScan
+{
+ BlockNumber current_block;
+ BlockNumber target_block;
+} CountNonDeletablePagesScan;
I think it's better to declare structs at the top of the file. What do
you think?
I'm still not sure how to use read_stream, so I don't have any
comments on read_stream at this time.
--
Best regards,
Shinya Kato
NTT OSS Center
Re: Extend COPY FROM with HEADER to skip multiple lines
On Wed, Jul 2, 2025 at 4:48 PM Fujii Masao wrote: > >> Regarding the documentation, how about explicitly stating that when MATCH > >> is specified, only > >> the first line is skipped? While this may seem obvious, it’s worth > >> clarifying, as the semantics > >> of the HEADER option have become a bit more complex with this change. > > > > Agreed. I have updated the documentation as follows: > > > > + lines are discarded. If the option is set to > > MATCH, > > + the number and names of the columns in the header line must exactly > > + match those of the table and, in order, after which the header line > > is > > + discarded; otherwise an error is raised. The > > MATCH > > How about making the wording a bit clearer? For example: > > If set to MATCH, the first line is discarded, and it must contain column > names that > exactly match the table's columns, in both number and order; otherwise, > an error is raised. Thank you for the review. I fixed it. > > Also, the phrase "if this option is set to..." is repeated three times in the > current text. > For the second and third instances, we could simplify it to just "if set > to...". Agreed. However, for the sake of symmetry between "On output" and "On input" and to maintain clarity between the paragraphs, I have omitted "this option is" from the "On input" paragraph only. -- Best regards, Shinya Kato NTT OSS Center v5-0001-Add-support-for-multi-line-header-skipping-in-COP.patch Description: Binary data
Re: Extend COPY FROM with HEADER to skip multiple lines
On Thu, Jul 3, 2025 at 3:32 PM Fujii Masao wrote: > > > > On 2025/07/03 11:08, Shinya Kato wrote: > > On Wed, Jul 2, 2025 at 4:48 PM Fujii Masao > > wrote: > > > >>>> Regarding the documentation, how about explicitly stating that when > >>>> MATCH is specified, only > >>>> the first line is skipped? While this may seem obvious, it’s worth > >>>> clarifying, as the semantics > >>>> of the HEADER option have become a bit more complex with this change. > >>> > >>> Agreed. I have updated the documentation as follows: > >>> > >>> + lines are discarded. If the option is set to > >>> MATCH, > >>> + the number and names of the columns in the header line must exactly > >>> + match those of the table and, in order, after which the header > >>> line is > >>> + discarded; otherwise an error is raised. The > >>> MATCH > >> > >> How about making the wording a bit clearer? For example: > >> > >> If set to MATCH, the first line is discarded, and it must contain > >> column names that > >> exactly match the table's columns, in both number and order; > >> otherwise, an error is raised. > > > > Thank you for the review. I fixed it. > > Thanks for updating the patch! I've pushed the patch. > > > >> Also, the phrase "if this option is set to..." is repeated three times in > >> the current text. > >> For the second and third instances, we could simplify it to just "if set > >> to...". > > > > Agreed. However, for the sake of symmetry between "On output" and "On > > input" and to maintain clarity between the paragraphs, I have omitted > > "this option is" from the "On input" paragraph only. > > Yes, I agree that's better. > > Regards, > > -- > Fujii Masao > NTT DATA Japan Corporation > Thank you for pushing! -- Best regards, Shinya Kato NTT OSS Center
Re: Add log_autovacuum_{vacuum|analyze}_min_duration
On Mon, Jun 23, 2025 at 4:24 PM Shinya Kato wrote: > > On Wed, Jun 11, 2025 at 1:49 PM Shinya Kato wrote: >> >> On Thu, Jun 5, 2025 at 3:53 AM Sami Imseih wrote: >>> >>> > Approach 2: >>> > - log_autovacuum_min_duration: Changed behavior, which controls only >>> > autovacuum logging. >>> > - log_autoanalyze_min_duration: New parameter, which controls >>> > autoanalyze logging. >>> >>> My vote is for this approach. It is probably OK to change the behavior of >>> log_autovacuum_min_duration, as the new GUC will have the same default >>> value. >> >> >> Thank you for voting. I also think this approach is reasonable to implement. > > > A new patch is attached. > Thoughts? Rebased. -- Best regards, Shinya Kato NTT OSS Center v3-0001-Add-log_autoanalyze_min_duration.patch Description: Binary data
Re: Extend COPY FROM with HEADER to skip multiple lines
On Mon, Jun 30, 2025 at 4:24 PM Yugo Nagata wrote:
>
>
> > > I have a few minor comment on the patch.
> > >
> > > + if (ival < 0)
> > > + ereport(ERROR,
> > > +
> > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > +errmsg("%s requires
> > > a Boolean value, a non-negative "
> > > +
> > > "integer, or the string \"match\"",
> > > +
> > > def->defname)));
> > >
> > > ereport(ERROR,
> > > (errcode(ERRCODE_SYNTAX_ERROR),
> > > -errmsg("%s requires a Boolean value or \"match\"",
> > > +errmsg("%s requires a Boolean value, a non-negative
> > > integer, "
> > > + "or the string \"match\"",
> > > def->defname)));
> > >
> > > These two pieces of code raise the same error, but with different error
> > > codes:
> > > one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns
> > > ERRCODE_SYNTAX_ERROR.
> > >
> > > I believe the former is more appropriate, although the existing code uses
> > > the
> > > latter. In any case, the error codes should be consistent.
> >
> > I'm not sure there's an actual rule like "the error code must match
> > if the error message is the same." But if using the same message with
> > a different error code is confusing, I'm fine with changing
> > the earlier message. For example:
> >
> >
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > - errmsg("%s requires
> > a Boolean value, a non-negative "
> > -
> > "integer, or the string \"match\"",
> > + errmsg("a negative
> > integer value cannot be "
> > +
> > "specified for %s",
> >
> > def->defname)));
>
> Fair point. There might not be any explicit rule.
> However, I feel it somewhat confusing.
>
> After looking into the code, it seems that ERRCODE_SYNTAX_ERROR tends to be
> used
> for invalid keywords or parameter types, while ERRCODE_INVALID_PARAMETER_VALUE
> is typically used for values that are out of the acceptable range.
>
> So, the proposed change seems reasonable to me.
Thank you for the review. The change looks good to me, too. A new
patch is attached.
> Regarding the documentation, how about explicitly stating that when MATCH is
> specified, only
> the first line is skipped? While this may seem obvious, it’s worth
> clarifying, as the semantics
> of the HEADER option have become a bit more complex with this change.
Agreed. I have updated the documentation as follows:
+ lines are discarded. If the option is set to MATCH,
+ the number and names of the columns in the header line must exactly
+ match those of the table and, in order, after which the header line is
+ discarded; otherwise an error is raised. The MATCH
--
Best regards,
Shinya Kato
NTT OSS Center
v4-0001-Add-support-for-multi-line-header-skipping-in-COP.patch
Description: Binary data
Add backup_type to pg_stat_progress_basebackup
Hi hackers, Starting with PostgreSQL 17, pg_basebackup supports incremental backups. However, the pg_stat_progress_basebackup view doesn't currently show the backup type (i.e., whether it's a full or incremental backup). Therefore, I propose adding a backup_type column to this view. While this information is available in pg_stat_activity, the backup type is important for monitoring the progress of pg_basebackup, and including it directly in the progress view would be very useful. Thoughts? -- Best regards, Shinya Kato NTT OSS Center v1-0001-Add-backup_type-to-pg_stat_progress_basebackup.patch Description: Binary data
Re: Add backup_type to pg_stat_progress_basebackup
On Tue, Jul 22, 2025 at 6:06 PM Yugo Nagata wrote: > > On Tue, 22 Jul 2025 17:48:35 +0900 > Shinya Kato wrote: > > > Hi hackers, > > > > Starting with PostgreSQL 17, pg_basebackup supports incremental > > backups. However, the pg_stat_progress_basebackup view doesn't > > currently show the backup type (i.e., whether it's a full or > > incremental backup). > > > > Therefore, I propose adding a backup_type column to this view. While > > this information is available in pg_stat_activity, the backup type is > > important for monitoring the progress of pg_basebackup, and including > > it directly in the progress view would be very useful. > > > > Thoughts? > > That seems reasonable to me. > > Just one minor comment on the patch: > > + > + > + > + backup_type bigint > + > + > +Backup type. Either full or > +incremental. > + > + > > The type should be text rather than bigint. Thank you for the review. I made a careless mistake. Fixed. -- Best regards, Shinya Kato NTT OSS Center v2-0001-Add-backup_type-to-pg_stat_progress_basebackup.patch Description: Binary data
Re: Add backup_type to pg_stat_progress_basebackup
On Sat, Aug 2, 2025 at 8:12 AM Masahiko Sawada wrote: > > On Tue, Jul 22, 2025 at 2:42 AM Shinya Kato wrote: > > > > On Tue, Jul 22, 2025 at 6:06 PM Yugo Nagata wrote: > > > > > > On Tue, 22 Jul 2025 17:48:35 +0900 > > > Shinya Kato wrote: > > > > > > > Hi hackers, > > > > > > > > Starting with PostgreSQL 17, pg_basebackup supports incremental > > > > backups. However, the pg_stat_progress_basebackup view doesn't > > > > currently show the backup type (i.e., whether it's a full or > > > > incremental backup). > > > > > > > > Therefore, I propose adding a backup_type column to this view. While > > > > this information is available in pg_stat_activity, the backup type is > > > > important for monitoring the progress of pg_basebackup, and including > > > > it directly in the progress view would be very useful. > > > > > > > > Thoughts? > > > > > > That seems reasonable to me. > > I like this idea. Thank you for reviewing my patch! > > Thank you for the review. > > I made a careless mistake. Fixed. > > The patch seems reasonably simple and looks good to me. I've updated > the comment in bbsink_progress_new() and attached the modified version > patch (with the commit message). Please review it. Thanks for the patch. I reviewed it, and LGTM. -- Best regards, Shinya Kato NTT OSS Center
