psql: add \create_function command
Hello hackers, Currently a function definition must include its body inline. Because of this, when storing function definitions in files, linters and syntax highlighters for non-SQL languages (python, perl, tcl, etc) won't work. An example can be seen on: https://github.com/postgres/postgres/blob/5eafacd2797dc0b04a0bde25fbf26bf79903e7c2/src/pl/plpython/sql/plpython_test.sql#L15-L24 To solve the above issue, this patch adds a psql command to create a function and obtain its body from another file. It is used as: \create_function from ./data/max.py max(int,int) returns int LANGUAGE plpython3u Its design is similar to the `\copy` command, which is a frontend version of the COPY statement. This patch is at an initial stage but includes tests with plpython3u, pltcl, plperl and tab completion. Any feedback is welcomed. Best regards, Steve Chavez From 98f505ba81e8ef317d2a9b764348b523346d7f24 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Mon, 8 Jan 2024 18:57:26 -0500 Subject: [PATCH] psql: add \create_function command Currently a function definition must include its body inline. Because of this, when storing function definitions in files, linters and syntax highlighters for non-SQL languages (python, perl, tcl, etc) won't work. This patch adds a psql command to create a function and obtain its body from another file. It is used as: \create_function from ./data/max.py max(int,int) returns int LANGUAGE plpython3u Its design is similar to the `\copy` command, which is a frontend version of the COPY statement. Includes tests with plpython3u, pltcl, plperl and tab completion. --- src/bin/psql/Makefile | 1 + src/bin/psql/command.c| 26 src/bin/psql/create_function.c| 128 ++ src/bin/psql/create_function.h| 15 ++ src/bin/psql/meson.build | 1 + src/bin/psql/tab-complete.c | 17 ++- src/pl/plperl/data/max.pl | 2 + src/pl/plperl/expected/plperl.out | 7 + src/pl/plperl/sql/plperl.sql | 4 + src/pl/plpython/data/max.py | 3 + src/pl/plpython/expected/plpython_test.out| 7 + src/pl/plpython/sql/plpython_test.sql | 4 + src/pl/tcl/data/max.tcl | 2 + src/pl/tcl/expected/pltcl_setup.out | 7 + src/pl/tcl/sql/pltcl_setup.sql| 4 + src/test/regress/data/max.sql | 1 + .../regress/expected/create_function_sql.out | 10 +- src/test/regress/sql/create_function_sql.sql | 4 + 18 files changed, 241 insertions(+), 2 deletions(-) create mode 100644 src/bin/psql/create_function.c create mode 100644 src/bin/psql/create_function.h create mode 100644 src/pl/plperl/data/max.pl create mode 100644 src/pl/plpython/data/max.py create mode 100644 src/pl/tcl/data/max.tcl create mode 100644 src/test/regress/data/max.sql diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index 374c4c3ab8..285291b8ab 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -29,6 +29,7 @@ OBJS = \ command.o \ common.o \ copy.o \ + create_function.o \ crosstabview.o \ describe.o \ help.o \ diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 5c906e4806..d2c5799ed0 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -30,6 +30,7 @@ #include "common/logging.h" #include "common/string.h" #include "copy.h" +#include "create_function.h" #include "crosstabview.h" #include "describe.h" #include "fe_utils/cancel.h" @@ -71,6 +72,7 @@ static backslashResult exec_command_cd(PsqlScanState scan_state, bool active_bra static backslashResult exec_command_conninfo(PsqlScanState scan_state, bool active_branch); static backslashResult exec_command_copy(PsqlScanState scan_state, bool active_branch); static backslashResult exec_command_copyright(PsqlScanState scan_state, bool active_branch); +static backslashResult exec_command_create_function(PsqlScanState scan_state, bool active_branch); static backslashResult exec_command_crosstabview(PsqlScanState scan_state, bool active_branch); static backslashResult exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd); @@ -323,6 +325,8 @@ exec_command(const char *cmd, status = exec_command_copy(scan_state, active_branch); else if (strcmp(cmd, "copyright") == 0) status = exec_command_copyright(scan_state, active_branch); + else if (strcmp(cmd, "create_function") == 0) + status = exec_command_create_function(scan_state, active_branch); else if (strcmp(cmd, "crosstabview") == 0) status = exec_command_crosstabview(scan_state, active_branch); else if (cmd[0] == 'd') @@ -720,6 +724,28 @@ exec_command_copyright(PsqlScanState scan_state, bool active_branch) return PSQL_CMD_S
Re: psql: add \create_function command
> I like your ideas upthread about \file_read and :{filename}
Great ideas! :{filename} looks more convenient to use than \file_read just
because it's one less command to execute.
However, :{?variable_name} is already taken by psql to test whether a
variable is defined or not. It might be confusing to use the same syntax.
How about using the convention of interpreting an identifier as a file path
if it has an slash on it?
This is used in the Nix language and from experience it works very well:
https://nix.dev/manual/nix/2.18/language/values#type-path
It also makes it very clear that you're using a file path, e.g. :{filename}
vs :./filename. Examples:
select jsonb_to_recordset(:./contents.json);
create function foo() returns text AS :/absolute/path/contents.py language
plpython3u;
Any thoughts?
Best regards,
Steve Chavez
On Mon, 29 Jan 2024 at 08:42, Andrew Dunstan wrote:
>
> On 2024-01-26 Fr 15:17, Tom Lane wrote:
> > Pavel Stehule writes:
> >> I don't know, maybe I have a problem with the described use case. I
> cannot
> >> imagine holding the body and head of PL routines in different places
> and I
> >> don't understand the necessity to join it.
> > It seems a little weird to me too, and I would vote against accepting
> > \create_function as described because I think too few people would
> > want to use it. However, the idea of an easy way to pull in a file
> > and convert it to a SQL literal seems like it has many applications.
> >
> >
>
>
> Yes, this proposal is far too narrow and would not cater for many use
> cases I have had in the past.
>
> I like your ideas upthread about \file_read and :{filename}
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>
psql: fix variable existence tab completion
Hello hackers,
psql has the :{?name} syntax for testing a psql variable existence.
But currently doing \echo :{?VERB doesn't trigger tab completion.
This patch fixes it. I've also included a TAP test.
Best regards,
Steve Chavez
From adb1f997b67d8ef603017ab34e1b9061e4e229ea Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sat, 2 Mar 2024 19:06:13 -0500
Subject: [PATCH 1/1] psql: fix variable existence tab completion
---
src/bin/psql/t/010_tab_completion.pl | 8
src/bin/psql/tab-complete.c | 4 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index b6575b075e..b45c39f0f5 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -413,6 +413,14 @@ check_completion(
clear_query();
+# check completion for psql variable test
+check_completion(
+ "\\echo :{?VERB\t",
+ qr/:\{\?VERBOSITY} /,
+ "complete a psql variable test");
+
+clear_query();
+
# check no-completions code path
check_completion("blarg \t\t", qr//, "check completion failure path");
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index ada711d02f..a16dac9e73 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -76,7 +76,7 @@
#endif
/* word break characters */
-#define WORD_BREAKS "\t\n@><=;|&{() "
+#define WORD_BREAKS "\t\n@><=;|&() "
/*
* Since readline doesn't let us pass any state through to the tab completion
@@ -1785,6 +1785,8 @@ psql_completion(const char *text, int start, int end)
matches = complete_from_variables(text, ":'", "'", true);
else if (text[1] == '"')
matches = complete_from_variables(text, ":\"", "\"", true);
+ else if (text[1] == '{' && text[2] == '?')
+ matches = complete_from_variables(text, ":{?", "}", true);
else
matches = complete_from_variables(text, ":", "", true);
}
--
2.40.1
Re: psql: add \create_function command
> Maybe we could go with :{+...} or the like?
> or maybe :{{ ... }}
Tab completion didn't work for :{?} and I noted that the same problem
would arise for :{+ or :{{ (and tab completion would be more important
here). So I fixed that on:
https://www.postgresql.org/message-id/flat/CAGRrpzZU48F2oV3d8eDLr=4tu9xfh5jt9ed+qu1+x91gmh6...@mail.gmail.com
Would be great to have the above fix reviewed/committed to keep making
progress here.
Besides that, since :{ is already sort of a prefix for psql functions, how
about having `:{file()}`? That would be clearer than :{+ or :{{.
Best regards,
Steve Chavez
On Mon, 29 Jan 2024 at 12:29, Pavel Stehule wrote:
>
>
> po 29. 1. 2024 v 18:11 odesílatel Tom Lane napsal:
>
>> Steve Chavez writes:
>> > However, :{?variable_name} is already taken by psql to test whether a
>> > variable is defined or not. It might be confusing to use the same
>> syntax.
>>
>> Hmm. Maybe we could go with :{+...} or the like?
>>
>> > How about using the convention of interpreting an identifier as a file
>> path
>> > if it has an slash on it?
>>
>> Sorry, that is just horrid. foo/bar means division, and "foo/bar"
>> is simply an identifier per SQL standard, so you can't squeeze that
>> in without breaking an ocean of stuff. Plus, there are many use-cases
>> where there's no reason to put a slash in a relative filename.
>>
>
> sometimes paths starts by $ or .
>
> or maybe :{{ ... }}
>
>
>
>>
>> regards, tom lane
>>
>
'converts internal representation to "..."' comment is confusing
Hello hackers, I found "..." confusing in some comments, so this patch changes it to "cstring". Which seems to be the intention after all. Best regards, Steve From cb1792c45ea9a2fbd2c08e185653b60dc262a17d Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Sun, 14 May 2023 18:00:49 -0300 Subject: [PATCH] Fix "..." in comments "..." is confusing, change it to cstring. --- src/backend/utils/adt/name.c| 4 ++-- src/backend/utils/adt/tags | 1 + src/backend/utils/adt/varlena.c | 8 3 files changed, 7 insertions(+), 6 deletions(-) create mode 12 src/backend/utils/adt/tags diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c index 79c1b90030..c136eabdbc 100644 --- a/src/backend/utils/adt/name.c +++ b/src/backend/utils/adt/name.c @@ -38,7 +38,7 @@ /* - * namein - converts "..." to internal representation + * namein - converts cstring to internal representation * * Note: *[Old] Currently if strlen(s) < NAMEDATALEN, the extra chars are nulls @@ -65,7 +65,7 @@ namein(PG_FUNCTION_ARGS) } /* - * nameout - converts internal representation to "..." + * nameout - converts internal representation to cstring */ Datum nameout(PG_FUNCTION_ARGS) diff --git a/src/backend/utils/adt/tags b/src/backend/utils/adt/tags new file mode 12 index 00..8d6288c7b2 --- /dev/null +++ b/src/backend/utils/adt/tags @@ -0,0 +1 @@ +./../../../../tags \ No newline at end of file diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index b571876468..b895413c1d 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -571,7 +571,7 @@ bytea_string_agg_finalfn(PG_FUNCTION_ARGS) } /* - * textin - converts "..." to internal representation + * textin - converts cstring to internal representation */ Datum textin(PG_FUNCTION_ARGS) @@ -582,7 +582,7 @@ textin(PG_FUNCTION_ARGS) } /* - * textout - converts internal representation to "..." + * textout - converts internal representation to cstring */ Datum textout(PG_FUNCTION_ARGS) @@ -626,7 +626,7 @@ textsend(PG_FUNCTION_ARGS) /* - * unknownin - converts "..." to internal representation + * unknownin - converts cstring to internal representation */ Datum unknownin(PG_FUNCTION_ARGS) @@ -638,7 +638,7 @@ unknownin(PG_FUNCTION_ARGS) } /* - * unknownout - converts internal representation to "..." + * unknownout - converts internal representation to cstring */ Datum unknownout(PG_FUNCTION_ARGS) -- 2.34.1
Using make_ctags leaves tags files in git
Hello hackers, I use postgres/src/tools/make_ctags and it works great. But it leaves the tags files ready to be committed in git. So, I've added 'tags' to .gitignore. Best regards, Steve
Re: Using make_ctags leaves tags files in git
Hello Michael, On the previous patch: - `.*.swp` was added, which I entirely agree shouldn't be in .gitignore as it's editor specific(despite me using vim). - The discussion dabbled too much around the *.swp addition. In this case I just propose adding 'tags'. I believe it's reasonable to ignore these as they're produced by make_ctags. Best regards, Steve On Sun, 14 May 2023 at 20:44, Michael Paquier wrote: > On Sun, May 14, 2023 at 06:13:21PM -0300, Steve Chavez wrote: > > I use postgres/src/tools/make_ctags and it works great. But it leaves the > > tags files ready to be committed in git. So, I've added 'tags' to > > .gitignore. > > This has been proposed in the past, where no conclusion was reached > about whether this would be the best move (backup files from vim or > emacs are not ignored, either): > > https://www.postgresql.org/message-id/CAFcNs+rG-DASXzHcecYKvAj+rmxi8CpMAgbpGpEK-mjC96F=l...@mail.gmail.com > > And there are global rules, as well. > -- > Michael >
Re: 'converts internal representation to "..."' comment is confusing
Thanks a lot for the clarification! The "..." looks enigmatic right now. I think cstring would save newcomers some head-scratching. Open to suggestions though. Best regards, Steve On Sun, 14 May 2023 at 22:36, Tom Lane wrote: > Steve Chavez writes: > > I found "..." confusing in some comments, so this patch changes it to > > "cstring". Which seems to be the intention after all. > > Those comments are Berkeley-era, making them probably a decade older > than the "cstring" pseudotype (invented in b663f3443). Perhaps what > you suggest is an improvement, but I'm not sure that appealing to > original intent can make the case. > > regards, tom lane >
Add pg_basetype() function to obtain a DOMAIN base type
Hello hackers,
Currently obtaining the base type of a domain involves a somewhat long
recursive query. Consider:
```
create domain mytext as text;
create domain mytext_child_1 as mytext;
create domain mytext_child_2 as mytext_child_1;
```
To get `mytext_child_2` base type we can do:
```
WITH RECURSIVE
recurse AS (
SELECT
oid,
typbasetype,
COALESCE(NULLIF(typbasetype, 0), oid) AS base
FROM pg_type
UNION
SELECT
t.oid,
b.typbasetype,
COALESCE(NULLIF(b.typbasetype, 0), b.oid) AS base
FROM recurse t
JOIN pg_type b ON t.typbasetype = b.oid
)
SELECT
oid::regtype,
base::regtype
FROM recurse
WHERE typbasetype = 0 and oid = 'mytext_child_2'::regtype;
oid | base
+--
mytext_child_2 | text
```
Core has the `getBaseType` function, which already gets a domain base type
recursively.
I've attached a patch that exposes a `pg_basetype` SQL function that uses
`getBaseType`, so the long query above just becomes:
```
select pg_basetype('mytext_child_2'::regtype);
pg_basetype
-
text
(1 row)
```
Tests and docs are added.
Best regards,
Steve Chavez
From 9be553c2a3896c12d959bc722a808589765f3db0 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sat, 9 Sep 2023 00:58:44 -0300
Subject: [PATCH] Add pg_basetype(regtype)
Currently obtaining the base type of a domain involves a long SQL query,
this specially in the case of recursive domain types.
To solve this, use the already available getBaseType() function,
and expose it as the pg_basetype SQL function.
---
doc/src/sgml/func.sgml | 25 +++
src/backend/utils/adt/misc.c | 14 +++
src/include/catalog/pg_proc.dat | 3 +++
src/test/regress/expected/domain.out | 36
src/test/regress/sql/domain.sql | 17 +
5 files changed, 95 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 24ad87f910..69393ca557 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24723,6 +24723,31 @@ SELECT typlen FROM pg_type WHERE oid = pg_typeof(33);
+
+
+
+ pg_basetype
+
+pg_basetype ( type oid )
+regtype
+
+
+ Returns the OID of the base type of a domain or if the argument is a basetype it returns the same type.
+ If there's a chain of domain dependencies, it will recurse until finding the base type.
+
+
+For example:
+
+CREATE DOMAIN mytext as text;
+
+SELECT pg_basetype('mytext'::regtype);
+ pg_typeof
+---
+ text
+
+
+
+
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 5d78d6dc06..c0c3c9e98b 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -43,6 +43,7 @@
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/syscache.h"
#include "utils/lsyscache.h"
#include "utils/ruleutils.h"
#include "utils/timestamp.h"
@@ -566,6 +567,19 @@ pg_typeof(PG_FUNCTION_ARGS)
PG_RETURN_OID(get_fn_expr_argtype(fcinfo->flinfo, 0));
}
+/*
+ * Return the base type of the argument.
+ */
+Datum
+pg_basetype(PG_FUNCTION_ARGS)
+{
+ Oid oid = PG_GETARG_OID(0);
+
+ if (!SearchSysCacheExists1(TYPEOID, ObjectIdGetDatum(oid)))
+ PG_RETURN_NULL();
+
+ PG_RETURN_OID(getBaseType(oid));
+}
/*
* Implementation of the COLLATE FOR expression; returns the collation
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..f19bf47242 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3874,6 +3874,9 @@
{ oid => '1619', descr => 'type of the argument',
proname => 'pg_typeof', proisstrict => 'f', provolatile => 's',
prorettype => 'regtype', proargtypes => 'any', prosrc => 'pg_typeof' },
+{ oid => '6312', descr => 'get the base type of a domain',
+ proname => 'pg_basetype', proisstrict => 'f', provolatile => 's',
+ prorettype => 'regtype', proargtypes => 'oid', prosrc => 'pg_basetype' },
{ oid => '3162',
descr => 'collation of the argument; implementation of the COLLATION FOR expression',
proname => 'pg_collation_for', proisstrict => 'f', provolatile => 's',
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 6d94e84414..4f0253cd71 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1207,3 +1207,39 @@ create domain testdomain1 as int constraint unsigned check (value > 0);
a
Re: Add pg_basetype() function to obtain a DOMAIN base type
Just to give a data point for the need of this function:
https://dba.stackexchange.com/questions/231879/how-to-get-the-basetype-of-a-domain-in-pg-type
This is also a common use case for services/extensions that require
postgres metadata for their correct functioning, like postgREST or
pg_graphql.
Here's a query for getting domain base types, taken from the postgREST
codebase:
https://github.com/PostgREST/postgrest/blob/531a183b44b36614224fda432335cdaa356b4a0a/src/PostgREST/SchemaCache.hs#L342-L364
So having `pg_basetype` would be really helpful in those cases.
Looking forward to hearing any feedback. Or if this would be a bad idea.
Best regards,
Steve Chavez
On Sat, 9 Sept 2023 at 01:17, Steve Chavez wrote:
> Hello hackers,
>
> Currently obtaining the base type of a domain involves a somewhat long
> recursive query. Consider:
>
> ```
> create domain mytext as text;
> create domain mytext_child_1 as mytext;
> create domain mytext_child_2 as mytext_child_1;
> ```
>
> To get `mytext_child_2` base type we can do:
>
> ```
> WITH RECURSIVE
> recurse AS (
> SELECT
> oid,
> typbasetype,
> COALESCE(NULLIF(typbasetype, 0), oid) AS base
> FROM pg_type
> UNION
> SELECT
> t.oid,
> b.typbasetype,
> COALESCE(NULLIF(b.typbasetype, 0), b.oid) AS base
> FROM recurse t
> JOIN pg_type b ON t.typbasetype = b.oid
> )
> SELECT
> oid::regtype,
> base::regtype
> FROM recurse
> WHERE typbasetype = 0 and oid = 'mytext_child_2'::regtype;
>
> oid | base
> +--
> mytext_child_2 | text
> ```
>
> Core has the `getBaseType` function, which already gets a domain base type
> recursively.
>
> I've attached a patch that exposes a `pg_basetype` SQL function that uses
> `getBaseType`, so the long query above just becomes:
>
> ```
> select pg_basetype('mytext_child_2'::regtype);
> pg_basetype
> -
> text
> (1 row)
> ```
>
> Tests and docs are added.
>
> Best regards,
> Steve Chavez
>
Re: Allow placeholders in ALTER ROLE w/o superuser
Hey Alexander,
Looks like your latest patch addresses the original issue I posted!
So now I can create a placeholder with the USERSET modifier without a
superuser, while non-USERSET placeholders still require superuser:
```sql
create role foo noinherit;
set role to foo;
alter role foo set prefix.bar to true user set;
ALTER ROLE
alter role foo set prefix.baz to true;
ERROR: permission denied to set parameter "prefix.baz"
set role to postgres;
alter role foo set prefix.baz to true;
ALTER ROLE
```
Also USERSET gucs are marked(`(u)`) on `pg_db_role_setting`:
```sql
select * from pg_db_role_setting ;
setdatabase | setrole | setconfig
-+-+--
0 | 16384 | {prefix.bar(u)=true,prefix.baz=true}
```
Which I guess avoids the need for adding columns to `pg_catalog` and makes
the "fix" simpler.
So from my side this all looks good!
Best regards,
Steve
On Sun, 20 Nov 2022 at 12:50, Alexander Korotkov
wrote:
> .On Sun, Nov 20, 2022 at 8:48 PM Alexander Korotkov
> wrote:
> > I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET
> syntax.
> >
> > These options are working only for USERSET GUC variables, but require
> > less privileges to set. I think there is no problem to implement
> >
> > Also it seems that this approach doesn't conflict with future
> > privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be
> > available unless explicitly REVOKEd. That mean we should be able to
> > check those privileges during ALTER ROLE.
> >
> > Opinions on the patch draft?
> >
> > Links
> > 1.
> https://mail.google.com/mail/u/0/?ik=a20b091faa&view=om&permmsgid=msg-f%3A1749871710745577015
>
> Uh, sorry for the wrong link. I meant
> https://www.postgresql.org/message-id/2271988.1668807...@sss.pgh.pa.us
>
> --
> Regards,
> Alexander Korotkov
>
csv_populate_recordset and csv_agg
Hello hackers, The `json_populate_recordset` and `json_agg` functions allow systems to process/generate json directly on the database. This "cut outs the middle tier"[1] and notably reduces the complexity of web applications. CSV processing is also a common use case and PostgreSQL has the COPY .. FROM .. CSV form but COPY is not compatible with libpq pipeline mode and the interface is clunkier to use. I propose to include two new functions: - csv_populate_recordset ( base anyelement, from_csv text ) - csv_agg ( anyelement ) I would gladly implement these if it sounds like a good idea. I see there's already some code that deals with CSV on - src/backend/commands/copyfromparse.c(CopyReadAttributesCSV) - src/fe_utils/print.c(csv_print_field) - src/backend/utils/error/csvlog(write_csvlog) So perhaps a new csv module could benefit the codebase as well. Best regards, Steve [1]: https://www.crunchydata.com/blog/generating-json-directly-from-postgres
Assert name/short_desc to prevent SHOW ALL segfault
Hello hackers,
The DefineCustomStringVariable function(or any
other DefineCustomXXXVariable) has a short_desc parameter that can be
NULL and it's not apparent that this will lead to a segfault when SHOW ALL
is used.
This happens because the ShowAllGUCConfig function expects a non-NULL
short_desc.
This happened for the Supabase supautils extension(
https://github.com/supabase/supautils/issues/24) and any other extension
that uses the DefineCustomXXXVariable has the same bug risk.
This patch does an Assert on the short_desc(also on the name as an extra
measure), so a postgres built with --enable-cassert can prevent the above
issue.
---
Steve Chavez
Engineering at https://supabase.com/
From ad8a61125a0fe33853d459f12e521dff771130d4 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Thu, 19 May 2022 08:59:46 -0500
Subject: [PATCH] Assert name/short_desc to prevent SHOWALL segfault
Every DefineCustomXXXVariable function can have a NULL short_desc,
and it's not apparent that this will lead to a segfault when SHOW ALL is
used. This happens because ShowAllGUCConfig expects a non-NULL
short_desc.
Assertions are added to init_custom_variable to ensure name and
short_desc are present at minimum.
---
src/backend/utils/misc/guc.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..635d39b7d4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9239,6 +9239,10 @@ init_custom_variable(const char *name,
{
struct config_generic *gen;
+ /* Ensure at least the name and short description are present */
+ Assert(name != NULL);
+ Assert(short_desc != NULL);
+
/*
* Only allow custom PGC_POSTMASTER variables to be created during shared
* library preload; any later than that, we can't ensure that the value
--
2.32.0
Re: Assert name/short_desc to prevent SHOW ALL segfault
Thank you for the reviews Nathan, Michael.
I agree with handling NULL in ShowAllGUCConfig() instead.
I've attached the updated patch.
--
Steve Chavez
Engineering at https://supabase.com/
On Tue, 24 May 2022 at 20:21, Michael Paquier wrote:
> On Tue, May 24, 2022 at 11:41:49AM -0700, Nathan Bossart wrote:
> > I would actually ERROR on this so that we aren't relying on
> > --enable-cassert builds to catch it. That being said, if there's no
> strong
> > reason to enforce that a short description be provided, then why not
> adjust
> > ShowAllGUCConfig() to set that column to NULL when short_desc is missing?
>
> Well, issuing an ERROR on the stable branches would be troublesome for
> extension developers when reloading after a minor update if they did
> not set their short_desc in a custom GUC. So, while I'd like to
> encourage the use of short_desc, using your suggestion to make the
> code more flexible with NULL is fine by me. GetConfigOptionByNum()
> does that for long_desc by the way, meaning that we also have a
> problem there on a build with --enable-nls for short_desc, no?
> --
> Michael
>
From 7b3d1451938f37d08e692a1f252b2f71c1ae5279 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Tue, 24 May 2022 23:46:40 -0500
Subject: [PATCH] Handle a NULL short_desc in ShowAllGUCConfig
Prevent a segfault when using a SHOW ALL, in case a DefineCustomXXXVariable()
was defined with a NULL short_desc.
---
src/backend/utils/misc/guc.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..d7dd727d45 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9780,7 +9780,16 @@ ShowAllGUCConfig(DestReceiver *dest)
isnull[1] = true;
}
- values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ if (conf->short_desc)
+ {
+ values[2] = PointerGetDatum(cstring_to_text(conf->short_desc));
+ isnull[2] = false;
+ }
+ else
+ {
+ values[2] = PointerGetDatum(NULL);
+ isnull[2] = true;
+ }
/* send it to dest */
do_tup_output(tstate, values, isnull);
@@ -9792,7 +9801,10 @@ ShowAllGUCConfig(DestReceiver *dest)
pfree(setting);
pfree(DatumGetPointer(values[1]));
}
- pfree(DatumGetPointer(values[2]));
+ if (conf->short_desc)
+ {
+ pfree(DatumGetPointer(values[2]));
+ }
}
end_tup_output(tstate);
--
2.32.0
Allow placeholders in ALTER ROLE w/o superuser
Hello hackers,
Using placeholders for application variables allows the use of RLS for
application users as shown in this blog post
https://www.2ndquadrant.com/en/blog/application-users-vs-row-level-security/
.
SET my.username = 'tomas'
CREATE POLICY chat_policy ON chat
USING (current_setting('my.username') IN (message_from, message_to))
WITH CHECK (message_from = current_setting('my.username'))
This technique has enabled postgres sidecar services(PostgREST,
PostGraphQL, etc) to keep the application security at the database level,
which has worked great.
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"
Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
services to store metadata scoped to its pertaining role.
I've attached a patch that removes this restriction. From my testing, this
doesn't affect permission checking when an extension defines its custom GUC
variables.
DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
PGC_SUSET, ..);
Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".
---
Steve Chavez
Engineering at https://supabase.com/
From c3493373a8cddfff56d10bddce1dcdabc0722a34 Mon Sep 17 00:00:00 2001
From: Steve Chavez
Date: Sun, 5 Jun 2022 19:10:52 -0500
Subject: [PATCH] Allow placeholders in ALTER ROLE w/o superuser
Removes inconsistent superuser check for placeholders on
validate_option_array_item.
---
src/backend/utils/misc/guc.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..7c83bc3004 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11658,19 +11658,12 @@ validate_option_array_item(const char *name, const char *value,
struct config_generic *gconf;
/*
- * There are three cases to consider:
+ * There are two cases to consider:
*
* name is a known GUC variable. Check the value normally, check
* permissions normally (i.e., allow if variable is USERSET, or if it's
* SUSET and user is superuser).
*
- * name is not known, but exists or can be created as a placeholder (i.e.,
- * it has a valid custom name). We allow this case if you're a superuser,
- * otherwise not. Superusers are assumed to know what they're doing. We
- * can't allow it for other users, because when the placeholder is
- * resolved it might turn out to be a SUSET variable;
- * define_custom_variable assumes we checked that.
- *
* name is not known and can't be created as a placeholder. Throw error,
* unless skipIfNoPermissions is true, in which case return false.
*/
@@ -11681,21 +11674,6 @@ validate_option_array_item(const char *name, const char *value,
return false;
}
- if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)
- {
- /*
- * We cannot do any meaningful check on the value, so only permissions
- * are useful to check.
- */
- if (superuser())
- return true;
- if (skipIfNoPermissions)
- return false;
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to set parameter \"%s\"", name)));
- }
-
/* manual permissions check so we can avoid an error being thrown */
if (gconf->context == PGC_USERSET)
/* ok */ ;
--
2.32.0
Add red-black tree missing comparison searches
Hello hackers,
Currently the red-black tree implementation only has an equality search.
Other extensions might need other comparison searches, like less-or-equal
or greater-or-equal. For example OrioleDB defines a greater-or-equal search
on its postgres fork:
https://github.com/orioledb/postgres/blob/4c18ae94c20e3e95c374b9947f1ace7d1d6497a1/src/backend/lib/rbtree.c#L164-L186
So I thought this might be valuable to have in core. I've added
less-or-equal and greater-or-equal searches functions plus tests in
the test_rbtree module. I can add the remaining less/great searches if this
is deemed worth it.
Also I refactored the sentinel used in the rbtree.c to use C99 designators.
Thanks in advance for any feedback!
--
Steve Chavez
Engineering at https://supabase.com/
From 85435fe0fad92d593940f98a493d1acd973ccda2 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Tue, 28 Jun 2022 23:46:38 -0500
Subject: [PATCH] Add red-black tree missing comparison searches
* Added find great/less or equal
* Change rbtree sentinel to C99 designator
---
src/backend/lib/rbtree.c | 60 +++-
src/include/lib/rbtree.h | 2 +
src/test/modules/test_rbtree/test_rbtree.c | 104 +
3 files changed, 165 insertions(+), 1 deletion(-)
diff --git a/src/backend/lib/rbtree.c b/src/backend/lib/rbtree.c
index a9981dbada..af3990d01c 100644
--- a/src/backend/lib/rbtree.c
+++ b/src/backend/lib/rbtree.c
@@ -62,7 +62,7 @@ struct RBTree
static RBTNode sentinel =
{
- RBTBLACK, RBTNIL, RBTNIL, NULL
+ .color = RBTBLACK,.left = RBTNIL,.right = RBTNIL,.parent = NULL
};
@@ -161,6 +161,64 @@ rbt_find(RBTree *rbt, const RBTNode *data)
return NULL;
}
+/*
+ * rbt_find_great_equal: search for an equal or greater value in an RBTree
+ *
+ * Returns the matching tree entry, or NULL if no match is found.
+ */
+RBTNode *
+rbt_find_great_equal(RBTree *rbt, const RBTNode *data)
+{
+ RBTNode*node = rbt->root;
+ RBTNode*greater = NULL;
+
+ while (node != RBTNIL)
+ {
+ int cmp = rbt->comparator(data, node, rbt->arg);
+
+ if (cmp == 0)
+ return node;
+ else if (cmp < 0)
+ {
+ greater = node;
+ node = node->left;
+ }
+ else
+ node = node->right;
+ }
+
+ return greater;
+}
+
+/*
+ * rbt_find_less_equal: search for an equal or lesser value in an RBTree
+ *
+ * Returns the matching tree entry, or NULL if no match is found.
+ */
+RBTNode *
+rbt_find_less_equal(RBTree *rbt, const RBTNode *data)
+{
+ RBTNode*node = rbt->root;
+ RBTNode*lesser = NULL;
+
+ while (node != RBTNIL)
+ {
+ int cmp = rbt->comparator(data, node, rbt->arg);
+
+ if (cmp == 0)
+ return node;
+ else if (cmp < 0)
+ node = node->left;
+ else
+ {
+ lesser = node;
+ node = node->right;
+ }
+ }
+
+ return lesser;
+}
+
/*
* rbt_leftmost: fetch the leftmost (smallest-valued) tree node.
* Returns NULL if tree is empty.
diff --git a/src/include/lib/rbtree.h b/src/include/lib/rbtree.h
index 580a3e3439..2e8f5b0a8f 100644
--- a/src/include/lib/rbtree.h
+++ b/src/include/lib/rbtree.h
@@ -67,6 +67,8 @@ extern RBTree *rbt_create(Size node_size,
void *arg);
extern RBTNode *rbt_find(RBTree *rbt, const RBTNode *data);
+extern RBTNode *rbt_find_great_equal(RBTree *rbt, const RBTNode *data);
+extern RBTNode *rbt_find_less_equal(RBTree *rbt, const RBTNode *data);
extern RBTNode *rbt_leftmost(RBTree *rbt);
extern RBTNode *rbt_insert(RBTree *rbt, const RBTNode *data, bool *isNew);
diff --git a/src/test/modules/test_rbtree/test_rbtree.c b/src/test/modules/test_rbtree/test_rbtree.c
index 7cb38759a2..6a5a8abee2 100644
--- a/src/test/modules/test_rbtree/test_rbtree.c
+++ b/src/test/modules/test_rbtree/test_rbtree.c
@@ -278,6 +278,108 @@ testfind(int size)
}
}
+/*
+ * Check the correctness of the rbt_find_great_equal operation by searching for
+ * an equal key and all of the greater keys.
+ */
+static void
+testfindgte(int size)
+{
+ RBTree *tree = create_int_rbtree();
+
+ /*
+ * Using the size as the random key to search wouldn't allow us to get at
+ * least one greater match, so we do size - 1
+ */
+ int randomKey = pg_prng_uint64_range(&pg_global_prng_state, 0, size - 1);
+ IntRBTreeNode searchNode = {.key = randomKey};
+ IntRBTreeNode *eqNode;
+ IntRBTreeNode *gtNode;
+
+ /* Insert natural numbers */
+ rbt_populate(tree, size, 1);
+
+ /*
+ * Since the search key is included in the naturals of the tree, we're
+ * sure to find an equal match
+ */
+ eqNode = (IntRBTreeNode *) rbt_find_great_equal(tree, (RBTNode *) &searchNode);
+
+ if (eqNode == NULL)
+ elog(ERROR, "key was not found");
+
+ /* ensure we find an equal match */
+ if (!(eqNode->key == searchNode.key))
+ elog(ERROR, "find_great_equal operation in rbtree didn't find an equal key");
+
+ /* Delete the equal match so we can find greater matches */
+ rbt_delete(tree, (RBTNode *) eqNode);
+
+ /* Find the rest of the
Re: Add red-black tree missing comparison searches
Yes, I've already added it here: https://commitfest.postgresql.org/38/3742/ Thanks! On Thu, 30 Jun 2022 at 12:09, Greg Stark wrote: > Please add this to the commitfest at > https://commitfest.postgresql.org/38/ so it doesn't get missed. The > commitfest starts imminently so best add it today. >
Re: Add red-black tree missing comparison searches
Hey Alexander,
> But I think we can support strict inequalities too (e.g.
less and greater without equals). Could you please make it a bool
argument equal_matches?
Sure, an argument is a good idea to keep the code shorter.
> Could you please extract this change as a separate patch.
Done!
On Thu, 30 Jun 2022 at 14:34, Alexander Korotkov
wrote:
> Hi, Steve!
>
> Thank you for working on this.
>
> On Thu, Jun 30, 2022 at 7:51 PM Steve Chavez wrote:
> > Currently the red-black tree implementation only has an equality search.
> Other extensions might need other comparison searches, like less-or-equal
> or greater-or-equal. For example OrioleDB defines a greater-or-equal search
> on its postgres fork:
> >
> >
> https://github.com/orioledb/postgres/blob/4c18ae94c20e3e95c374b9947f1ace7d1d6497a1/src/backend/lib/rbtree.c#L164-L186
> >
> > So I thought this might be valuable to have in core. I've added
> less-or-equal and greater-or-equal searches functions plus tests in the
> test_rbtree module. I can add the remaining less/great searches if this is
> deemed worth it.
>
> Looks good. But I think we can support strict inequalities too (e.g.
> less and greater without equals). Could you please make it a bool
> argument equal_matches?
>
> > Also I refactored the sentinel used in the rbtree.c to use C99
> designators.
>
> Could you please extract this change as a separate patch.
>
> --
> Regards,
> Alexander Korotkov
>
From 8046463e2c98fb4c51ec05740cee130bdf2ad533 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Tue, 28 Jun 2022 23:46:38 -0500
Subject: [PATCH 1/2] Change rbtree sentinel to C99 designator
---
src/backend/lib/rbtree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/lib/rbtree.c b/src/backend/lib/rbtree.c
index a9981dbada..e1cc4110bd 100644
--- a/src/backend/lib/rbtree.c
+++ b/src/backend/lib/rbtree.c
@@ -62,7 +62,7 @@ struct RBTree
static RBTNode sentinel =
{
- RBTBLACK, RBTNIL, RBTNIL, NULL
+ .color = RBTBLACK,.left = RBTNIL,.right = RBTNIL,.parent = NULL
};
--
2.34.1
From 5d49ddd764b05083e1c276df0b50d43ec6896931 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Wed, 29 Jun 2022 16:28:02 -0500
Subject: [PATCH 2/2] Add rbtree missing comparison searches
* Add find great/less or equal by adding an argument
---
src/backend/lib/rbtree.c | 61 +
src/include/lib/rbtree.h | 2 +
src/test/modules/test_rbtree/test_rbtree.c | 101 +
3 files changed, 164 insertions(+)
diff --git a/src/backend/lib/rbtree.c b/src/backend/lib/rbtree.c
index e1cc4110bd..92f6b99164 100644
--- a/src/backend/lib/rbtree.c
+++ b/src/backend/lib/rbtree.c
@@ -161,6 +161,67 @@ rbt_find(RBTree *rbt, const RBTNode *data)
return NULL;
}
+/*
+ * rbt_find_great: search for a greater value in an RBTree
+ *
+ * If equal_match is true, this will be a great or equal search.
+ *
+ * Returns the matching tree entry, or NULL if no match is found.
+ */
+RBTNode *
+rbt_find_great(RBTree *rbt, const RBTNode *data, bool equal_match)
+{
+ RBTNode*node = rbt->root;
+ RBTNode*greater = NULL;
+
+ while (node != RBTNIL)
+ {
+ int cmp = rbt->comparator(data, node, rbt->arg);
+
+ if (equal_match && cmp == 0)
+ return node;
+ else if (cmp < 0)
+ {
+ greater = node;
+ node = node->left;
+ }
+ else
+ node = node->right;
+ }
+
+ return greater;
+}
+
+/*
+ * rbt_find_less: search for a lesser value in an RBTree
+ *
+ * If equal_match is true, this will be a less or equal search.
+ *
+ * Returns the matching tree entry, or NULL if no match is found.
+ */
+RBTNode *
+rbt_find_less(RBTree *rbt, const RBTNode *data, bool equal_match)
+{
+ RBTNode*node = rbt->root;
+ RBTNode*lesser = NULL;
+
+ while (node != RBTNIL)
+ {
+ int cmp = rbt->comparator(data, node, rbt->arg);
+
+ if (equal_match && cmp == 0)
+ return node;
+ else if (cmp > 0){
+ lesser = node;
+ node = node->right;
+ }
+ else
+ node = node->left;
+ }
+
+ return lesser;
+}
+
/*
* rbt_leftmost: fetch the leftmost (smallest-valued) tree node.
* Returns NULL if tree is empty.
diff --git a/src/include/lib/rbtree.h b/src/include/lib/rbtree.h
index 580a3e3439..13cf68415e 100644
--- a/src/include/lib/rbtree.h
+++ b/src/include/lib/rbtree.h
@@ -67,6 +67,8 @@ extern RBTree *rbt_create(Size node_size,
void *arg);
extern RBTNode *rbt_find(RBTree *rbt, const RBTNode *data);
+extern RBTNode *rbt_find_great(RBTree *rbt, const RBTNode *data, bool equal_match);
+extern RBTNode *rbt_find_less(RBTree *rbt, const RBTNode *data, bool equal_match);
extern RBTNode *rbt_leftmost(RBTree *rbt);
extern RBTNode *rbt_insert(RBTree *rbt, const RBTNode *data, bool *isNew);
diff --git a/src/test/modules/test_rbtree/test_rbtree.c b/src/test/modules/test_rbtree/test_rbt
Fwd: Add red-black tree missing comparison searches
-- Forwarded message - From: Steve Chavez Date: Wed, 6 Jul 2022 at 18:14 Subject: Re: Add red-black tree missing comparison searches To: Alexander Korotkov Thanks Alexander! wrt to the new patch. I think the following comment is misleading since keyDeleted can be true or false: + /* switch equal_match to false so we only find greater matches now */ + node = (IntRBTreeNode *) rbt_find_great(tree, (RBTNode *) &searchNode, + keyDeleted); Maybe it should be the same used for searching lesser keys: + /* + * Find the next key. If the current key is deleted, we can pass + * equal_match == true and still find the next one. + */ On Wed, 6 Jul 2022 at 13:53, Alexander Korotkov wrote: > Hi, Steve! > > On Sat, Jul 2, 2022 at 10:38 PM Steve Chavez wrote: > > > But I think we can support strict inequalities too (e.g. > > less and greater without equals). Could you please make it a bool > > argument equal_matches? > > > > Sure, an argument is a good idea to keep the code shorter. > > > > > Could you please extract this change as a separate patch. > > > > Done! > > Thank you! > > I did some improvements to the test suite, run pgindent and wrote > commit messages. > > I think this is quite straightforward and low-risk patch. I'm going > to push it if no objections. > > -- > Regards, > Alexander Korotkov >
Re: Allow placeholders in ALTER ROLE w/o superuser
Thanks a lot for the feedback Nathan.
Taking your options into consideration, for me the correct behaviour should
be:
- The ALTER ROLE placeholder should always be stored with a PGC_USERSET
GucContext. It's a placeholder anyway, so it should be the less restrictive
one. If the user wants to define it as PGC_SUSET or other this should be
done through a custom extension.
- When an extension claims the placeholder, we should check the
DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match,
then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can
WARN or ERR about the defined role GUCs.
What do you think?
On Mon, 18 Jul 2022 at 19:03, Nathan Bossart
wrote:
> On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
> > On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
> >> However, defining placeholders at the role level require superuser:
> >>
> >> alter role myrole set my.username to 'tomas';
> >> ERROR: permission denied to set parameter "my.username"
> >>
> >> Which is inconsistent and surprising behavior. I think it doesn't make
> >> sense since you can already set them at the session or transaction
> >> level(SET LOCAL my.username = 'tomas'). Enabling this would allow
> sidecar
> >> services to store metadata scoped to its pertaining role.
> >>
> >> I've attached a patch that removes this restriction. From my testing,
> this
> >> doesn't affect permission checking when an extension defines its custom
> GUC
> >> variables.
> >>
> >>DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom,
> NULL,
> >> PGC_SUSET, ..);
> >>
> >> Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
> >> when doing "make installcheck".
> >
> > IIUC you are basically proposing to revert a6dcd19 [0], but it is not
> clear
> > to me why that is safe. Am I missing something?
>
> I spent some more time looking into this, and I think I've constructed a
> simple example that demonstrates the problem with removing this
> restriction.
>
> postgres=# CREATE ROLE test CREATEROLE;
> CREATE ROLE
> postgres=# CREATE ROLE other LOGIN;
> CREATE ROLE
> postgres=# GRANT CREATE ON DATABASE postgres TO other;
> GRANT
> postgres=# SET ROLE test;
> SET
> postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
> ALTER ROLE
> postgres=> \c postgres other
> You are now connected to database "postgres" as user "other".
> postgres=> CREATE EXTENSION plperl;
> CREATE EXTENSION
> postgres=> SHOW plperl.on_plperl_init;
> plperl.on_plperl_init
> ---
> test
> (1 row)
>
> In this example, the non-superuser role sets a placeholder GUC for another
> role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
> non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
> record of who ran ALTER ROLE, we might be able to apply appropriate
> permissions checking when the module is loaded, but this information
> doesn't exist in pg_db_role_setting. IIUC we have the following options:
>
> 1. Store information about who ran ALTER ROLE. I think there are a
>couple of problems with this. For example, what happens if the
>grantor was dropped or its privileges were altered? Should we
>instead store the context of the user (i.e., PGC_USERSET or
>PGC_SUSET)? Do we need to add entries to pg_depend?
> 2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs.
> Since
>we don't know who ran ALTER ROLE, we can't trust the value.
> 3. Require superuser to use ALTER ROLE for a placeholder. This is
> what
>we do today. Since we know a superuser set the value, we can
> always
>apply it when the custom GUC is finally defined.
>
> If this is an accurate representation of the options, it seems clear why
> the superuser restriction is in place.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>
Re: 'converts internal representation to "..."' comment is confusing
Hello hackers, Tom, could we apply this patch since Robert agrees it's an improvement? Best regards, Steve On Tue, 16 May 2023 at 07:49, Robert Haas wrote: > On Sun, May 14, 2023 at 9:37 PM Tom Lane wrote: > > Steve Chavez writes: > > > I found "..." confusing in some comments, so this patch changes it to > > > "cstring". Which seems to be the intention after all. > > > > Those comments are Berkeley-era, making them probably a decade older > > than the "cstring" pseudotype (invented in b663f3443). Perhaps what > > you suggest is an improvement, but I'm not sure that appealing to > > original intent can make the case. > > FWIW, it does seem like an improvement to me. > > -- > Robert Haas > EDB: http://www.enterprisedb.com >
Castable Domains for different JSON representations
Hello hackers,
Currently domain casts are ignored. Yet this would be very useful for
representing data in different formats such as json.
Let's take a tsrange as an example. Its json output by default:
select to_json('(2022-12-31 11:00, 2023-01-01 06:00)'::tsrange);
to_json
-
"(\"2022-12-31 11:00:00\",\"2023-01-01 06:00:00\")"
We can refine its representation in a custom way as:
-- using a custom type for this example
create type mytsrange as range (subtype = timestamp, subtype_diff =
tsrange_subdiff);
create or replace function mytsrange_to_json(mytsrange) returns json as $$
select json_build_object(
'lower', lower($1)
, 'upper', upper($1)
, 'lower_inc', lower_inc($1)
, 'upper_inc', upper_inc($1)
);
$$ language sql;
create cast (mytsrange as json) with function mytsrange_to_json(mytsrange)
as assignment;
-- now we get the custom representation
select to_json('(2022-12-31 11:00, 2023-01-01 06:00)'::mytsrange);
to_json
--
{"lower" : "2022-12-31T11:00:00", "upper" : "2023-01-01T06:00:00",
"lower_inc" : false, "upper_inc" : false}
(1 row)
Although this works for this example, using a custom type requires
knowledge of the `tsrange` internals. It would be much simpler to do:
create domain mytsrange as range;
But casts on domains are currently ignored:
create cast (mytsrange as json) with function mytsrange_to_json(mytsrange)
as assignment;
WARNING: cast will be ignored because the source data type is a domain
CREATE CAST
Checking the code seems supporting this is a TODO? Or are there any other
concerns of why this shouldn't be done?
I would like to work on this if there is an agreement.
Best regards,
Steve
Fwd: Castable Domains for different JSON representations
> The bigger picture here, though, is what are you really buying
compared to just invoking the special conversion function explicitly?
> If you have to write "sometsrangecolumn::mytsrange::json", that's
not shorter and certainly not clearer than writing a function call.
The main benefit is to be able to call `json_agg` on tables with these
custom json representations. Then the defined json casts work
transparently when doing:
select json_agg(x) from mytbl x;
json_agg
---
[{"id":1,"val":{"lower" : "2022-12-31T11:00:00", "upper" :
"2023-01-01T06:00:00", "lower_inc" : false, "upper_inc" : false}}]
-- example table
create table mytbl(id int, val mytsrange);
insert into mytbl values (1, '(2022-12-31 11:00, 2023-01-01 06:00)');
This output is directly consumable on web applications and as
you can see the expression is pretty short, with no need to use
the explicit casts as `json_agg` already does them internally.
Best regards,
Steve
[PATCH] clarify palloc comment on quote_literal_cstr
Hello hackers, I found the numbers in `quote_literal_cstr` palloc quite magical. So I've added a comment clarifying what they mean. The change is small: /* We make a worst-case result area; wasting a little space is OK */ - result = palloc(len * 2 + 3 + 1); + result = palloc( + (len * 2)/* worst-case doubling for every character if each one is a quote */ + + 3 /* two outer quotes + possibly 'E' if needed */ + + 1 /* null terminator */ + ); Best regards, Steve From 1eb0a72ea819ee9217d84abb8112e72bf0df61d8 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Sun, 6 Apr 2025 12:33:55 -0500 Subject: [PATCH] clarify palloc comment on quote_literal_cstr --- src/backend/utils/adt/quote.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/quote.c b/src/backend/utils/adt/quote.c index 677efb93e8d..4b9c52e6d9e 100644 --- a/src/backend/utils/adt/quote.c +++ b/src/backend/utils/adt/quote.c @@ -108,7 +108,11 @@ quote_literal_cstr(const char *rawstr) len = strlen(rawstr); /* We make a worst-case result area; wasting a little space is OK */ - result = palloc(len * 2 + 3 + 1); + result = palloc( + (len * 2)/* worst-case doubling for every character if each one is a quote */ + + 3 /* two outer quotes + possibly 'E' if needed */ + + 1 /* null terminator */ + ); newlen = quote_literal_internal(result, rawstr, len); result[newlen] = '\0'; -- 2.40.1
Re: [PATCH] clarify palloc comment on quote_literal_cstr
I haven't found a similar style of comment on any other function call. I've attached a new patch using the style you suggest. That being said, I do find the first form much more readable, but I understand this is a debatable subject. Best regards, Steve Chavez On Mon, 7 Apr 2025 at 06:33, Ashutosh Bapat wrote: > On Mon, Apr 7, 2025 at 3:19 AM Michael Paquier > wrote: > > > > On Sun, Apr 06, 2025 at 12:37:24PM -0500, Steve Chavez wrote: > > > I found the numbers in `quote_literal_cstr` palloc quite magical. So > I've > > > added a comment clarifying what they mean. The change is small: > > > > > > /* We make a worst-case result area; wasting a little space is > OK */ > > > - result = palloc(len * 2 + 3 + 1); > > > + result = palloc( > > > + (len * 2)/* worst-case doubling for every > character if > > > each one is a quote */ > > > + + 3 /* two outer quotes + possibly 'E' if > needed */ > > > + + 1 /* null terminator */ > > > + ); > > > > Agreed that this is a good idea to have a better documentation here if > > someone decides to tweak this area in the future, rather than have > > them guess the numbers. > > > > Looking at what quote_literal_internal() does, it seems to me that you > > are right based on ESCAPE_STRING_SYNTAX, SQL_STR_DOUBLE(), and the > > extra backslashes added to the destination buffer. > > This is an improvement in readability. However, I have not seen this > style of commenting arguments of a function. If there's a precedence, > please let me know. Usually we explain it in a comment before the > call. For example, something like > /* Allocate memory for worst case result factoring in a. double the > length number of bytes, in case every character is a quote, b. two > bytes for two outer quotes c. extra byte for E' d. one byte for null > terminator. */ > > -- > Best Wishes, > Ashutosh Bapat > From 99f64dfc687273ca1ef90199b8c6a9addb6578a9 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Mon, 7 Apr 2025 13:34:55 -0500 Subject: [PATCH] clarify palloc comment on quote_literal_cstr --- src/backend/utils/adt/quote.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/quote.c b/src/backend/utils/adt/quote.c index 677efb93e8d..60bda20606d 100644 --- a/src/backend/utils/adt/quote.c +++ b/src/backend/utils/adt/quote.c @@ -107,7 +107,10 @@ quote_literal_cstr(const char *rawstr) int newlen; len = strlen(rawstr); - /* We make a worst-case result area; wasting a little space is OK */ + /* Allocate memory for worst case result factoring in a. double the + length number of bytes, in case every character is a quote, b. two + bytes for two outer quotes c. extra byte for E' d. one byte for null + terminator. */ result = palloc(len * 2 + 3 + 1); newlen = quote_literal_internal(result, rawstr, len); -- 2.40.1
Re: Allow database owners to CREATE EVENT TRIGGER
Hello hackers, I've now modified my patch to allow regular users (not just database owners) to create event triggers. As mentioned by Tom before, this now relies on role membership to allowlist the target roles of the event trigger. A summary of the changes: - EventTriggerCacheItem now has an owneroid - EventTriggerCommonSetup now returns a list of EventTriggerCacheItem instead of a list of function oids - The cache items are used in EventTriggerInvoke to discriminate the role memberships using the `is_member_of_role_nosuper` function - The event_trigger.sql tests are minimally modified. - A new file event_trigger_nosuper.sql is added for the new tests. Any feedback is welcomed. I'll also gladly modify the docs if the patch looks good. Best regards, Steve Chavez On Sat, 8 Mar 2025 at 00:03, Steve Chavez wrote: > Hi Tom, > > > Well, no. That still allows the database owner to commandeer any > > non-superuser role. Even if we tightened "nosuperuser" to mean > > "not superuser and not any built-in role", I don't think it will fly. > > Why would the predefined roles be taken into consideration here? The docs > on https://www.postgresql.org/docs/current/predefined-roles.html say: > > " pg_read_server_files, pg_write_server_files and > pg_execute_server_program..." > " ..they could be used to gain superuser-level access, therefore great > care should be taken when granting these roles to users." > > If a dbowner event trigger does `GRANT pg_read_server_files TO > current_user;` inside it will fail with `ERROR: permission denied to grant > role "pg_read_server_files"`. > The only way for that to succeed is for a superuser to explicitly grant > access to `pg_read_server_files` before, and that would have to be a > conscious operation. > > I would appreciate any clarification here. > > > maybe say that an event trigger fires for queries that are run by a role > - that the trigger's owning role is a member of? > > The role membership approach would work but it seems insufficient. For > example consider `pgaudit` which installs event triggers and requires > superuser. > > Let's assume `pgaudit` would try to adopt this new feature. Then it would > need to provide some special role like `pgaudit_admin`, create the event > triggers under this role, > and users of this extension would have to manually grant membership to > `pgaudit_admin` for the audit event triggers to fire. > That is a problem because that's easy to forget when creating new roles > and the audit event triggers won't be "enforced". > So in that case I guess `pgaudit` developers would keep requiring > superuser and not bother to adopt this new feature. > > From a PoLP perspective it would be a desirable side-effect of this > feature to stop requiring superuser for certain extensions too. > > Best regards, > Steve Chavez > > On Fri, 7 Mar 2025 at 15:19, Tom Lane wrote: > >> Steve Chavez writes: >> > This is why I thought the database owner is the right role to allow >> evtrig >> > creation since it won't need an explicit list of roles. >> >> > How about requiring explicit non-superuser execution for the database >> owner >> > evtrig? It would be like: >> > CREATE EVENT TRIGGER name ... FOR NOSUPERUSER; >> >> Well, no. That still allows the database owner to commandeer any >> non-superuser role. Even if we tightened "nosuperuser" to mean >> "not superuser and not any built-in role", I don't think it will fly. >> >> Here is the real problem: database owners are not specially >> privileged in Postgres. Yeah, they can drop their DB, but they >> don't have automatic permissions to mess with other people's >> objects inside the DB. (Much the same can be said of schema >> owners.) So any proposal that effectively gives DB owners >> such privileges is going to fail. I realize that some other >> DBMSes assign more privileges to schema or DB owners, but we >> don't and I don't think we're open to changing that. >> >> I think you need to be thinking of this in terms of "what sort >> of feature can we add that can be allowed to any SQL user?" >> The notion I proposed earlier that an event trigger only fires >> on queries executed by roles the trigger's owner belongs to >> is (AFAICS) safe to allow to anyone. If that's not good enough >> for your notion of what a DB owner should be able to do, the >> answer is to grant the DB owner membership in every role that >> uses her database. That's effectively what the feature
Re: Allow database owners to CREATE EVENT TRIGGER
Sorry, attached the output file.
From 2165513e858a5a6c7a620b1499b2f634c4f2ab44 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH] Allow regular users to create event triggers
---
src/backend/commands/event_trigger.c | 33 +++--
src/backend/utils/cache/evtcache.c| 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
.../expected/event_trigger_nosuper.out| 74 +++
src/test/regress/parallel_schedule| 4 +
src/test/regress/sql/event_trigger.sql| 11 +--
.../regress/sql/event_trigger_nosuper.sql | 65
8 files changed, 156 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..9dc813adb6a 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
NameStr(form->evtname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of event trigger \"%s\"",
- NameStr(form->evtname)),
- errhint("The owner of an event trigger must be a superuser.")));
-
form->evtowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
@@ -698,7 +678,7 @@ EventTriggerCommonSetup(Node *parsetree,
if (unfiltered || filter_event_trigger(tag, item))
{
/* We must plan to fire this trigger. */
- runlist = lappend_oid(runlist, item->fnoid);
+ runlist = lappend(runlist, item);
}
}
@@ -1084,11 +1064,16 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
foreach(lc, fn_oid_list)
{
LOCAL_FCINFO(fcinfo, 0);
- Oid fnoid = lfirst_oid(lc);
+ EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ Oid current_user = GetUserId();
+
+ if (!is_member_of_role_nosuper(current_user, item->owneroid)) {
+ continue;
+ }
- elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ elog(DEBUG1, "EventTriggerInvoke %u", item->fnoid);
/*
* We want each event trigger to be able to see the results of the
@@ -1102,7 +1087,7 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
CommandCounterIncrement();
/* Look up the function */
- fmgr_info(fnoid, &flinfo);
+ fmgr_info(item->fnoid, &flinfo);
/* Call the function, passing no arguments but setting a context. */
InitFunctionCallInfoData(*fcinfo, &flinfo, 0,
diff --git a/src/backend/utils/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index ce596bf5638..1dc9a864034 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -175,6 +175,7 @@ BuildEventTriggerCache(void)
item = palloc0(sizeof(EventTriggerCacheItem));
item->fnoid = form->evtfoid;
item->enabled = form->evtenabled;
+ item->owneroid = form->evtowner;
/* Decode and sort tags array. */
evttags = heap_getattr(tup, Anum_pg_event_trigger_evttags,
diff --git a/src/include/utils/evtcache.h b/src/include/utils/evtcache.h
index 9d9fcb8657b..0ecd6a54c5f 100644
--- a/src/include/utils/evtcache.h
+++ b/src/include/utils/evtcache.h
@@ -31,6 +31,7 @@ typedef struct
Oid fnoid; /* function to be called */
char enabled; /* as SESSION_REPLICATION_ROLE_* */
Bitmapset *tagset; /* command tags, or NULL if empty */
+ Oid owneroid; /* owner of the event trigger */
} EventTriggerCacheItem;
extern List *EventCacheLookup(EventTriggerEvent event);
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 7b2198eac6f..0ca16ec5e38 100644
--- a/src/test/regress
Re: Allow database owners to CREATE EVENT TRIGGER
> Also, this looks unconventional…
> EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
Just noticed the mistake there, I would have expected a compilation error.
New patch attached with the following change:
EventTriggerCacheItem *item = lfirst(lc);
On Sun, 20 Apr 2025 at 22:55, Steve Chavez wrote:
> Sorry, attached the output file.
>
>
From 2d0ee67eb7c2d005ca5011c2a9ae01933dc5dba4 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH] Allow regular users to create event triggers
---
src/backend/commands/event_trigger.c | 33 +++--
src/backend/utils/cache/evtcache.c| 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
.../expected/event_trigger_nosuper.out| 74 +++
src/test/regress/parallel_schedule| 4 +
src/test/regress/sql/event_trigger.sql| 11 +--
.../regress/sql/event_trigger_nosuper.sql | 65
8 files changed, 156 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..7ae0636f7c2 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
NameStr(form->evtname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of event trigger \"%s\"",
- NameStr(form->evtname)),
- errhint("The owner of an event trigger must be a superuser.")));
-
form->evtowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
@@ -698,7 +678,7 @@ EventTriggerCommonSetup(Node *parsetree,
if (unfiltered || filter_event_trigger(tag, item))
{
/* We must plan to fire this trigger. */
- runlist = lappend_oid(runlist, item->fnoid);
+ runlist = lappend(runlist, item);
}
}
@@ -1084,11 +1064,16 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
foreach(lc, fn_oid_list)
{
LOCAL_FCINFO(fcinfo, 0);
- Oid fnoid = lfirst_oid(lc);
+ EventTriggerCacheItem *item = lfirst(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ Oid current_user = GetUserId();
+
+ if (!is_member_of_role_nosuper(current_user, item->owneroid)) {
+ continue;
+ }
- elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ elog(DEBUG1, "EventTriggerInvoke %u", item->fnoid);
/*
* We want each event trigger to be able to see the results of the
@@ -1102,7 +1087,7 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
CommandCounterIncrement();
/* Look up the function */
- fmgr_info(fnoid, &flinfo);
+ fmgr_info(item->fnoid, &flinfo);
/* Call the function, passing no arguments but setting a context. */
InitFunctionCallInfoData(*fcinfo, &flinfo, 0,
diff --git a/src/backend/utils/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index ce596bf5638..1dc9a864034 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -175,6 +175,7 @@ BuildEventTriggerCache(void)
item = palloc0(sizeof(EventTriggerCacheItem));
item->fnoid = form->evtfoid;
item->enabled = form->evtenabled;
+ item->owneroid = form->evtowner;
/* Decode and sort tags array. */
evttags = heap_getattr(tup, Anum_pg_event_trigger_evttags,
diff --git a/src/include/utils/evtcache.h b/src/include/utils/evtcache.h
index 9d9fcb8657b..0ecd6a54c5f 100644
--- a/src/include/utils/evtcache.h
+++ b/src/include/utils/evtcache.h
@@ -31,6 +31,7 @@ typedef struct
Oid fnoid; /* function to be called */
char enabled; /* as SESSION_REPLICATION_ROLE_* */
Bitmapset *tagset; /* command tag
Re: Allow database owners to CREATE EVENT TRIGGER
> You have a bad drop table cleanup command, and I’d drop the entire alter
event trigger owner test.
My bad, removed the bad drop table and also removed the alter owner test.
> The other thing I’m wondering, but haven’t gotten around to testing, is
whether a role affected by the event trigger is able to just drop the
trigger given this implementation. I always get member/member-of dynamics
confused. Having member (possibly via set role…) trying to drop the
trigger would be good to prove that it isn’t allowed.
So this one is a problem. If we do `grant evtrig_owner to member` then
`member` will be able to drop the event trigger, which is no good. There
are 2 option to solve this:
1. Do `grant evtrig_owner to member with inherit false`, then `member` will
not be able to drop the event trigger. I've updated the tests to reflect
that.
2. `create role member noinherit`, which won't let `member` drop the event
trigger with a regular `grant evtrig_owner to member`. But this change is
more invasive towards existing roles.
That being said, it's not a good default behavior to let evtrigger targets
to drop the evtrigger. Should we enforce that only granting with inherit
false will make the role members targets of the evtrigger? Are there any
other options?
On Tue, 22 Apr 2025 at 20:18, David G. Johnston
wrote:
> On Tuesday, April 22, 2025, David G. Johnston
> wrote:
>
>> On Tuesday, April 22, 2025, David G. Johnston
>> wrote:
>>
>>> On Tuesday, April 22, 2025, Steve Chavez wrote:
>>>
>>>> > alter event trigger command which doesn’t need to be exercised here
>>>>
>>>> That part does need to be tested, I modified
>>>> `AlterEventTriggerOwner_internal` to allow altering owners to regular
>>>> users. Before it was only restricted to superusers.
>>>>
>>>
>> Ok. I missed this.
>>
>
> Sorry for the self-reply but this nagged at me.
>
> It’s probably not a big deal either way, but the prior test existed to
> ensure that a superuser couldn’t do something they are otherwise are always
> permitted to do - assign object to whomever they wish. So
> event_trigger.sql had a test that errored showing this anomaly. You moved
> the test and now are proving it doesn’t error. But it is not expected to
> error; and immediately above you already show that a non-superuser can be
> an owner. We don’t need a test to show a superuser demonstrating their
> normal abilities.
>
> IOW, select test cases around the feature as it is implemented now, not
> its history. A personal one-off test to ensure that no super-user
> prohibitions remained will suffice to make sure all such code that needed
> to be removed is gone.
>
> David J.
>
>
From e9e294a638d55458adb6d08c9d2ce22e0c41f267 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v3] Allow regular users to create event triggers
* fix bad drop table
* remove alter owner test
* add test for not allowing member to drop evtrig
---
src/backend/commands/event_trigger.c | 33 +++---
src/backend/utils/cache/evtcache.c| 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
.../expected/event_trigger_nosuper.out| 60 +++
src/test/regress/parallel_schedule| 4 ++
src/test/regress/sql/event_trigger.sql| 11 +---
.../regress/sql/event_trigger_nosuper.sql | 53
8 files changed, 130 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..7ae0636f7c2 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
Re: Allow database owners to CREATE EVENT TRIGGER
> alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified
`AlterEventTriggerOwner_internal` to allow altering owners to regular
users. Before it was only restricted to superusers.
> Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.
I've updated the tests accordingly. Please let me know if that's what you
had in mind.
Best regards,
Steve Chavez
On Sun, 20 Apr 2025 at 23:13, David G. Johnston
wrote:
> On Sunday, April 20, 2025, Steve Chavez wrote:
>
>> > Also, this looks unconventional…
>> > EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
>>
>> Just noticed the mistake there, I would have expected a compilation
>> error. New patch attached with the following change:
>>
>> EventTriggerCacheItem *item = lfirst(lc);
>>
>> On Sun, 20 Apr 2025 at 22:55, Steve Chavez wrote:
>>
>>> Sorry, attached the output file.
>>>
>>>
> You can remove role member_1 and trigger..1 and “create table foo” from
> the nosuper script without any loss of test coverage. Or member2 trigger2
> table_bar along with the alter event trigger command which doesn’t need to
> be exercised here. Ownership is all that matters. Whether come to
> directly or via alter.
>
> Actually, leave the other member around, but not granted ownership, and
> both create tables, to demonstrate that a non-superuser and non-owner is
> unaffected by the trigger.
>
> David J.
>
>
From e9a73cda8145a04b8375d21e37a6e713af1bed34 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v2] Allow regular users to create event triggers
---
src/backend/commands/event_trigger.c | 33 +++--
src/backend/utils/cache/evtcache.c| 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
.../expected/event_trigger_nosuper.out| 71 +++
src/test/regress/parallel_schedule| 4 ++
src/test/regress/sql/event_trigger.sql| 11 +--
.../regress/sql/event_trigger_nosuper.sql | 59 +++
8 files changed, 147 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..7ae0636f7c2 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
NameStr(form->evtname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of event trigger \"%s\"",
- NameStr(form->evtname)),
- errhint("The owner of an event trigger must be a superuser.")));
-
form->evtowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
@@ -698,7 +678,7 @@ EventTriggerCommonSetup(Node *parsetree,
if (unfiltered || filter_event_trigger(tag, item))
{
/* We must plan to fire this trigger. */
- runlist = lappend_oid(runlist, item->fnoid);
+ runlist = lappend(runlist, item);
}
}
@@ -1084,11 +1064,16 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
foreach(lc, fn_oid_list)
{
LOCAL_FCINFO(fcinfo, 0);
- Oid fnoid = lfirst_oid(lc);
+ EventTriggerCacheItem *item = lfirst(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ Oid current_user = GetUserId();
+
+ if (!is_member_of_role_nosuper(current_user, item->owneroid)) {
+ continue;
+ }
- elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ elog(DEBUG1, "EventTriggerInvoke %u", item-
Re: Allow database owners to CREATE EVENT TRIGGER
Hi Tom, > Well, no. That still allows the database owner to commandeer any > non-superuser role. Even if we tightened "nosuperuser" to mean > "not superuser and not any built-in role", I don't think it will fly. Why would the predefined roles be taken into consideration here? The docs on https://www.postgresql.org/docs/current/predefined-roles.html say: " pg_read_server_files, pg_write_server_files and pg_execute_server_program..." " ..they could be used to gain superuser-level access, therefore great care should be taken when granting these roles to users." If a dbowner event trigger does `GRANT pg_read_server_files TO current_user;` inside it will fail with `ERROR: permission denied to grant role "pg_read_server_files"`. The only way for that to succeed is for a superuser to explicitly grant access to `pg_read_server_files` before, and that would have to be a conscious operation. I would appreciate any clarification here. > maybe say that an event trigger fires for queries that are run by a role - that the trigger's owning role is a member of? The role membership approach would work but it seems insufficient. For example consider `pgaudit` which installs event triggers and requires superuser. Let's assume `pgaudit` would try to adopt this new feature. Then it would need to provide some special role like `pgaudit_admin`, create the event triggers under this role, and users of this extension would have to manually grant membership to `pgaudit_admin` for the audit event triggers to fire. That is a problem because that's easy to forget when creating new roles and the audit event triggers won't be "enforced". So in that case I guess `pgaudit` developers would keep requiring superuser and not bother to adopt this new feature. >From a PoLP perspective it would be a desirable side-effect of this feature to stop requiring superuser for certain extensions too. Best regards, Steve Chavez On Fri, 7 Mar 2025 at 15:19, Tom Lane wrote: > Steve Chavez writes: > > This is why I thought the database owner is the right role to allow > evtrig > > creation since it won't need an explicit list of roles. > > > How about requiring explicit non-superuser execution for the database > owner > > evtrig? It would be like: > > CREATE EVENT TRIGGER name ... FOR NOSUPERUSER; > > Well, no. That still allows the database owner to commandeer any > non-superuser role. Even if we tightened "nosuperuser" to mean > "not superuser and not any built-in role", I don't think it will fly. > > Here is the real problem: database owners are not specially > privileged in Postgres. Yeah, they can drop their DB, but they > don't have automatic permissions to mess with other people's > objects inside the DB. (Much the same can be said of schema > owners.) So any proposal that effectively gives DB owners > such privileges is going to fail. I realize that some other > DBMSes assign more privileges to schema or DB owners, but we > don't and I don't think we're open to changing that. > > I think you need to be thinking of this in terms of "what sort > of feature can we add that can be allowed to any SQL user?" > The notion I proposed earlier that an event trigger only fires > on queries executed by roles the trigger's owner belongs to > is (AFAICS) safe to allow to anyone. If that's not good enough > for your notion of what a DB owner should be able to do, the > answer is to grant the DB owner membership in every role that > uses her database. That's effectively what the feature you're > suggesting would do anyway. > > regards, tom lane >
Re: Allow database owners to CREATE EVENT TRIGGER
Many thanks for the feedback. > an attribute of the trigger and allow both superusers and non-superusers to create them. The above is the crux of the issue. Superuser evtrigs can target every role but non-superusers evtrigs must apply only to a restricted set of roles to avoid privilege escalation. With an explicit attribute, I guess the SQL syntax should be like: > Seems better to make “execute_for” an attribute of the trigger CREATE EVENT TRIGGER name ... FOR role1, role2; Now say a new role is created and has usage/create on this database and we want the evtrig to apply to it. We would need to manually update the list of roles, it won't be done automatically. That is a problem if, for example, we need to enforce an audit trail through event triggers. This is why I thought the database owner is the right role to allow evtrig creation since it won't need an explicit list of roles. How about requiring explicit non-superuser execution for the database owner evtrig? It would be like: CREATE EVENT TRIGGER name ... FOR NOSUPERUSER; I welcome any alternative ideas. Best regards, Steve Chavez On Wed, 5 Mar 2025 at 08:54, David G. Johnston wrote: > On Wednesday, March 5, 2025, Aleksander Alekseev > wrote: >> >> > Unlike superuser event triggers, which execute functions for every >> role, database owner event triggers are only executed for non-superusers. >> > > All this doesn't strike me as a great UI. >> > > Yeah. Seems better to make “execute_for” an attribute of the trigger and > allow both superusers and non-superusers to create them. Then enforce that > non-superusers must specify the more limited value. > > Though it would seem nice to be able to exclude the pseudo-admin roles > these service providers create as well. > > David J. > >
Allow database owners to CREATE EVENT TRIGGER
Hello hackers,
Currently PostgreSQL only allows creating event triggers for superusers,
this prevents usage on PostgreSQL service providers, which do not grant
superuser access.
This patch allows database owners to create event triggers, while
preventing privilege escalation.
Unlike superuser event triggers, which execute functions for every role,
database owner event triggers are only executed for non-superusers.
This is necessary to prevent privesc. i.e. a superuser tripping on an event
trigger containing an `ALTER ROLE dbowner SUPERUSER`.
For skipping dbowner event triggers for superusers:
- A restriction is added for superuser event triggers, the event trigger
function must be owned by a superuser.
+ While this is a breaking change, I think it's minor as the usual flow
is to "login as superuser" -> "create an evtrig function" -> "create the
evtrig". This is also proved by the existing tests, which barely change.
- A restriction is added for dbowner event triggers, the event trigger
function must not be owned by a superuser.
This way we can filter dbowner event trigger functions inside
`EventTriggerInvoke`.
Tests are included in the patch, I've added a dedicated regression file for
easier review. Only a couple of error messages of the existing event
trigger regression tests are changed.
Any feedback is welcomed. I haven't added docs yet but I'll gladly add them
if the community thinks this patch makes sense.
(Previous thread that also discussed allowing event triggers for
non-superusers:
https://www.postgresql.org/message-id/flat/81C10FFB-5ADC-4956-9337-FA248A4CC20D%40enterprisedb.com#77738d12b82c9a403ea2c56ed09949a3
)
Best regards,
Steve Chavez
From 84965d3ad70c4794f93473b939f5437c0d154826 Mon Sep 17 00:00:00 2001
From: steve-chavez
Date: Wed, 26 Feb 2025 20:17:36 -0500
Subject: [PATCH] Allow database owners to CREATE EVENT TRIGGER
---
src/backend/commands/dbcommands.c | 22
src/backend/commands/event_trigger.c | 38 --
src/backend/utils/cache/lsyscache.c | 23
src/include/commands/dbcommands.h | 1 +
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/event_trigger.out | 2 +-
src/test/regress/parallel_schedule| 2 +-
.../regress/sql/event_trigger_dbowner.sql | 117 ++
8 files changed, 197 insertions(+), 9 deletions(-)
create mode 100644 src/test/regress/sql/event_trigger_dbowner.sql
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 1753b289074..4cd31dc1d90 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -3201,6 +3201,28 @@ get_database_name(Oid dbid)
return result;
}
+/*
+ * get_database_owner - given a database OID, look up the owner OID
+ *
+ * If the owner is not found, it will return InvalidOid
+ */
+Oid
+get_database_owner(Oid dbid)
+{
+ HeapTupledbtuple;
+ Oid dbowner;
+
+ dbtuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(dbid));
+ if (HeapTupleIsValid(dbtuple))
+ {
+ dbowner = ((Form_pg_database) GETSTRUCT(dbtuple))->datdba;
+ ReleaseSysCache(dbtuple);
+ }
+ else
+ dbowner = InvalidOid;
+
+ return dbowner;
+}
/*
* While dropping a database the pg_database row is marked invalid, but the
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..62e7c7326f4 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -34,6 +34,7 @@
#include "catalog/pg_trigger.h"
#include "catalog/pg_ts_config.h"
#include "catalog/pg_type.h"
+#include "commands/dbcommands.h"
#include "commands/event_trigger.h"
#include "commands/extension.h"
#include "commands/trigger.h"
@@ -125,18 +126,17 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
Oid evtowner = GetUserId();
ListCell *lc;
List *tags = NULL;
+ bool is_database_owner = (evtowner == get_database_owner(MyDatabaseId));
+ bool owner_is_super = superuser_arg(evtowner);
+ bool function_is_owned_by_super;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
+ if (!owner_is_super && !is_database_owner){
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to create event trigger \"%s\"",
stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
+ errhint("Must be the database owner or superuser to create an event trigger.")));
+ }
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
@@ -200,6 +200,24 @@ CreateEventTrigger(Creat
Re: PG 18 beta1 release notes misses mention of pg_noreturn
Yes, otherwise I had to grep the commits in `git log` and find what's the correct way to use `pg_noreturn`. It's not a simple keyword replacement since the order has to change now: Old: void my_worker(Datum main_arg) pg_attribute_noreturn(); New: pg_noreturn void my_worker(Datum main_arg); I've added the link to the commit in the patch, which should be enough to infer the above. How can I get this committed? Is the hacker mailing list the right place for release notes correction? Best regards, Steve Chavez On Wed, 2 Jul 2025 at 11:24, Daniel Gustafsson wrote: > > On 2 Jul 2025, at 04:51, Steve Chavez wrote: > > > While updating an extension to support 18beta1, I stumbled on the > removal of `pg_attribute_noreturn()` in favor of `pg_noreturn`, which > wasn't mentioned in the release notes. > > That admittedly seems like something worth including since it's otherwise > not > documented anywhere. > > -- > Daniel Gustafsson > >
PG 18 beta1 release notes misses mention of pg_noreturn
Hello hackers, While updating an extension to support 18beta1, I stumbled on the removal of `pg_attribute_noreturn()` in favor of `pg_noreturn`, which wasn't mentioned in the release notes. I've attached a patch for fixing this. Best regards, Steve Chavez From 52c479141457ab9d7fee1a5ac8b3184a35c5b497 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Tue, 1 Jul 2025 21:46:06 -0500 Subject: [PATCH] doc PG 18 relnotes: mention of pg_noreturn --- doc/src/sgml/release-18.sgml | 7 +++ 1 file changed, 7 insertions(+) diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml index 9a6e4fb8d0e..43b4091cbdd 100644 --- a/doc/src/sgml/release-18.sgml +++ b/doc/src/sgml/release-18.sgml @@ -3163,6 +3163,13 @@ Author: Tom Lane Remove support for the HPPA/PA-RISC architecture (Tom Lane) § + + + + +pg_attribute_noreturn() was removed in favor of pg_noreturn (Peter Eisentraut) +§ + -- 2.40.1
Re: Allow database owners to CREATE EVENT TRIGGER
Isaac, > Can somebody remind me why triggers don't run as their owner in the first place? > It would make triggers way more useful, and eliminate the whole issue of trigger owners escalating to whomever tries to access the object on which the trigger is defined. Just noted this is already possible when marking the event trigger function as SECURITY DEFINER (instead of having the SECURITY INVOKER default), it will fire for every role but keeping the privilege of the event trigger creator. Seeing that we have a problem with membership-based event triggers, how about if we require that regular user event triggers can only have SECURITY DEFINER functions? We can enforce this at `create event trigger` time. Best regards, Steve Chavez On Wed, 5 Mar 2025 at 11:17, Isaac Morland wrote: > On Wed, 5 Mar 2025 at 10:28, Tom Lane wrote: > >> I wrote: >> > Or in other words: not-superuser to superuser is far from the only >> > type of privilege escalation that we need to prevent. >> >> After reflecting on that for a moment: maybe say that an event trigger >> fires for queries that are run by a role that the trigger's owning >> role is a member of? That changes nothing for superuser-owned >> triggers. >> > > Can somebody remind me why triggers don't run as their owner in the first > place? > > It would make triggers way more useful, and eliminate the whole issue of > trigger owners escalating to whomever tries to access the object on which > the trigger is defined. >
