Re: Possibility to disable `ALTER SYSTEM`
At 2024-03-26 08:11:33 -0400, robertmh...@gmail.com wrote: > > On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian wrote: > > > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM? > > > > > > maybe "externally_managed_auto_config" > > > > How many people associate "auto" with ALTER SYSTEM? I assume not many. > > > > To me, externally_managed_configuration is promising a lot more than it > > delivers because there is still a lot of ocnfiguration it doesn't > > control. I am also confused why the purpose of the feature, external > > management of configuation, is part of the variable name. We usually > > name parameters for what they control. > > I actually agree with this. I wasn't going to quibble with it because > other people seemed to like it. But I think something like > allow_alter_system would be better, as it would describe the exact > thing that the parameter does, rather than how we think the parameter > ought to be used. Yes, "externally_managed_configuration" raises far more questions than it answers. "enable_alter_system" is clearer in terms of what to expect when you set it. "enable_alter_system_command" is rather long, but even better in that it is specific enough to not promise anything about not allowing superusers to change the configuration some other way. -- Abhijit (as someone who could find a use for this feature)
Re: [PATCH] Exponential backoff for auth_delay
At 2024-01-19 15:08:36 +0100, mba...@gmx.net wrote: > > I wonder whether maybe auth_delay.max_seconds should either be renamed > to auth_delay.exponential_backoff_max_seconds (but then it is rather > long) in order to make it clearer it only applies in that context or > alternatively just apply to auth_delay.milliseconds as well (though > that would be somewhat weird). I think it's OK as-is. The description/docs are pretty clear. > I wonder though whether this might be a useful message to have at some > more standard level like INFO? I don't have a strong opinion about this, but I suspect anyone who is annoyed enough by repeated authentication failures to use auth_delay will also be happy to have less noise in the logs about it. > You mean something like "after 5 minutes, reset the delay to 0 again"? > I agree that this would probably be useful, but would also make the > change more complex. Yes, that's the kind of thing I meant. I agree that it would make this patch more complex, and I don't think it's necessary to implement. However, since it's a feature that seems to go hand-in-hand with exponential backoff in general, it _may_ be good to mention in the docs that the sleep time for a host is reset only by successful authentication, not by any timeout. Not sure. > What I had in mind is that admins would lower auth_delay.milliseconds to > something like 100 or 125 when exponential_backoff is on Ah, that makes a lot of sense. Thanks for explaining. Your new v3 patch looks fine to me. I'm marking it as ready for committer. -- Abhijit
Re: Extend pgbench partitioning to pgbench_history
At 2023-11-30 11:29:15 +0100, gabriele.bartol...@enterprisedb.com wrote: > > With the attached patch, I extend the partitioning capability to the > pgbench_history table too. > > I have been thinking of adding an option to control this, but I preferred > to ask in this list whether it really makes sense or not (I struggle indeed > to see use cases where accounts is partitioned and history is not). I don't have a strong opinion about this, but I also can't think of a reason to want to create partitions for pgbench_accounts but leave out pgbench_history. > From ba8f507b126a9c5bd22dd40bb8ce0c1f0c43ac59 Mon Sep 17 00:00:00 2001 > From: Gabriele Bartolini > Date: Thu, 30 Nov 2023 11:02:39 +0100 > Subject: [PATCH] Include pgbench_history in partitioning method for pgbench > > In case partitioning, make sure that pgbench_history is also partitioned with > the same criteria. I think "If partitioning" or "If we're creating partitions" would read better here. Also, same criteria as what? Maybe you could just add "as pgbench_accounts" to the end. > diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml > index 05d3f81619..4c02d2a61d 100644 > --- a/doc/src/sgml/ref/pgbench.sgml > +++ b/doc/src/sgml/ref/pgbench.sgml > […] > @@ -378,9 +378,9 @@ pgbench options > d > > --partitions=NUM > > > -Create a partitioned pgbench_accounts table with > -NUM partitions of nearly equal size for > -the scaled number of accounts. > +Create partitioned pgbench_accounts and > pgbench_history > +tables with NUM partitions of nearly > equal size for > +the scaled number of accounts - and future history records. > Default is 0, meaning no partitioning. > I would just leave out the "-" and write "number of accounts and future history records". > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c > index 2e1650d0ad..87adaf4d8f 100644 > --- a/src/bin/pgbench/pgbench.c > +++ b/src/bin/pgbench/pgbench.c > […] > @@ -889,8 +889,10 @@ usage(void) > " --index-tablespace=TABLESPACE\n" > " create indexes in the specified > tablespace\n" > " --partition-method=(range|hash)\n" > -" partition pgbench_accounts with > this method (default: range)\n" > -" --partitions=NUM partition pgbench_accounts into > NUM parts (default: 0)\n" > +" partition pgbench_accounts and > pgbench_history with this method" > +" (default: range)." > +" --partitions=NUM partition pgbench_accounts and > pgbench_history into NUM parts" > +" (default: 0)\n" > " --tablespace=TABLESPACE create tables in the specified > tablespace\n" > " --unlogged-tablescreate tables as unlogged > tables\n" > "\nOptions to select what to run:\n" There's a missing newline after "(default: range).". I read through the rest of the patch closely. It looks fine to me. It applies, builds, and does create the partitions as intended. -- Abhijit
Re: [PATCH] Exponential backoff for auth_delay
At 2024-01-04 08:30:36 +0100, mba...@gmx.net wrote:
>
> +typedef struct AuthConnRecord
> +{
> + charremote_host[NI_MAXHOST];
> + boolused;
> + double sleep_time; /* in milliseconds */
> +} AuthConnRecord;
Do we really need a "used" field here? You could avoid it by setting
remote_host[0] = '\0' in cleanup_conn_record.
> static void
> auth_delay_checks(Port *port, int status)
> {
> + double delay;
I would just initialise this to auth_delay_milliseconds here, instead of
the harder-to-notice "else" below.
> @@ -43,8 +69,150 @@ auth_delay_checks(Port *port, int status)
>*/
> if (status != STATUS_OK)
> {
> - pg_usleep(1000L * auth_delay_milliseconds);
> + if (auth_delay_exp_backoff)
> + {
> + /*
> + * Exponential backoff per remote host.
> + */
> + delay = record_failed_conn_auth(port);
> + if (auth_delay_max_seconds > 0)
> + delay = Min(delay, 1000L *
> auth_delay_max_seconds);
> + }
I would make this comment more specific, maybe something like "Delay by
2^n seconds after each authentication failure from a particular host,
where n is the number of consecutive authentication failures".
It's slightly discordant to see the new delay being returned by a
function named "record_" (rather than "calculate_delay" or
similar). Maybe a name like "backoff_after_failed_auth" would be better?
Or "increase_delay_on_auth_fail"?
> +static double
> +record_failed_conn_auth(Port *port)
> +{
> + AuthConnRecord *acr = NULL;
> + int j = -1;
> +
> + acr = find_conn_record(port->remote_host, );
> +
> + if (!acr)
> + {
> + if (j == -1)
> +
> + /*
> + * No free space, MAX_CONN_RECORDS reached. Wait as
> long as the
> + * largest delay for any remote host.
> + */
> + return find_conn_max_delay();
In this extraordinary situation (where there are lots of hosts with lots
of authentication failures), why not delay by auth_delay_max_seconds
straightaway, especially when the default is only 10s? I don't see much
point in coordinating the delay between fifty known hosts and an unknown
number of others.
> + elog(DEBUG1, "new connection: %s, index: %d", acr->remote_host,
> j);
I think this should be removed, but if you want to leave it in, the
message should be more specific about what this is actually about, and
where the message is from, so as to not confuse debug-log readers.
(The same applies to the other debug messages.)
> +static AuthConnRecord *
> +find_conn_record(char *remote_host, int *free_index)
> +{
> + int i;
> +
> + *free_index = -1;
> + for (i = 0; i < MAX_CONN_RECORDS; i++)
> + {
> + if (!acr_array[i].used)
> + {
> + if (*free_index == -1)
> + /* record unused element */
> + *free_index = i;
> + continue;
> + }
> + if (strcmp(acr_array[i].remote_host, remote_host) == 0)
> + return _array[i];
> + }
> +
> + return NULL;
> +}
It's not a big deal, but to be honest, I would much prefer to (get rid
of used, as suggested earlier, and) have separate find_acr_for_host()
and find_free_acr() functions.
> +static void
> +record_conn_failure(AuthConnRecord *acr)
> +{
> + if (acr->sleep_time == 0)
> + acr->sleep_time = (double) auth_delay_milliseconds;
> + else
> + acr->sleep_time *= 2;
> +}
I would just roll this function into record_failed_conn_auth (or
whatever it's named), but if you want to keep it a separate function, it
should at least have a name that's sufficiently different from
record_failed_conn_auth.
In terms of the logic, it would have been slightly clearer to store the
number of failures and calculate the delay, but it's easier to double
the sleep time that way you've written it. I think it's fine.
It's worth noting that there is no time-based reset of the delay with
this feature, which I think is something that people might expect to go
hand-in-hand with exponential backoff. I think that's probably OK too.
> +static void
> +auth_delay_shmem_startup(void)
> +{
> + Sizerequired;
> + boolfound;
> +
> + if (shmem_startup_next)
> + shmem_startup_next();
> +
> + required = sizeof(AuthConnRecord) * MAX_CONN_RECORDS;
> + acr_array = ShmemInitStruct("Array of AuthConnRecord", required,
> );
> + if (found)
> + /* this should not happen ? */
> + elog(DEBUG1, "variable acr_array already exists");
> + /* all fileds are set to 0 */
> + memset(acr_array, 0,
Re: introduce dynamic shared memory registry
At 2024-01-12 11:21:52 -0600, nathandboss...@gmail.com wrote: > > From: Nathan Bossart > Date: Thu, 11 Jan 2024 21:55:25 -0600 > Subject: [PATCH v6 1/3] reorganize shared memory and lwlocks documentation > > --- > doc/src/sgml/xfunc.sgml | 182 +--- > 1 file changed, 114 insertions(+), 68 deletions(-) > > diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml > index 89116ae74c..0ba52b41d4 100644 > --- a/doc/src/sgml/xfunc.sgml > +++ b/doc/src/sgml/xfunc.sgml > @@ -3397,90 +3397,136 @@ CREATE FUNCTION make_array(anyelement) RETURNS > anyarray > > > […] > - from your shmem_request_hook. > - > - > - LWLocks are reserved by calling: > + Each backend sould obtain a pointer to the reserved shared memory by sould → should > + Add-ins can reserve LWLocks on server startup. Like with shared > memory, (Would "As with shared memory" read better? Maybe, but then again maybe it should be left alone because you also write "Unlike with" elsewhere.) -- Abhijit
Re: Report planning memory in EXPLAIN ANALYZE
At 2024-01-12 17:52:27 +0100, alvhe...@alvh.no-ip.org wrote: > > I think this patch is mostly OK (After the last few rounds of changes, it looks fine to me too.) > Planning: >Buffers: shared hit=120 read=30 >Memory: used=67944 bytes allocated=73728 bytes > Planning Time: 0.892 ms > > […] > > Or we could leave it as you have it, but to me that's akin to giving up > on doing it nicely. For completeness, there's a third option, which is easier to write and a bit more friendly to the sort of thing that might already be parsing "Planning Time", viz., Planning Buffers: shared hit=120 read=30 Planning Memory: used=67944 bytes allocated=73728 bytes Planning Time: 0.892 ms (Those "bytes" look slightly odd to me in the midst of all the x=y pieces, but that's probably not worth thinking about.) -- Abhijit
Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c)
At 2023-09-27 10:37:45 -0300, ranier...@gmail.com wrote: > > Forgive my impulsiveness, anyone who loves perfect, well written code, > would understand. I actually find this characterisation offensive. Being scrupulous about not grouping random drive-by changes together with the primary change is right up there in importance with writing good commit messages to help future readers to reduce their WTFs/minute score one, five, seven, or ten years later. Ignoring that concern once is thoughtless. Ignoring it over and over again is disrespectful. Casually deriding it by equating it to hating "perfect, well-written code" is gross. -- Abhijit
Re: Naming of gss_accept_deleg
At 2023-05-22 09:42:44 -0400, t...@sss.pgh.pa.us wrote:
>
> Alvaro Herrera writes:
> > I noticed that the value that enables this feature at libpq client side
> > is 'enable'. However, for other Boolean settings like sslsni,
> > keepalives, requiressl, sslcompression, the value that enables feature
> > is '1' -- we use strings only for "enum" type of settings.
>
> > Also, it looks like connectOptions2() doesn't validate the string value.
>
> Hmm, it certainly seems like this ought to accept exactly the
> same inputs as other libpq boolean settings. I can take a look
> unless somebody else is already on it.
Here's the diff, but the 0/1 values of settings like sslsni and
sslcompression don't seem to be validated anywhere, unlike the string
options in connectOptions2, so I didn't do anything for gssdelegation.
(I've never run the Kerberos tests before, but I changed one
"gssdelegation=disable" to "gssdelegation=1" and got a test failure, so
they're probably working as expected.)
-- Abhijit
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index e38a7debc3..2225e4e0ef 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2059,9 +2059,9 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
Forward (delegate) GSS credentials to the server. The default is
-disable which means credentials will not be forwarded
-to the server. Set this to enable to have
-credentials forwarded when possible.
+0 which means credentials will not be forwarded
+to the server. Set this to 1 to have credentials
+forwarded when possible.
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index de0e13e50d..88fd0f3d80 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -97,7 +97,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
if (!pg_GSS_have_cred_cache(>gcred))
conn->gcred = GSS_C_NO_CREDENTIAL;
- if (conn->gssdelegation && pg_strcasecmp(conn->gssdelegation, "enable") == 0)
+ if (conn->gssdelegation && conn->gssdelegation[0] == '1')
gss_flags |= GSS_C_DELEG_FLAG;
maj_stat = gss_init_sec_context(_stat,
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 786d22a770..a8584d2c68 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -343,8 +343,8 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"GSS-library", "", 7, /* sizeof("gssapi") == 7 */
offsetof(struct pg_conn, gsslib)},
- {"gssdelegation", "PGGSSDELEGATION", NULL, NULL,
- "GSS-delegation", "", 8, /* sizeof("disable") == 8 */
+ {"gssdelegation", "PGGSSDELEGATION", "0", NULL,
+ "GSS-delegation", "", 1,
offsetof(struct pg_conn, gssdelegation)},
{"replication", NULL, NULL, NULL,
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index c77d5cfe9f..7e373236e9 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -622,7 +622,7 @@ pqsecure_open_gss(PGconn *conn)
if (ret != STATUS_OK)
return PGRES_POLLING_FAILED;
- if (conn->gssdelegation && pg_strcasecmp(conn->gssdelegation, "enable") == 0)
+ if (conn->gssdelegation && conn->gssdelegation[0] == '1')
{
/* Acquire credentials if possible */
if (conn->gcred == GSS_C_NO_CREDENTIAL)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index f1854f9919..0045f83cbf 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -404,7 +404,7 @@ struct pg_conn
char *krbsrvname; /* Kerberos service name */
char *gsslib; /* What GSS library to use ("gssapi" or
* "sspi") */
- char *gssdelegation; /* Try to delegate GSS credentials? */
+ char *gssdelegation; /* Try to delegate GSS credentials? (0 or 1) */
char *ssl_min_protocol_version; /* minimum TLS protocol version */
char *ssl_max_protocol_version; /* maximum TLS protocol version */
char *target_session_attrs; /* desired session properties */
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index bff26fda0c..0deb9bffc8 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -381,7 +381,7 @@ test_access(
'test1',
'SELECT gss_authenticated AND encrypted AND NOT credentials_delegated FROM pg_stat_gssapi WHERE pid = pg_backend_pid();',
0,
- 'gssencmode=prefer gssdelegation=enable',
+ 'gssencmode=prefer gssdelegation=1',
'succeeds with GSS-encrypted access preferred with host hba and credentials not delegated even though asked for (ticket not forwardable)',
"connection authenticated: identity=\"test1\@$realm\" method=gss",
"connection authorized: user=$username database=$dbname application_name=$application GSS (authenticated=yes, encrypted=yes, delegated_credentials=no, principal=test1\@$realm)"
@@ -391,7 +391,7 @@ test_access(
Re: Naming of gss_accept_deleg
At 2023-05-22 09:42:44 -0400, t...@sss.pgh.pa.us wrote: > > Alvaro Herrera writes: > > I noticed that the value that enables this feature at libpq client side > > is 'enable'. However, for other Boolean settings like sslsni, > > keepalives, requiressl, sslcompression, the value that enables feature > > is '1' -- we use strings only for "enum" type of settings. > > > Also, it looks like connectOptions2() doesn't validate the string value. > > Hmm, it certainly seems like this ought to accept exactly the > same inputs as other libpq boolean settings. I can take a look > unless somebody else is already on it. I'm working on it. -- Abhijit
Re: Naming of gss_accept_deleg
At 2023-05-20 23:21:57 -0400, t...@sss.pgh.pa.us wrote:
>
> Nathan Bossart writes:
> > On Sat, May 20, 2023 at 09:33:44PM -0400, Bruce Momjian wrote:
> >> With less then 48 hours to beta 1 packaging, I have made this change and
> >> adjusted internal variable to match.
>
> > The buildfarm and cfbot seem unhappy with 9c0a0e2. It looks like there are
> > a few remaining uses of gss_accept_deleg to rename. I'm planning to commit
> > the attached patch shortly.
>
> I thought the plan was to also rename the libpq "gssdeleg" connection
> parameter and so on? I can look into that tomorrow, if nobody beats
> me to it.
I was trying the change to see if it would be better to name it
"gssdelegate" instead (as in delegate on one side, and accept the
delegation on the other), but decided that "gssdelegation=enable"
reads better than "gssdelegate=enable".
Here's the diff.
-- Abhijit
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 826baac9f1..c8c4614b54 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -172,7 +172,7 @@ ALTER SERVER testserver1 OPTIONS (
--requirepeer 'value',
krbsrvname 'value',
gsslib 'value',
- gssdeleg 'value'
+ gssdelegation 'value'
--replication 'value'
);
-- Error, invalid list syntax
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index fe40d50c6d..5c301e0ef3 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -289,10 +289,10 @@ InitPgFdwOptions(void)
{"sslkey", UserMappingRelationId, true},
/*
- * gssdeleg is also a libpq option but should be allowed in a user
- * mapping context too
+ * gssdelegation is also a libpq option but should be allowed in
+ * a user mapping context too
*/
- {"gssdeleg", UserMappingRelationId, true},
+ {"gssdelegation", UserMappingRelationId, true},
{NULL, InvalidOid, false}
};
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 15f3af6c29..b54903ad8f 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -186,7 +186,7 @@ ALTER SERVER testserver1 OPTIONS (
--requirepeer 'value',
krbsrvname 'value',
gsslib 'value',
- gssdeleg 'value'
+ gssdelegation 'value'
--replication 'value'
);
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index cce25d06e6..e38a7debc3 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2054,8 +2054,8 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
-
- gssdeleg
+
+ gssdelegation
Forward (delegate) GSS credentials to the server. The default is
@@ -8271,10 +8271,10 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
- PGGSSDELEG
+ PGGSSDELEGATION
- PGGSSDELEG behaves the same as the connection parameter.
+ PGGSSDELEGATION behaves the same as the connection parameter.
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 6e1977fa62..ca3ad55b62 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -574,7 +574,7 @@ static const struct ConnectionOption libpq_conninfo_options[] = {
{"requiressl", ForeignServerRelationId},
{"sslmode", ForeignServerRelationId},
{"gsslib", ForeignServerRelationId},
- {"gssdeleg", ForeignServerRelationId},
+ {"gssdelegation", ForeignServerRelationId},
{NULL, InvalidOid}
};
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 0dc31988b4..de0e13e50d 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -97,7 +97,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
if (!pg_GSS_have_cred_cache(>gcred))
conn->gcred = GSS_C_NO_CREDENTIAL;
- if (conn->gssdeleg && pg_strcasecmp(conn->gssdeleg, "enable") == 0)
+ if (conn->gssdelegation && pg_strcasecmp(conn->gssdelegation, "enable") == 0)
gss_flags |= GSS_C_DELEG_FLAG;
maj_stat = gss_init_sec_context(_stat,
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 30486c59ba..786d22a770 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -343,9 +343,9 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"GSS-library", "", 7, /* sizeof("gssapi") == 7 */
offsetof(struct pg_conn, gsslib)},
- {"gssdeleg", "PGGSSDELEG", NULL, NULL,
+ {"gssdelegation", "PGGSSDELEGATION", NULL, NULL,
"GSS-delegation", "", 8, /* sizeof("disable") == 8 */
- offsetof(struct pg_conn, gssdeleg)},
+ offsetof(struct pg_conn, gssdelegation)},
{"replication", NULL, NULL, NULL,
"Replication", "D", 5,
@@ -4453,7 +4453,7 @@ freePGconn(PGconn *conn)
free(conn->gssencmode);
free(conn->krbsrvname);
free(conn->gsslib);
- free(conn->gssdeleg);
+
Re: Naming of gss_accept_deleg
At 2023-05-19 09:16:09 -0400, br...@momjian.us wrote: > > On Fri, May 19, 2023 at 09:07:26AM -0400, Stephen Frost wrote: > > > > > Why is the new PG 16 GUC called "gss_accept_deleg" and not > > > "gss_accept_delegation"? The abbreviation here seems atypical. > > > > At the time it felt natural to me but I don't feel strongly about it, > > happy to change it if folks would prefer it spelled out. > > Yes, please do spell it out, thanks. The fact "deleg" looks similar to > "debug" also doesn't help. Note that GSS-API itself calls it the "DELEG" flag: if (conn->gcred != GSS_C_NO_CREDENTIAL) gss_flags |= GSS_C_DELEG_FLAG; I would also prefer a GUC named gss_accept_delegation, but the current name matches the libpq gssdeleg connection parameter and the PGSSDELEG environment variable. Maybe there's something to be said for keeping those three things alike? -- Abhijit
Re: refactoring basebackup.c
At 2022-02-02 10:55:53 -0500, robertmh...@gmail.com wrote: > > On Tue, Jan 18, 2022 at 1:55 PM Robert Haas wrote: > > 0001 adds "server" and "blackhole" as backup targets. It now has some > > tests. This might be more or less ready to ship, unless somebody else > > sees a problem, or I find one. > > I played around with this a bit and it seems quite easy to extend this > further. So please find attached a couple more patches to generalize > this mechanism. It took me a while to assimilate these patches, including the backup targets one, which I hadn't looked at before. Now that I've wrapped my head around how to put the pieces together, I really like the idea. As you say, writing non-trivial integrations in C will take some effort, but it seems worthwhile. It's also nice that one can continue to use pg_basebackup to trigger the backups and see progress information. > Granted, coding up a new base backup target is > something only experienced C hackers are likely to do, but the fact > that I was able to throw this together so quickly suggests to me that > I've got the design basically right, and that anyone who does want to > plug into the new mechanism shouldn't have too much trouble doing so. > > Thoughts? Yes, it looks simple to follow the example set by basebackup_to_shell to write a custom target. The complexity will be in whatever we need to do to store/forward the backup data, rather than in obtaining the data in the first place, which is exactly as it should be. Thanks! -- Abhijit
Re: SSL compression
At 2021-11-08 10:10:55 +0100, mjbaars1977.pgsql.hack...@gmail.com wrote: > > > https://en.wikipedia.org/wiki/CRIME > > Well Abhijit, personally I don't see any connection between crime and > compression. Reading the page I linked to may help you make the connection. If not, try any of the numerous other pages that talk about vulnerabilities in TLS related to compression. > As I understand it, TLS is a predecessor of SSL. No. SSL is the predecessor. > People are trying to move away from TLS, not from compression. No. -- ams
Re: SSL compression
At 2021-11-08 08:41:42 +0100, mjbaars1977.pgsql.hack...@gmail.com wrote: > > Could someone please explain to me, why compression is being > considered unsafe / insecure? https://en.wikipedia.org/wiki/CRIME > Might the underlying reason be, that certain people have shown > interest in my libpq/PQblockwrite algorithms ( > https://www.postgresql.org/message-id/c7cccd0777f39c53b9514e3824badf276759fa87.camel%40cyberfiber.eu) > but felt turned down and are now persuading me to trade the algorithms > against SSL compression, than just say so please. I'll see what I can > do. The whole world is trying to move away from TLS compression (which has been removed from TLS 1.3). It has nothing to do with you. -- Abhijit
Re: pg_waldump/heapdesc.c and struct field names
At 2021-01-03 19:54:38 -0800, p...@bowt.ie wrote: > > It just seems worth removing gratuitous inconsistencies, > such as this one. Agreed. -- Abhijit
Re: [PATCH] SET search_path += octopus
At 2020-10-28 18:29:30 -0700, and...@anarazel.de wrote:
>
> I think my gut feeling about this still is that it's not worth
> implementing something that doesn't work in postgresql.conf. The
> likelihood of ending up with something that makes it hard to to
> eventually implement proper postgresql.conf seems high.
OK, thanks for explaining. That seems a reasonable concern.
I can't think of a reasonable way to accommodate the variety of possible
modifications to settings in postgresql.conf without introducing some
kind of functional syntax:
shared_preload_libraries =
list('newval', $shared_preload_libraries, 'otherval')
I rather doubt my ability to achieve anything resembling consensus on a
feature like that, even if it were restricted solely to list operations
on a few settings to begin with. I am also concerned that such a feature
would make it substantially harder to understand where and how the value
of a particular setting is derived (even if I do find some way to record
multiple sources in pg_settings—a problem that was brought up in earlier
discussions).
I'm certainly not in favour of introducing multiple operators to express
the various list operations, like += for prepend and =+ for append. (By
the standard of assembling such things from spare parts, the right thing
to do would be to add M4 support to postgresql.conf, but I'm not much of
a fan of that idea either.)
Therefore, for lack of any plausible way to proceed, I do not plan to
work on this feature any more.
-- Abhijit
Re: PATCH: Report libpq version and configuration
At 2020-10-26 20:56:57 +0800, craig.rin...@enterprisedb.com wrote: > > $ ./build/src/interfaces/libpq/libpq.so > VERSION_NUM: 14 > VERSION: PostgreSQL 14devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) > 10.2.1 20200723 (Red Hat 10.2.1-1), 64-bit > CONFIGURE_ARGS: '--cache-file=config.cache-' > '--prefix=/home/craig/pg/master' '--enable-debug' '--enable-cassert' > '--enable-tap-tests' '--enable-dtrace' 'CC=/usr/lib64/ccache/gcc' > 'CFLAGS=-Og -ggdb3' 'CPPFLAGS=' 'CPP=/usr/lib64/ccache/gcc -E' > USE_SSL: 0 > ENABLE_GSS: 0 > ENABLE_THREAD_SAFETY: 1 > HAVE_UNIX_SOCKETS: 1 > DEFAULT_PGSOCKET_DIR: /tmp > DEF_PGPORT: 5432 This is excellent. -- Abhijit
Re: [PATCH] SET search_path += octopus
At 2020-10-20 10:53:04 -0700, and...@anarazel.de wrote: > > > postgres=# ALTER SYSTEM SET max_worker_processes += 4; > > ALTER SYSTEM > > Much less clear that this is a good idea... I agree it's less clear. I still think it might be useful in some cases (such as the example with max_worker_processes quoted above), but it's not as compelling as altering search_path/shared_preload_libraries. (That's partly why I posted it as a separate patch.) > It seems to me that appending and incrementing using the same syntax > is a) confusing b) will be a limitation before long. I understand (a), but what sort of limitation do you foresee in (b)? Do you think both features should be implemented, but with a different syntax, or are you saying incrementing should not be implemented now? > > These patches do not affect configuration file parsing in any way: > > its use is limited to "SET" and "ALTER xxx SET". > > Are you including user / database settings as part of ALTER ... SET? > Or just SYSTEM? Yes, it works the same for all of the ALTER … SET variants, including users and databases. -- Abhijit
[PATCH] SET search_path += octopus
Hi.
The first attached patch
(0001-Accept-SET-xyz-pqr-to-add-pqr-to-the-current-setting.patch) adds
support for commands like the following, applicable to any configuration
settings that are represented as a comma-separated list of strings
(i.e., GUC_LIST_INPUT):
postgres=# SET search_path += octopus;
SET
postgres=# SET search_path += "giant squid", kraken, narwhal; -- [1]
SET
postgres=# SET search_path -= public, narwhal;
SET
postgres=# SHOW search_path;
┌─┐
│ search_path │
├─┤
│ "$user", octopus, "giant squid", kraken │
└─┘
(1 row)
The implementation extends to ALTER SYSTEM SET with next to no effort,
so you can also add entries to shared_preload_libraries without having
to know its current value:
ALTER SYSTEM SET shared_preload_libraries += auto_explain;
The second patch
(0002-Support-SET-syntax-for-numeric-configuration-setting.patch) adds
support to modify numeric configuration settings:
postgres=# SET cpu_tuple_cost += 0.02;
SET
postgres=# SET effective_cache_size += '2GB';
SET
postgres=# SHOW effective_cache_size;
┌──┐
│ effective_cache_size │
├──┤
│ 6GB │
└──┘
(1 row)
postgres=# ALTER SYSTEM SET max_worker_processes += 4;
ALTER SYSTEM
Being able to safely modify shared_preload_libraries (in particular) and
max_worker_processes during automated extension deployments is a problem
I've struggled with more than once in the past.
These patches do not affect configuration file parsing in any way: its
use is limited to "SET" and "ALTER xxx SET". (After I started working on
this, I came to know that this idea has been proposed in different forms
in the past, and objections to those proposals centred around various
difficulties involved in adding this syntax to configuration files. I'm
not particularly fond of that idea, and it's not what I've done here.)
(Another feature that could be implemented using this framework is to
ensure the current setting is at least as large as a given value:
ALTER SYSTEM SET shared_buffers >= '8GB';
This would not change shared_buffers if it were already larger than 8GB.
I have not implemented this, pending feedback on what's already there,
but it would be simple to do.)
Comments welcome.
-- Abhijit
1. This feature supports a wide variety of marine creatures, with no
implied judgement about their status, real or mythical; however,
adding them to shared_preload_libraries is not advisable.
>From b7f262cf98be76215a9b9968c8800831874cf1d7 Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen
Date: Sun, 27 Sep 2020 06:52:30 +0530
Subject: Accept "SET xyz += pqr" to add pqr to the current setting of xyz
A new function called by ExtractSetVariableArgs() modifies the current
value of a configuration setting represented as a comma-separated list
of strings (e.g., search_path) by adding or removing each of the given
arguments, based on new stmt->kind values of VAR_{ADD,SUBTRACT}_VALUE.
Using += x will add x if it is not already present and do nothing
otherwise, and -= x will remove x if it is present and do nothing
otherwise.
The implementation extends to ALTER SYSTEM SET and similar commands, so
this can be used by extension creation scripts to add individual entries
to shared_preload_libraries.
Examples:
SET search_path += my_schema, other_schema;
SET search_path -= public;
ALTER SYSTEM SET shared_preload_libraries += auto_explain;
---
doc/src/sgml/ref/set.sgml | 18
src/backend/parser/gram.y | 22 +
src/backend/parser/scan.l | 23 -
src/backend/tcop/utility.c | 2 +
src/backend/utils/misc/guc.c | 168 +
src/include/nodes/parsenodes.h | 4 +-
src/include/parser/scanner.h | 1 +
7 files changed, 233 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
index 63f312e812..e30e9b42f0 100644
--- a/doc/src/sgml/ref/set.sgml
+++ b/doc/src/sgml/ref/set.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
SET [ SESSION | LOCAL ] configuration_parameter { TO | = } { value | 'value' | DEFAULT }
+SET [ SESSION | LOCAL ] configuration_parameter { += | -= } { value | 'value' }
SET [ SESSION | LOCAL ] TIME ZONE { timezone | LOCAL | DEFAULT }
@@ -40,6 +41,14 @@ SET [ SESSION | LOCAL ] TIME ZONE { timezone
+
+ For configuration parameters that accept a list of values, such as
+ search_path, you can modify the existing setting by adding
+ or removing individual elements with the += and
+ -= syntax. The former will add a value if it is not
+ already present, while the latter will remove an existing value.
+
+
If
Re: Fix a couple of typos in JIT
At 2020-08-20 22:51:41 +1200, dgrowle...@gmail.com wrote: > > > +This is done at query execution time, possibly even only in cases where > > +the relevant task is done a number of times, makes it JIT, rather than > > +ahead-of-time (AOT). Given the way JIT compilation is used in PostgreSQL, > > +the lines between interpretation, AOT and JIT are somewhat blurry. > > […] > > Oh, I see. I missed that. Perhaps it would be better changed to "The > fact that this" Or maybe even: This is JIT, rather than ahead-of-time (AOT) compilation, because it is done at query execution time, and perhaps only in cases where the relevant task is repeated a number of times. Given the way … -- Abhijit
Re: Fix a couple of typos in JIT
At 2020-08-20 22:19:49 +1200, dgrowle...@gmail.com wrote: > > I was just looking over the JIT code and noticed a few comment and > documentation typos. The attached fixes them. The first change does not seem to be correct: -That this is done at query execution time, possibly even only in cases -where the relevant task is done a number of times, makes it JIT, -rather than ahead-of-time (AOT). Given the way JIT compilation is used -in PostgreSQL, the lines between interpretation, AOT and JIT are -somewhat blurry. +This is done at query execution time, possibly even only in cases where +the relevant task is done a number of times, makes it JIT, rather than +ahead-of-time (AOT). Given the way JIT compilation is used in PostgreSQL, +the lines between interpretation, AOT and JIT are somewhat blurry. The original sentence may not be the most shining example of sentence-ry, but it is correct, and removing the "That" breaks it. -- Abhijit
Re: PostgreSQL pollutes the file system
At 2019-03-20 23:22:44 +0100, tomas.von...@2ndquadrant.com wrote: > > I don't really understand what issue are we trying to solve here. > > Can someone describe a scenario where this (name of the binary not > clearly indicating it's related postgres) causes issues in practice? > On my system, there are ~1400 binaries in /usr/bin, and for the vast > majority of them it's rather unclear where do they come from. It sounds like a problem especially when described with charged terms like "pollutes", but I agree with you and others that it just doesn't seem worth the effort to try to rename everything. -- Abhijit
Re: Why aren't we using strsignal(3) ?
At 2018-12-17 11:52:03 -0500, t...@sss.pgh.pa.us wrote: > > While a dozen lines in pgstrsignal.c certainly are not worth worrying > over, getting rid of the configure test for sys_siglist[] would save > some cycles on every build. So I'm tempted to drop it. Thoughts? Removing it makes sense to me. -- Abhijit
Re: pgsql: Integrate recovery.conf into postgresql.conf
At 2018-11-26 10:18:52 -0500, sfr...@snowman.net wrote: > > > This came originally to check that recovery.conf handles correctly > > its recovery targets when multiple ones are defined with the last > > one winning […] > > Ugh, no, I don't agree that this property makes sense to keep and > since we're making these changes, I'd argue that it's exactly the > right time to do away with that. I strongly agree with everything Stephen said here. -- Abhijit
Re: PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS
At 2018-04-09 15:42:35 +0200, tomas.von...@2ndquadrant.com wrote: > > On 04/09/2018 12:29 AM, Bruce Momjian wrote: > > > > An crazy idea would be to have a daemon that checks the logs and > > stops Postgres when it seems something wrong. > > > > That doesn't seem like a very practical way. Not least because Craig's tests showed that you can't rely on *always* getting an error message in the logs. -- Abhijit
Re: Incorrect use of "an" and "a" in code comments and docs
At 2018-03-05 14:42:14 +0900, mich...@paquier.xyz wrote: > > > - sinval is a signal invalidation, so it seems to me that "a" is > > correct, not "an". I guess it depends on whether you read it as "sin-val" or "ess-inval". > diff --git a/src/backend/access/gin/ginvacuum.c > b/src/backend/access/gin/ginvacuum.c > index 398532d80b..8b08b46ff6 100644 > --- a/src/backend/access/gin/ginvacuum.c > +++ b/src/backend/access/gin/ginvacuum.c > @@ -381,7 +381,7 @@ ginVacuumPostingTreeLeaves(GinVacuumState *gvs, > BlockNumber blkno, bool isRoot) > > /* >* All subtree is empty - just return true to indicate that > parent > - * must do a cleanup. Unless we are ROOT an there is way to go > upper. > + * must do a cleanup. Unless we are ROOT and there is way to go > upper. >*/ That particular comment could use some more changes. :-) -- Abhijit
