Re: pg_get__*_ddl consolidation

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 NOBYPASSRLS VALID 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/