Обсуждение: pg_get__*_ddl consolidation
Greetings Euler Taveira and I have been working on consolidating these patches. These patches came out of a suggestion from me some time back [1], and I used it as the base for some work at an EDB internal program. Perhaps I was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a hundred schools of thought contend." I wanted to see what people would come up with. Therefore, if this has seemed a bit chaotic, I apologize, both to the authors and to the list. I won't do things quite this way in future. Rather than adding to the already huge ruleutils.c, we decided to create a new ddlutils.c file to contain these functions and their associated infrastructure. There is in fact a fairly clean separation between these functions and ruleutils. We just need to expose one function in ruleutils. We (Euler and I) decided to concentrate on setting up common infrastucture and ensuring a common argument and result structure. In this first round, we are proposing to add functions for getting the DDL for databases, tablespaces, and roles. We decided to stop there for now. This sets up a good basis for dealing with more object types in future. To the authors of the remaining patches - rest assured you have not been forgotten. Patch 1 sets up the functions used by the rest for option parsing. see [2] Patch 2 implements pg_get_role_dll see[3] Patch 3 implements pg_get_tabespace_ddl see [4] Patch 4 implements pg_get_database_ddl see [2] cheers andrew [1] https://www.postgresql.org/message-id/flat/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net [2] https://www.postgresql.org/message-id/flat/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com [3] https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com [4] https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com -- Andrew Dunstan EDB: https://www.enterprisedb.com
Вложения
On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan <andrew@dunslane.net> wrote: > > Greetings > > Euler Taveira and I have been working on consolidating these patches. > > These patches came out of a suggestion from me some time back [1], and I > used it as the base for some work at an EDB internal program. Perhaps I > was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a > hundred schools of thought contend." I wanted to see what people would > come up with. Therefore, if this has seemed a bit chaotic, I apologize, > both to the authors and to the list. I won't do things quite this way in > future. > > Rather than adding to the already huge ruleutils.c, we decided to create > a new ddlutils.c file to contain these functions and their associated > infrastructure. There is in fact a fairly clean separation between these > functions and ruleutils. We just need to expose one function in ruleutils. > > We (Euler and I) decided to concentrate on setting up common > infrastucture and ensuring a common argument and result structure. In > this first round, we are proposing to add functions for getting the DDL > for databases, tablespaces, and roles. We decided to stop there for now. > This sets up a good basis for dealing with more object types in future. > To the authors of the remaining patches - rest assured you have not been > forgotten. > > Patch 1 sets up the functions used by the rest for option parsing. see [2] > Patch 2 implements pg_get_role_dll see[3] > Patch 3 implements pg_get_tabespace_ddl see [4] > Patch 4 implements pg_get_database_ddl see [2] > > > cheers > > > andrew > > > [1] > https://www.postgresql.org/message-id/flat/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net > > [2] > https://www.postgresql.org/message-id/flat/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com > > [3] > https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com > > [4] > https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com Hi all, I was reading these patches and found that any user can get the definition of database/roles by pg_get__*_ddl. I think these functions should be restricted only to super users as these are cluster level objects. TAB is not working for these functions. I think these functions should be displayed with TAB. I will do some more study and will do some more tests. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
On 2026-03-19 Th 3:55 PM, Mahendra Singh Thalor wrote:
On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan <andrew@dunslane.net> wrote:Greetings Euler Taveira and I have been working on consolidating these patches. These patches came out of a suggestion from me some time back [1], and I used it as the base for some work at an EDB internal program. Perhaps I was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a hundred schools of thought contend." I wanted to see what people would come up with. Therefore, if this has seemed a bit chaotic, I apologize, both to the authors and to the list. I won't do things quite this way in future. Rather than adding to the already huge ruleutils.c, we decided to create a new ddlutils.c file to contain these functions and their associated infrastructure. There is in fact a fairly clean separation between these functions and ruleutils. We just need to expose one function in ruleutils. We (Euler and I) decided to concentrate on setting up common infrastucture and ensuring a common argument and result structure. In this first round, we are proposing to add functions for getting the DDL for databases, tablespaces, and roles. We decided to stop there for now. This sets up a good basis for dealing with more object types in future. To the authors of the remaining patches - rest assured you have not been forgotten. Patch 1 sets up the functions used by the rest for option parsing. see [2] Patch 2 implements pg_get_role_dll see[3] Patch 3 implements pg_get_tabespace_ddl see [4] Patch 4 implements pg_get_database_ddl see [2] cheers andrew [1] https://www.postgresql.org/message-id/flat/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net [2] https://www.postgresql.org/message-id/flat/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com [3] https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com [4] https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com -- Andrew Dunstan EDB: https://www.enterprisedb.comHi all, I was reading these patches and found that any user can get the definition of database/roles by pg_get__*_ddl. I think these functions should be restricted only to super users as these are cluster level objects.
You could construct these functions using plpgsql. The information isn't hidden from non-superusers. So what exactly would making these functions superuser-only achieve? Now it's true that the user might not be able to execute the DDL. But that's not the point.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Fri, 20 Mar 2026 at 01:44, Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 2026-03-19 Th 3:55 PM, Mahendra Singh Thalor wrote: > > On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan <andrew@dunslane.net> wrote: > > Greetings > > Euler Taveira and I have been working on consolidating these patches. > > These patches came out of a suggestion from me some time back [1], and I > used it as the base for some work at an EDB internal program. Perhaps I > was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a > hundred schools of thought contend." I wanted to see what people would > come up with. Therefore, if this has seemed a bit chaotic, I apologize, > both to the authors and to the list. I won't do things quite this way in > future. > > Rather than adding to the already huge ruleutils.c, we decided to create > a new ddlutils.c file to contain these functions and their associated > infrastructure. There is in fact a fairly clean separation between these > functions and ruleutils. We just need to expose one function in ruleutils. > > We (Euler and I) decided to concentrate on setting up common > infrastucture and ensuring a common argument and result structure. In > this first round, we are proposing to add functions for getting the DDL > for databases, tablespaces, and roles. We decided to stop there for now. > This sets up a good basis for dealing with more object types in future. > To the authors of the remaining patches - rest assured you have not been > forgotten. > > Patch 1 sets up the functions used by the rest for option parsing. see [2] > Patch 2 implements pg_get_role_dll see[3] > Patch 3 implements pg_get_tabespace_ddl see [4] > Patch 4 implements pg_get_database_ddl see [2] > > > cheers > > > andrew > > > [1] > https://www.postgresql.org/message-id/flat/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net > > [2] > https://www.postgresql.org/message-id/flat/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com > > [3] > https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com > > [4] > https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > > Hi all, > I was reading these patches and found that any user can get the > definition of database/roles by pg_get__*_ddl. I think these functions > should be restricted only to super users as these are cluster level > objects. > > > You could construct these functions using plpgsql. The information isn't hidden from non-superusers. So what exactly wouldmaking these functions superuser-only achieve? Now it's true that the user might not be able to execute the DDL. Butthat's not the point. Thanks Andrew for the clarification. > > > cheers > > > andrew > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com Sorry, my question might be stupid. Can we use these functions directly into our dump utilities so that common code can be removed and if there is any syntax change for a particular object, then no need to change into different-2 places, rather we just need to change it into ddlutils.c file only. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
On 2026-03-19 Th 4:28 PM, Mahendra Singh Thalor wrote: > On Fri, 20 Mar 2026 at 01:44, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> You could construct these functions using plpgsql. The information isn't hidden from non-superusers. So what exactly wouldmaking these functions superuser-only achieve? Now it's true that the user might not be able to execute the DDL. Butthat's not the point. > Thanks Andrew for the clarification. > > Sorry, my question might be stupid. Can we use these functions > directly into our dump utilities so that common code can be removed > and if there is any syntax change for a particular object, then no > need to change into different-2 places, rather we just need to change > it into ddlutils.c file only. Well, we have tried to make the output like pg_dump. We might later provide options for more compact output. But it remains to be seen if it can be used in the pg_dump utilities. After all, it is going to produce output suitable for the source version, since there is no knowledge in the server of future versions. But pg_dump emits SQL suitable for the target version. My original motivation was quite different. It was to give the user a piece of SQL that they could modify as they saw fit, rather than making them start with a blank canvas. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hello!
I found a few problematic corner cases while testing the patches,
please look at the following:
Doesn't pg_get_database_ddl need more filtering for roles?
See example:
CREATE DATABASE testdb;
CREATE ROLE testrole;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
SELECT pg_get_database_ddl('testdb');
Another issue is that the data style isn't fixed:
CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
SET DateStyle TO 'SQL, DMY';
SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
-- returned statement fails with invalid input syntax for timestamp
+ appendStringInfo(&buf, "ALTER DATABASE %s OWNER = %s;",
+ dbname, quote_identifier(owner));
Shouldn't that be OWNER TO? Similarly this will result in an error
when executed.
Role memberships seem to be missing. I would expect those to be included?
CREATE ROLE regress_parent;
CREATE ROLE regress_child;
GRANT regress_parent TO regress_child;
SELECT * FROM pg_get_role_ddl('regress_child');
+ dbname = quote_identifier(NameStr(dbform->datname));
Isn't an pstrdup missing from here? dbname is used after ReleaseSysCache.
On Thu, Mar 19, 2026, at 6:32 PM, Zsolt Parragi wrote:
>
> I found a few problematic corner cases while testing the patches,
> please look at the following:
>
Thanks for testing.
> Doesn't pg_get_database_ddl need more filtering for roles?
>
> See example:
>
> CREATE DATABASE testdb;
> CREATE ROLE testrole;
> ALTER DATABASE testdb SET work_mem TO '256MB';
> ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
> SELECT pg_get_database_ddl('testdb');
>
That's a design flaw. The goal is to returns SQL commands to reconstruct the
object. Since I don't know if you will use the testrole role to create testdb
database or even if the testrole exists in the cluster, it shouldn't return the
ALTER DATABASE testdb SET work_mem TO '512MB' (because that property belongs to
testrole role). It should only consider cases that setrole = 0 (as the comment
said). (This is not in the last patch [1] but the output of these pg_get_*_ddl
functions should be as close as possible to pg_dump(all) output. There is
another discussion [2] that asks how to test these functions; one way is to
compare the output with pg_dump(all). Another point is that these functions can
be used by a dump tool.)
> Another issue is that the data style isn't fixed:
>
> CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
> SET DateStyle TO 'SQL, DMY';
> SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
> -- returned statement fails with invalid input syntax for timestamp
>
I couldn't reproduce the failure. The non-fixed DateStyle is by design. It
mimics pg_dumpall.
$ psql -d postgres
psql (19devel)
Type "help" for help.
postgres=# CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
CREATE ROLE
postgres=# SHOW DateStyle;
DateStyle
-----------
ISO, DMY
(1 row)
postgres=# SET DateStyle TO 'SQL, DMY';
SET
postgres=# SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
pg_get_role_ddl
---------------------------------------------------------------------------------------------------------------------------------------------------------
CREATE ROLE regress_datestyle_test NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION NOBYPASSRLS VALID
UNTIL'31/12/2030 20:59:59 -03';
(1 row)
postgres=# CREATE ROLE regress_datestyle_test2 NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION
NOBYPASSRLSVALID UNTIL '31/12/2030 20:59:59 -03';
CREATE ROLE
postgres=# \du
List of roles
Role name | Attributes
-------------------------+------------------------------------------------------------
euler | Superuser, Create role, Create DB, Replication, Bypass RLS
regress_datestyle_test | Cannot login +
| Password valid until 31/12/2030 20:59:59 -03
regress_datestyle_test2 | Cannot login +
| Password valid until 31/12/2030 20:59:59 -03
>
> + appendStringInfo(&buf, "ALTER DATABASE %s OWNER = %s;",
> + dbname, quote_identifier(owner));
>
> Shouldn't that be OWNER TO? Similarly this will result in an error
> when executed.
>
Ooops. Let me get my brown paper bag.
> Role memberships seem to be missing. I would expect those to be included?
>
> CREATE ROLE regress_parent;
> CREATE ROLE regress_child;
> GRANT regress_parent TO regress_child;
> SELECT * FROM pg_get_role_ddl('regress_child');
>
That's a good suggestion. Using the same argument as the first question, you
don't know if regress_parent role exists in the other cluster. I'm not opposed
to your idea but IMO it should be an option.
> + dbname = quote_identifier(NameStr(dbform->datname));
>
> Isn't an pstrdup missing from here? dbname is used after ReleaseSysCache.
Yes. I missed this point while adding pg_db_role_setting code.
[1] https://postgr.es/m/CANxoLDfLDa6Oh9y=QtVoCd=03b9ydeFF6fUe8vR1wPYU7refBg@mail.gmail.com
[2] https://postgr.es/m/202603021736.6nix27wwg6e6@alvherre.pgsql
--
Euler Taveira
EDB https://www.enterprisedb.com/
On 2026-Mar-19, Andrew Dunstan wrote: > Greetings > > Euler Taveira and I have been working on consolidating these patches. Hmm, did you remove the permissions checking to dump objects? I thought we had concluded that these were needed -- ie. you have to have at least CONNECT to a database to be able to dump it, and so on. This way, the functions do not override a DBAs intention to hide the information, when they run REVOKE on the catalogs. I know this is a nonstandard thing to do, but some people do it nonetheless. https://postgr.es/m/202511131446.uzn4c25ljmd4@alvherre.pgsql -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Computing is too important to be left to men." (Karen Spärck Jones)
On 2026-03-20 Fr 9:15 AM, Álvaro Herrera wrote: > On 2026-Mar-19, Andrew Dunstan wrote: > >> Greetings >> >> Euler Taveira and I have been working on consolidating these patches. > Hmm, did you remove the permissions checking to dump objects? I thought > we had concluded that these were needed -- ie. you have to have at least > CONNECT to a database to be able to dump it, and so on. This way, the > functions do not override a DBAs intention to hide the information, when > they run REVOKE on the catalogs. I know this is a nonstandard thing to > do, but some people do it nonetheless. > > https://postgr.es/m/202511131446.uzn4c25ljmd4@alvherre.pgsql > Oh, hmm, yes, I think we did. Will work on it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, Andrew
On Thu, 19 Mar 2026 at 14:34, Andrew Dunstan <andrew@dunslane.net> wrote:
> Greetings
>
> Euler Taveira and I have been working on consolidating these patches.
>
> These patches came out of a suggestion from me some time back [1], and
> I used it as the base for some work at an EDB internal
> program. Perhaps I was motivated a bit by Mao's dictum "Let a hundred
> flowers bloom; let a hundred schools of thought contend." I wanted to
> see what people would come up with. Therefore, if this has seemed a
> bit chaotic, I apologize, both to the authors and to the list. I won't
> do things quite this way in future.
>
> Rather than adding to the already huge ruleutils.c, we decided to
> create a new ddlutils.c file to contain these functions and their
> associated infrastructure. There is in fact a fairly clean separation
> between these functions and ruleutils. We just need to expose one
> function in ruleutils.
>
> We (Euler and I) decided to concentrate on setting up common
> infrastucture and ensuring a common argument and result structure. In
> this first round, we are proposing to add functions for getting the
> DDL for databases, tablespaces, and roles. We decided to stop there
> for now. This sets up a good basis for dealing with more object types
> in future. To the authors of the remaining patches - rest assured you
> have not been forgotten.
>
> Patch 1 sets up the functions used by the rest for option parsing. see [2]
> Patch 2 implements pg_get_role_dll see[3]
> Patch 3 implements pg_get_tabespace_ddl see [4]
> Patch 4 implements pg_get_database_ddl see [2]
>
Thanks for updating the patches. Here are some initial comments.
Patch 1
=======
1.
+ DDL_OPT_INT
+} DdlOptType;
A trailing comma should be added to match our coding conventions.
2.
+ bool boolval; /* filled in for DDL_OPT_BOOL */
+ char *textval; /* filled in for DDL_OPT_TEXT (palloc'd) */
+ int intval; /* filled in for DDL_OPT_INT */
Is it feasible to use a union for these three fields?
3.
+append_ddl_option(StringInfo buf, bool pretty, int indent,
+ const char *fmt,...)
+{
+ va_list args;
IMO, limiting the variable scope to the for loop would be better.
Patch 2
=======
1.
+ foreach(lc, namelist)
+ {
+ char *curname = (char *) lfirst(lc);
+
+ appendStringInfoString(&buf, quote_literal_cstr(curname));
+ if (lnext(namelist, lc))
+ appendStringInfoString(&buf, ", ");
+ }
We can use a boolean variable, such as first, to avoid calling lnext(),
and then replace foreach() with foreach_ptr().
2.
+ if (funcctx->call_cntr < funcctx->max_calls)
+ {
+ char *stmt;
+
+ lc = list_nth_cell(statements, funcctx->call_cntr);
+ stmt = (char *) lfirst(lc);
+
+ SRF_RETURN_NEXT(funcctx, CStringGetTextDatum(stmt));
+ }
Could we use list_nth() in a similar manner to patch 3?
Patch 4
=======
Same as patch 2.
>
> cheers
>
>
> andrew
>
>
> [1]
> https://www.postgresql.org/message-id/flat/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net
>
> [2]
> https://www.postgresql.org/message-id/flat/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com
>
> [3]
> https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com
>
> [4]
> https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
> [2. text/x-patch; 0001-Add-DDL-option-parsing-infrastructure-for-pg_get_-_d.patch]...
>
> [3. text/x-patch; 0002-Add-pg_get_role_ddl-function.patch]...
>
> [4. text/x-patch; 0004-Add-pg_get_database_ddl-function.patch]...
>
> [5. text/x-patch; 0003-Add-pg_get_tablespace_ddl-function.patch]...
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
> I couldn't reproduce the failure. The non-fixed DateStyle is by design. It
> mimics pg_dumpall.
Sorry, I made a mistake while only copying the relevant parts of the
test case here. This is the correct test case:
SET DateStyle TO 'SQL, DMY'; -- change from default MDY to DMY
CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
DROP ROLE regress_datestyle_test;
SET DateStyle TO 'SQL, MDY'; -- go back to default, or open a new
connection to use the generated command
-- try to run statement generated by pg_get_role_ddl
I'm not saying that this is necessarily wrong, but perhaps it's worth
mentioning somewhere?
> Since I don't know if you will use the testrole role to create testdb
> database or even if the testrole exists in the cluster, it shouldn't return the
> ALTER DATABASE testdb SET work_mem TO '512MB' (because that property belongs to
> testrole role).
Sorry, I wasn't specific previously, it returns the role specific
role, without the role specific condition:
CREATE DATABASE testdb;
CREATE ROLE testrole;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
SELECT pg_get_database_ddl('testdb');
pg_get_database_ddl
-------------------------------------------------------------------------------------------------------------------
CREATE DATABASE testdb WITH TEMPLATE = template0 ENCODING = 'UTF8'
LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE testdb OWNER = dutow;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER DATABASE testdb SET work_mem TO '512MB';
> one way is to
> compare the output with pg_dump(all). Another point is that these functions can
> be used by a dump tool.)
I did my previous testing with this in mind.
Both the role specific database options and the role memberships are
related: I understand if you say that this is a design decision /
limitation. On the other hand if the goal is that users should be able
to replicate dump(all) with these functions, then we should have a
way to also get that information, either by providing two different
outputs, or by specific additional getter functions.
On Fri, Mar 20, 2026, at 10:31 AM, Andrew Dunstan wrote: > > Oh, hmm, yes, I think we did. Will work on it. > Here is a new patchset (v2) including all suggested changes in this thread. It fixes: * DDLOptType: comma in the last element; * union for boolval, textval, intval; * va_list in a restricted scope; * foreach_ptr + boolean in patches 0002 and 0004; * list_nth instead of list_nth_cell in patches 0002 and 0004; * OWNER = role typo. Add test; * use pstrdup for dbname; * output only database-specific GUCs; * add ACL_CONNECT check as the original patch. I removed the pg_read_all_stats case because it doesn't match the role description; However, I didn't include the suggestion to explain that pg_get_role_ddl is dependent on the DateStyle. I think it fits better in the CREATE ROLE [1] that does not mention it in the VALID UNTIL clause. I'm not opposed to the idea of adding a sentence to the function description but my suggestion is that this new sentence points to CREATE ROLE page. [1] https://www.postgresql.org/docs/current/sql-createrole.html -- Euler Taveira EDB https://www.enterprisedb.com/
Вложения
v2 is definitely better, I can confirm the improvements.
> including all suggested changes in this thread
But I want to point out that the permission check question, and the
role membership question is still open.
For the membership question
> I'm not opposed
> to your idea but IMO it should be an option.
I'm also fine with leaving it out, but then it should be a mentioned limitation.
For v2:
Shouldn't the tablespace function also support an owner option
similarly to the database function?
A pfree(nulls) and pfree(spcname) seem to be missing, all other
variables are freed properly.
+
+ /*
+ * Variables that are marked GUC_LIST_QUOTE were already fully
+ * quoted before they were put into the setconfig array. Break
+ * the list value apart and then quote the elements as string
+ * literals.
+ */
+ if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
+ {
This part seems to be duplicated between the two functions, maybe
could be a helper?
+ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';
Maybe it would be useful to also test the "SET search_path TO
myschema, public" variant, without quotes?
I also want to go back to the datestyle question one more time:
> The non-fixed DateStyle is by design. It mimics pg_dumpall.
Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
"pg_dumpall works this way" argument because pg_dump works
differently. Maybe either of those tools should also be fixed?
The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
applied to pg_dumpall, so it seems like an oversight to me?
> At present, dates are put into a dump in the format specified by the
> default datestyle. This is not portable between installations.
>
> This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the
> dates written into the dump will be restorable onto any database,
> regardless of how its default datestyle is set.
On 2026-03-24 Tu 5:56 PM, Zsolt Parragi wrote:
> v2 is definitely better, I can confirm the improvements.
>
>> including all suggested changes in this thread
> But I want to point out that the permission check question, and the
> role membership question is still open.
>
> For the membership question
>
>> I'm not opposed
>> to your idea but IMO it should be an option.
> I'm also fine with leaving it out, but then it should be a mentioned limitation.
>
> For v2:
>
> Shouldn't the tablespace function also support an owner option
> similarly to the database function?
>
> A pfree(nulls) and pfree(spcname) seem to be missing, all other
> variables are freed properly.
>
> +
> + /*
> + * Variables that are marked GUC_LIST_QUOTE were already fully
> + * quoted before they were put into the setconfig array. Break
> + * the list value apart and then quote the elements as string
> + * literals.
> + */
> + if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
> + {
>
> This part seems to be duplicated between the two functions, maybe
> could be a helper?
>
>
> +ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';
>
> Maybe it would be useful to also test the "SET search_path TO
> myschema, public" variant, without quotes?
>
>
> I also want to go back to the datestyle question one more time:
>
>> The non-fixed DateStyle is by design. It mimics pg_dumpall.
> Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
> ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
> "pg_dumpall works this way" argument because pg_dump works
> differently. Maybe either of those tools should also be fixed?
>
> The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
> applied to pg_dumpall, so it seems like an oversight to me?
>
>> At present, dates are put into a dump in the format specified by the
>> default datestyle. This is not portable between installations.
>>
>> This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the
>> dates written into the dump will be restorable onto any database,
>> regardless of how its default datestyle is set.
OK, I hope the attached set addresses all the outstanding issues. We're
using ISO dates, and there are appropriate permissions checks. There is
an option to dump role memberships.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Вложения
v3 looks good to me
Hi, Andrew
On Tue, 31 Mar 2026 at 15:42, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2026-03-24 Tu 5:56 PM, Zsolt Parragi wrote:
>> v2 is definitely better, I can confirm the improvements.
>>
>>> including all suggested changes in this thread
>> But I want to point out that the permission check question, and the
>> role membership question is still open.
>>
>> For the membership question
>>
>>> I'm not opposed
>>> to your idea but IMO it should be an option.
>> I'm also fine with leaving it out, but then it should be a mentioned limitation.
>>
>> For v2:
>>
>> Shouldn't the tablespace function also support an owner option
>> similarly to the database function?
>>
>> A pfree(nulls) and pfree(spcname) seem to be missing, all other
>> variables are freed properly.
>>
>> +
>> + /*
>> + * Variables that are marked GUC_LIST_QUOTE were already fully
>> + * quoted before they were put into the setconfig array. Break
>> + * the list value apart and then quote the elements as string
>> + * literals.
>> + */
>> + if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
>> + {
>>
>> This part seems to be duplicated between the two functions, maybe
>> could be a helper?
>>
>>
>> +ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';
>>
>> Maybe it would be useful to also test the "SET search_path TO
>> myschema, public" variant, without quotes?
>>
>>
>> I also want to go back to the datestyle question one more time:
>>
>>> The non-fixed DateStyle is by design. It mimics pg_dumpall.
>> Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
>> ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
>> "pg_dumpall works this way" argument because pg_dump works
>> differently. Maybe either of those tools should also be fixed?
>>
>> The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
>> applied to pg_dumpall, so it seems like an oversight to me?
>>
>>> At present, dates are put into a dump in the format specified by the
>>> default datestyle. This is not portable between installations.
>>>
>>> This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the
>>> dates written into the dump will be restorable onto any database,
>>> regardless of how its default datestyle is set.
>
>
> OK, I hope the attached set addresses all the outstanding
> issues. We're using ISO dates, and there are appropriate permissions
> checks. There is an option to dump role memberships.
>
>
Thanks for updating the v3 patches. Here are some comments.
v3-0001
=======
1.
+ List *namelist;
+ bool first = true;
+
+ /* Parse string into list of identifiers */
+ if (!SplitGUCList(rawval, ',', &namelist))
According to SplitGUCList()'s comment, the caller should call list_free() on
the returned list even on error. Should we also call list_free() on namelist?
v3-0003
=======
1.
+ /* Add OWNER clause */
+ if (!no_owner)
+ {
+ spcowner = GetUserNameFromId(tspForm->spcowner, false);
+ append_ddl_option(&buf, pretty, 4, "OWNER %s",
+ quote_identifier(spcowner));
+ pfree(spcowner);
+ }
The spcowner is only used within the if scope, so we can narrow its scope.
v3-0004
========
1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");
I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:
[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
-----------------------------------------------------------------------------------------------------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)
Is this working as expected?
It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
> [2. text/x-patch; v3-0001-Add-infrastructure-for-pg_get_-_ddl-functions.patch]...
>
> [3. text/x-patch; v3-0002-Add-pg_get_role_ddl-function.patch]...
>
> [4. text/x-patch; v3-0003-Add-pg_get_tablespace_ddl-function.patch]...
>
> [5. text/x-patch; v3-0004-Add-pg_get_database_ddl-function.patch]...
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:
v3-0004
========
1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");
I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:
[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
------------------------------------------------------------ ------------------------------ -----------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)
Is this working as expected?
It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.
There is no way or use in constructing the original template clause, though I agree it’s worth a comment. At the end of the day the catalog data that was found in the db01 database already exists in the db02 database when executing these DLL reconstruction functions against the existing db02 database. Taking nothing from the template is the correct behavior - hence template0.
David J.
On 2026-04-02 Th 9:35 AM, David G. Johnston wrote:
On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:
v3-0004
========
1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");
I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:
[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
------------------------------------------------------------ ------------------------------ -----------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)
Is this working as expected?
It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.There is no way or use in constructing the original template clause, though I agree it’s worth a comment. At the end of the day the catalog data that was found in the db01 database already exists in the db02 database when executing these DLL reconstruction functions against the existing db02 database. Taking nothing from the template is the correct behavior - hence template0.
OK, here's a v4.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Вложения
On 2026-04-02 Th 12:27 PM, Andrew Dunstan wrote:
On 2026-04-02 Th 9:35 AM, David G. Johnston wrote:On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:
v3-0004
========
1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");
I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:
[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
------------------------------------------------------------ ------------------------------ -----------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)
Is this working as expected?
It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.There is no way or use in constructing the original template clause, though I agree it’s worth a comment. At the end of the day the catalog data that was found in the db01 database already exists in the db02 database when executing these DLL reconstruction functions against the existing db02 database. Taking nothing from the template is the correct behavior - hence template0.
OK, here's a v4.
Pushed. I have moved the remaining get_*_ddl items to PG20-1
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2026-04-05 11:06:09 -0400, Andrew Dunstan wrote: > Pushed. I have moved the remaining get_*_ddl items to PG20-1 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2026-04-05%2015%3A04%3A04 diff -U3 /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out --- /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out 2026-04-05 11:04:08 +++ /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out 2026-04-05 11:05:57 @@ -22,6 +22,7 @@ CREATE DATABASE regress_database_ddl ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0 OWNER regress_datdba; +WARNING: databases created by regression test cases should have names including "regression" ALTER DATABASE regress_database_ddl CONNECTION_LIMIT 123; ALTER DATABASE regress_database_ddl SET random_page_cost = 2.0; ALTER ROLE regress_datdba IN DATABASE regress_database_ddl SET random_page_cost = 1.1; But do we really have to create a new database and a new tablespace for these? Database and tablespace creations are quite heavyweight operations. We already have an existing tablespace and an existing database as part of the regression tests. Couldn't you make do with those? Greetings, Anres
On Sun, 5 Apr 2026 at 17:06, Andrew Dunstan <andrew@dunslane.net> wrote:
> Pushed. I have moved the remaining get_*_ddl items to PG20-1
+1 on having this feature in general. But I'm not sure I understand
why it needs the whole bespoke string-based option parsing in the
first commit. Why not use named arguments for this, i.e. have the
usage syntax be:
SELECT * FROM pg_get_role_ddl('regress_role_ddl_test3', pretty => true);
Instead of the current:
SELECT * FROM pg_get_role_ddl('regress_role_ddl_test3', 'pretty', 'true');
Hi, On 2026-04-05 11:40:33 -0400, Andres Freund wrote: > On 2026-04-05 11:06:09 -0400, Andrew Dunstan wrote: > > Pushed. I have moved the remaining get_*_ddl items to PG20-1 > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2026-04-05%2015%3A04%3A04 > > diff -U3 /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out > --- /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out 2026-04-05 11:04:08 > +++ /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out 2026-04-05 11:05:57 > @@ -22,6 +22,7 @@ > CREATE DATABASE regress_database_ddl > ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0 > OWNER regress_datdba; > +WARNING: databases created by regression test cases should have names including "regression" > ALTER DATABASE regress_database_ddl CONNECTION_LIMIT 123; > ALTER DATABASE regress_database_ddl SET random_page_cost = 2.0; > ALTER ROLE regress_datdba IN DATABASE regress_database_ddl SET random_page_cost = 1.1; Pushed a fixup for this and the pgindent failure, as it doesn't seem like a great time to have CI/BF fail. It is pretty odd that the naming restrictions for databases (regression*) is different than for all the other object types... > But do we really have to create a new database and a new tablespace for these? > Database and tablespace creations are quite heavyweight operations. > > We already have an existing tablespace and an existing database as part of the > regression tests. Couldn't you make do with those? Didn't do anything about that. Greetings, Andres Freund
On 2026-04-05 Su 4:03 PM, Andres Freund wrote: > Hi, > > On 2026-04-05 11:40:33 -0400, Andres Freund wrote: >> On 2026-04-05 11:06:09 -0400, Andrew Dunstan wrote: >>> Pushed. I have moved the remaining get_*_ddl items to PG20-1 >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2026-04-05%2015%3A04%3A04 >> >> diff -U3 /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out >> --- /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out 2026-04-05 11:04:08 >> +++ /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out 2026-04-05 11:05:57 >> @@ -22,6 +22,7 @@ >> CREATE DATABASE regress_database_ddl >> ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0 >> OWNER regress_datdba; >> +WARNING: databases created by regression test cases should have names including "regression" >> ALTER DATABASE regress_database_ddl CONNECTION_LIMIT 123; >> ALTER DATABASE regress_database_ddl SET random_page_cost = 2.0; >> ALTER ROLE regress_datdba IN DATABASE regress_database_ddl SET random_page_cost = 1.1; > Pushed a fixup for this and the pgindent failure, as it doesn't seem like a > great time to have CI/BF fail. Thanks for that. I'm not sure how my test regime managed to miss either. I will work on that. > It is pretty odd that the naming restrictions for databases (regression*) is > different than for all the other object types... > Yeah. >> But do we really have to create a new database and a new tablespace for these? >> Database and tablespace creations are quite heavyweight operations. >> >> We already have an existing tablespace and an existing database as part of the >> regression tests. Couldn't you make do with those? > Didn't do anything about that. > Well, the trouble is that the database test runs a bunch of alter and revoke statements on the created database, that we probably don't want to persist on the existing regression database. I could see an argument for converting this to a TAP test that would only be run once, given our current very profligate running of the core regression suite. That goes doubly for the tablespace test, which could also probably use ALTER TABLESPACE instead of creating a bunch of tablespaces and then dropping them. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2026-04-05 Su 12:35 PM, Jelte Fennema-Nio wrote:
> On Sun, 5 Apr 2026 at 17:06, Andrew Dunstan <andrew@dunslane.net> wrote:
>> Pushed. I have moved the remaining get_*_ddl items to PG20-1
> +1 on having this feature in general. But I'm not sure I understand
> why it needs the whole bespoke string-based option parsing in the
> first commit. Why not use named arguments for this, i.e. have the
> usage syntax be:
>
> SELECT * FROM pg_get_role_ddl('regress_role_ddl_test3', pretty => true);
>
> Instead of the current:
>
> SELECT * FROM pg_get_role_ddl('regress_role_ddl_test3', 'pretty', 'true');
There was quite a deal of discussion around this mechanism. See Euler's
review at [1] and follow-up at [2] for the original discussion of the
VARIADIC option-parsing design and the use cases it was meant to
address. I'm prepared to revisit it is there's a strong consensus on the
point.
cheers
andrew
[1]
https://www.postgresql.org/message-id/4e60bcae-8222-4e1f-8e5b-d73b59c93304%40app.fastmail.com
[2]
https://www.postgresql.org/message-id/4c695e76-5ab7-449f-8060-76518dd41468%40app.fastmail.com
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Mon, 6 Apr 2026 at 13:55, Andrew Dunstan <andrew@dunslane.net> wrote: > There was quite a deal of discussion around this mechanism. See Euler's > review at [1] and follow-up at [2] for the original discussion of the > VARIADIC option-parsing design and the use cases it was meant to > address. I'm prepared to revisit it is there's a strong consensus on the > point. Thanks for those links. I had not seen that part of the discussion. But I only see an explanation of why these functions are configurable with optional key+value pairs in their arguments. I think that makes sense, and I totally agree that we should do that. The thing I'm questioning is whether we need a new way of providing key+value pairs as optional arguments to functions. IMO we already had a perfectly fine one. Introducing another adds complexity (both to the code and to the user) and I don't see any compelling reason to do so. Attached is a patch with roughly what I have in mind instead. By doing this we can also make the functinos STRICT, so that we don't have to worry about handling NULL values for the first argument. Afaict this named parameter approach only has benefits over the VARIADIC argument one. But if I'm wrong about that, please let me know.
Вложения
On Mon, Apr 6, 2026, at 12:24 PM, Jelte Fennema-Nio wrote: > The thing I'm questioning is whether we need a new way of providing > key+value pairs as optional arguments to functions. IMO we already had a > perfectly fine one. Introducing another adds complexity (both to the > code and to the user) and I don't see any compelling reason to do so. > I did the same question when reviewing one of these patches. My first reaction was if we want flexibility to cover various use cases and maintainability to add/deprecate new options, we need a mechanism to avoid breaking compatibility or even overloading the function (complexity). My natural choice was key-value pair arguments. It needs new support code (although we already use this style in some of the backend functions -- e.g. pg_logical_slot_*_changes()). > Attached is a patch with roughly what I have in mind instead. By doing > this we can also make the functinos STRICT, so that we don't have to > worry about handling NULL values for the first argument. > That's a good point. > Afaict this named parameter approach only has benefits over the VARIADIC > argument one. But if I'm wrong about that, please let me know. > I also consider your approach but decided not to use it. The argument against named arguments is that you cannot add new argument *without* a DEFAULT value; if you do, all existing functions will fail. You also need to create another function with a different list of arguments to support a new option. If we are fine with this restriction, your proposal seems ok to me. One point in favor of your proposal is that the named arguments seems more intuitive than the key-value pair arguments. The first impression is that interchanging key and value is harder to figure out that the named argument notation. Of course, documentation and some examples can help the user to write these function calls. That's not the first function that uses this key-value argument approach. -- Euler Taveira EDB https://www.enterprisedb.com/
On Mon, 6 Apr 2026 at 19:10, Euler Taveira <euler@eulerto.com> wrote: > although we already use this style > in some of the backend functions -- e.g. pg_logical_slot_*_changes()). Thanks for the additional context. I didn't know about pg_logical_slot_*_changes using this style. I searched the docs locally and cannot find any other functions that use this style. I think what makes pg_logical_slot_*_changes special, is that it passes these options to the plugin. The plugin can define any valid options, and postgres core cannot know what they are. I think this approach makes sense for those functions because of that, but the ddl functions don't pass the options to a plugin, so that argument does not apply here. > I also consider your approach but decided not to use it. The argument against > named arguments is that you cannot add new argument *without* a DEFAULT value; > if you do, all existing functions will fail. I'm not sure what kind of change you're referring to here. I don't understand how variadic options allow you to add a required argument to an existing function without breaking existing callers. Could you give a concrete example of a change that the VARIADIC allows, but the named arguments don't? > You also need to create another > function with a different list of arguments to support a new option. I don't understand this either. We often add new optional arguments to existing functions in a new major release. e.g. pg_start_backup got the exclusive argument in PG9.6. Or do you mean something else here?
On Mon, Apr 6, 2026, at 5:24 PM, Jelte Fennema-Nio wrote:
> On Mon, 6 Apr 2026 at 19:10, Euler Taveira <euler@eulerto.com> wrote:
>> although we already use this style
>> in some of the backend functions -- e.g. pg_logical_slot_*_changes()).
>
> Thanks for the additional context. I didn't know about
> pg_logical_slot_*_changes using this style. I searched the docs
> locally and cannot find any other functions that use this style. I
> think what makes pg_logical_slot_*_changes special, is that it passes
> these options to the plugin. The plugin can define any valid options,
> and postgres core cannot know what they are. I think this approach
> makes sense for those functions because of that, but the ddl functions
> don't pass the options to a plugin, so that argument does not apply
> here.
>
There are other functions. See pg_restore_extended_stats() [1] and related
functions. If you are looking for flexibility, this key-value pair arguments is
one of the ways to achieve it.
>> I also consider your approach but decided not to use it. The argument against
>> named arguments is that you cannot add new argument *without* a DEFAULT value;
>> if you do, all existing functions will fail.
>
> I'm not sure what kind of change you're referring to here. I don't
> understand how variadic options allow you to add a required argument
> to an existing function without breaking existing callers. Could you
> give a concrete example of a change that the VARIADIC allows, but the
> named arguments don't?
>
Indeed. My sentence was confused. I want to say that the regular argument
list is not as flexible as the VARIADIC argument. Once you have an argument
with DEFAULT, you cannot have a next argument *without* DEFAULT. For VARIADIC
arguments, this restriction does not exist; there is no need to change the
function signature. The argument manipulation (default value, non null)
happens inside the function.
postgres=# create function foo(arg1 int default 0, arg2 int) returns int as $$ begin return arg1 + arg2; end; $$
languageplpgsql;
ERROR: input parameters after one with a default value must also have defaults
LINE 1: create function foo(arg1 int default 0, arg2 int) returns in...
It means these functions cannot add a new required argument (without DEFAULT).
Unless you change the current order of the arguments and put arg2 argument
before arg1. Doing that you could silently break existing function calls (if
argument type is that same as the existing one).
postgres=# create function foo(arg1 int default 0, arg2 int default 0) returns int as $$ begin return arg1 + arg2; end;
$$language plpgsql;
CREATE FUNCTION
postgres=# select foo(arg1 => 5, arg2 => 8);
foo
-----
13
(1 row)
postgres=# select foo(5, 8);
foo
-----
13
(1 row)
postgres=# -- include new argument
postgres=# drop function foo(int, int);
DROP FUNCTION
postgres=# create function foo(arg3 int, arg1 int default 0, arg2 int default 0) returns int as $$ begin return (arg1 +
arg2)* arg3; end; $$ language plpgsql;
CREATE FUNCTION
postgres=# select foo(5, 8);
foo
-----
40
(1 row)
Of course, if you are using named arguments an error is emitted.
postgres=# select foo(arg1 => 5, arg2 => 8);
ERROR: function foo(arg1 => integer, arg2 => integer) does not exist
LINE 1: select foo(arg1 => 5, arg2 => 8);
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.
The VARIADIC argument forces you to always specify the argument name; that's a
good thing. The regular argument list requires you to remember the order of the
arguments (unless you are using named arguments).
It is just a few arguments for the current functions but I predict that
pg_get_table_dll may have a dozen of arguments. IMO the VARIADIC approach is
superior when you want several options. The function call is smaller in
comparison to your proposal. (Let's say you want to specify the last argument
value. Inform all the other default arguments plus the argument you want to
change. For VARIADIC, specify only the argument you want to change.)
>> You also need to create another
>> function with a different list of arguments to support a new option.
>
> I don't understand this either. We often add new optional arguments to
> existing functions in a new major release. e.g. pg_start_backup got
> the exclusive argument in PG9.6. Or do you mean something else here?
I meant modifying the pg_proc.dat every time a new argument is added.
[1] https://www.postgresql.org/docs/18/functions-admin.html#FUNCTIONS-ADMIN-STATSMOD
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Tue, 7 Apr 2026 at 05:44, Euler Taveira <euler@eulerto.com> wrote:
> There are other functions. See pg_restore_extended_stats() [1] and related
> functions. If you are looking for flexibility, this key-value pair arguments is
> one of the ways to achieve it.
Ah, I missed that one indeed my search didn't find it because it used
type "any" for the variadic. That makes the usage nicer imo than the one
used here, because you can actually give booleans to it (not only 'true'
& 'false' as strings).
Looking at those examples I foresee another big downside to the VARIADIC
approach. It's impossible for an SQL autoformatter to make the function
call look nice, because it does not know the two arguments are supposed
to be together. They will all be put on a single line. The only way to
get a nicely looking function call is hand-formatting the code.
> postgres=# create function foo(arg1 int default 0, arg2 int) returns int as $$ begin return arg1 + arg2; end; $$
languageplpgsql;
> ERROR: input parameters after one with a default value must also have defaults
> LINE 1: create function foo(arg1 int default 0, arg2 int) returns in...
You don't give an example how VARIADIC gives you the ability to make
that behave differently. But I guess you mean that:
SELECT foo();
would start erroring "with 'arg2' is required" and users would have to do
SELECT foo('arg2', 123);
There's a pretty simple way to get that same behaviour for the named
arguments approach though. Simply use DEFAULT NULL as the default for
arg2, and make it nonstrict. Then you can check for NULL in the
implementation and throw an error, just like you would do for the
VARIADIC version. With that
SELECT foo(); --errors
SELECT foo(arg2 => 123) -- works
So I don't see how this VARIADIC differs in this case.
In any case it seems unlikely to me that we want to ever add new
required arguments to these functions. Simply for backwards
compatibility reasons that sounds like a huge hassle that we'll probably
want to avoid by giving any new arguments a default. So even if there
was a difference, I don't really consider that a useful benefit of the
VARIADIC approach.
> The VARIADIC argument forces you to always specify the argument name; that's a
> good thing. The regular argument list requires you to remember the order of the
> arguments (unless you are using named arguments).
I definitely agree with this. But I think that's solvable in practice by
having examples in the docs showing how to use named arguments for these
functions (see attached v6). That way most users will use that named
argument syntax as opposed to the positional one.
> It is just a few arguments for the current functions but I predict that
> pg_get_table_dll may have a dozen of arguments. IMO the VARIADIC approach is
> superior when you want several options. The function call is smaller in
> comparison to your proposal. (Let's say you want to specify the last argument
> value. Inform all the other default arguments plus the argument you want to
> change. For VARIADIC, specify only the argument you want to change.)
If you use named arguments to call the function, then all of this
doesn't matter. And actually the VARIADIC can be more confusing.
Especially with many arguments because it can be unclear which of the
arguments is a key and which one is a value.
And even for few arguments a reader can be confused, if the reader
doesn't realize that the arguments are interpreted as key value pairs.
For instance, I'd say that for a call like below, it's not obvious that
'foreign_keys' and 'all' are a pair. I'd have to look at the function
docs to realize that these are not two separate arguments (one set to
'foreign_keys', and the other to 'all'):
select pg_get_table_ddl('mytable', 'foreign_keys', 'all');
while with the named argument syntax makes that's immediately clearer:
select pg_get_table_ddl('mytable', foreign_keys => 'all');
> I meant modifying the pg_proc.dat every time a new argument is added.
Sure, but I don't understand why that would be problem. We do that all
the time in major releases. Even with the VARIADIC approach, I don't
think we should be adding optional arguments in minor releases.
So to summarize (from my biased viewpoint) I think the downsides are:
1. Uncommon calling convention: only pg_restore_*_stats and
pg_logical_slot_*_changes use it, while all other functions support
named parameters.
2. Needs custom option parsing logic
3. More characters to type because you have to quote booleans, integers
and argument names.
4. Requires functions to be marked as NOSTRICT, which then needs
additional NULL handling
5. It can be unclear to a reader of a query that the function arguments
should be interpreted as key-value pair
6. Breaks auto formatting
And the benefit:
1. Forces people to specify the argument name
I don't think those benefits outweigh the downsides.
Вложения
On Sun, 2026-04-05 at 11:06 -0400, Andrew Dunstan wrote:
> Pushed. I have moved the remaining get_*_ddl items to PG20-1
The line:
role_settings = DatumGetArrayTypeP(datum);
should be DatumGetArrayTypePCopy(), because it's being pfree()d later.
The existing code will sometimes make a copy and sometimes not, e.g.:
-- settings are contrived to make the datum inline
CREATE USER u1;
ALTER ROLE u1 SET search_path = 'public, pg_catalog, pg_temp';
ALTER ROLE u1 SET work_mem='64MB';
ALTER ROLE u1 SET statement_timeout='30s';
ALTER ROLE u1 SET lock_timeout='10s';
ALTER ROLE u1 SET idle_in_transaction_session_timeout = '60s';
SELECT pg_get_role_ddl('u1');
ERROR: pfree called with invalid pointer 0x7986dd0c7cc8 (header
0x0000400600000000)
Also, it looks like the scan key in pg_get_role_ddl_internal() uses
only the second attribute of the index, which might be worth a comment.
Regards,
Jeff Davis
Hi,
On Fri, Apr 10, 2026 at 1:03 PM Jeff Davis <pgsql@j-davis.com> wrote:
On Sun, 2026-04-05 at 11:06 -0400, Andrew Dunstan wrote:
> Pushed. I have moved the remaining get_*_ddl items to PG20-1
The line:
role_settings = DatumGetArrayTypeP(datum);
should be DatumGetArrayTypePCopy(), because it's being pfree()d later.
The existing code will sometimes make a copy and sometimes not, e.g.:
-- settings are contrived to make the datum inline
CREATE USER u1;
ALTER ROLE u1 SET search_path = 'public, pg_catalog, pg_temp';
ALTER ROLE u1 SET work_mem='64MB';
ALTER ROLE u1 SET statement_timeout='30s';
ALTER ROLE u1 SET lock_timeout='10s';
ALTER ROLE u1 SET idle_in_transaction_session_timeout = '60s';
SELECT pg_get_role_ddl('u1');
ERROR: pfree called with invalid pointer 0x7986dd0c7cc8 (header
0x0000400600000000)
Yes, it appears to be a bug. Attached a patch to fix this. Tested with the
attached patch and don't see server crashing after that.
postgres=# CREATE DATABASE crashtest TEMPLATE template0 LC_COLLATE 'C' LC_CTYPE 'C';
ALTER DATABASE crashtest SET search_path = 'public, pg_catalog';
ALTER DATABASE crashtest SET work_mem = '64MB';
ALTER DATABASE crashtest SET statement_timeout = '30s';
ALTER DATABASE crashtest SET random_page_cost = 1.5;
SELECT pg_get_database_ddl('crashtest');
CREATE DATABASE
ALTER DATABASE
ALTER DATABASE
ALTER DATABASE
ALTER DATABASE
pg_get_database_ddl
------------------------------------------------------------------------------------------------------------
CREATE DATABASE crashtest WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'C';
ALTER DATABASE crashtest OWNER TO azureuser;
ALTER DATABASE crashtest SET search_path TO 'public, pg_catalog';
ALTER DATABASE crashtest SET work_mem TO '64MB';
ALTER DATABASE crashtest SET statement_timeout TO '30s';
ALTER DATABASE crashtest SET random_page_cost TO '1.5';
(6 rows)
ALTER DATABASE crashtest SET search_path = 'public, pg_catalog';
ALTER DATABASE crashtest SET work_mem = '64MB';
ALTER DATABASE crashtest SET statement_timeout = '30s';
ALTER DATABASE crashtest SET random_page_cost = 1.5;
SELECT pg_get_database_ddl('crashtest');
CREATE DATABASE
ALTER DATABASE
ALTER DATABASE
ALTER DATABASE
ALTER DATABASE
pg_get_database_ddl
------------------------------------------------------------------------------------------------------------
CREATE DATABASE crashtest WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'C';
ALTER DATABASE crashtest OWNER TO azureuser;
ALTER DATABASE crashtest SET search_path TO 'public, pg_catalog';
ALTER DATABASE crashtest SET work_mem TO '64MB';
ALTER DATABASE crashtest SET statement_timeout TO '30s';
ALTER DATABASE crashtest SET random_page_cost TO '1.5';
(6 rows)
Thanks,
Satya
Вложения
On 2026-04-13 Mo 3:41 PM, SATYANARAYANA NARLAPURAM wrote:
Hi,On Fri, Apr 10, 2026 at 1:03 PM Jeff Davis <pgsql@j-davis.com> wrote:On Sun, 2026-04-05 at 11:06 -0400, Andrew Dunstan wrote:
> Pushed. I have moved the remaining get_*_ddl items to PG20-1
The line:
role_settings = DatumGetArrayTypeP(datum);
should be DatumGetArrayTypePCopy(), because it's being pfree()d later.
The existing code will sometimes make a copy and sometimes not, e.g.:
-- settings are contrived to make the datum inline
CREATE USER u1;
ALTER ROLE u1 SET search_path = 'public, pg_catalog, pg_temp';
ALTER ROLE u1 SET work_mem='64MB';
ALTER ROLE u1 SET statement_timeout='30s';
ALTER ROLE u1 SET lock_timeout='10s';
ALTER ROLE u1 SET idle_in_transaction_session_timeout = '60s';
SELECT pg_get_role_ddl('u1');
ERROR: pfree called with invalid pointer 0x7986dd0c7cc8 (header
0x0000400600000000)Yes, it appears to be a bug. Attached a patch to fix this. Tested with theattached patch and don't see server crashing after that.
Thanks, pushed.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2026-04-06 Mo 7:39 AM, Andrew Dunstan wrote: > > On 2026-04-05 Su 4:03 PM, Andres Freund wrote: > > >>> But do we really have to create a new database and a new tablespace >>> for these? >>> Database and tablespace creations are quite heavyweight operations. >>> >>> We already have an existing tablespace and an existing database as >>> part of the >>> regression tests. Couldn't you make do with those? >> Didn't do anything about that. >> > > Well, the trouble is that the database test runs a bunch of alter and > revoke statements on the created database, that we probably don't want > to persist on the existing regression database. I could see an > argument for converting this to a TAP test that would only be run > once, given our current very profligate running of the core regression > suite. That goes doubly for the tablespace test, which could also > probably use ALTER TABLESPACE instead of creating a bunch of > tablespaces and then dropping them. > > > Here's a patch that converts all these into a single TAP test, and reduces the number of tablespace creations. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com