Обсуждение: 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.