Re: pg_get__*_ddl consolidation

Hi, Andrew

On Thu, 19 Mar 2026 at 14:34, Andrew Dunstan <andrew(at)dunslane(dot)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(at)mail(dot)gmail(dot)com
>
> [3]
> https://www.postgresql.org/message-id/flat/4c5f895e-3281-48f8-b943-9228b7da6471(at)gmail(dot)com
>
> [4]
> https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw(at)mail(dot)gmail(dot)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.