Re: pg_get__*_ddl consolidation

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.