Hi, Andrew
On Thu, 19 Mar 2026 at 14:34, Andrew Dunstan <[email protected]> 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_...@mail.gmail.com
>
> [3]
> https://www.postgresql.org/message-id/flat/[email protected]
>
> [4]
> https://www.postgresql.org/message-id/flat/CAKWEB6rmnmGKUA87Zmq-s=b3scsnj02c0kobqjnbl2ajfpw...@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.
