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
>>
>
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
> 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: 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_SKIP_LINE; } +/* +
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
>
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);
alter domain testdomain1 rename constraint unsigned to unsigned_foo;
alter domain testdomain1 drop constraint unsigned_foo;
drop domain testdomain1;
+--
+-- Get the base type of a domain
+--
+create domain mytext as text;
+create domain mytext_child
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
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
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 >
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 >
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 >
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
'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
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=om=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
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, _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
>
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 *) , + 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: 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_rbtree.c
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. >
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(_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 *) );
+
+ 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 naturals greater than the search key */
+ fo
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, _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
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
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
