I have updated the documentation. Attached is the *v12 patch*, now ready for further review.
On Fri, Mar 6, 2026 at 1:07 PM Rafia Sabih <[email protected]> wrote: > > > On Thu, 5 Mar 2026 at 08:16, Akshay Joshi <[email protected]> > wrote: > >> >> >> On Thu, Mar 5, 2026 at 8:15 PM Rafia Sabih <[email protected]> >> wrote: >> >>> I like the idea for this patch and find this useful. >>> >>> I have few comments for the patch, looking at the test cases added in >>> the patch, it is not clear to me what is the default value for each of the >>> options. For example, in >>> >> >> Following the postgresql documentation, I created ddl_defaults.h to >> handle default options. This will be used across all database objects in >> future pg_get_<object>_ddl patches. >> >> >>> +-- With No Tablespace >>> +SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'defaults', >>> true, 'tablespace', false)); >>> + >>> ddl_filter >>> >>> >>> +---------------------------------------------------------------------------------------------------------------------------------------------------------- >>> + CREATE DATABASE regression_utf8 WITH OWNER = regress_datdba_after >>> ENCODING = 'UTF8' ALLOW_CONNECTIONS = true CONNECTION LIMIT = 123 >>> IS_TEMPLATE = false; >>> +(1 row) >>> + >>> owner option is omitted but the output contains the owner information. >>> So does it mean that the default value for each option is true? >>> But that doesn't seem so, because omitting pretty option, doesn't give >>> output in pretty, rather pretty is used only when provided. >>> I find this rather confusing, it is worth documenting what is the >>> default value for each of these options. >>> >> >> In my view, every database has an owner; it's a required attribute, >> not an optional one with a "default" value. The OWNER clause is correctly >> controlled only by the PG_DDL_NO_OWNER flag. Users must explicitly provide >> (*'owner', >> false/'no'/'off')* to opt out of including ownership info in the >> DDL. When comparing the Owner and Pretty options: by default, the Owner is >> included in the DDL reconstruction, whereas the Pretty option is set to >> false by default. >> >> Right, I think this needs to be added in the documentation as part of > this patch. Basically, here > <para> > + <literal>'pretty', true</literal> (or <literal>'pretty', > 'yes'</literal>): > + Adds newlines and indentation for better readability. > + </para> > it needs to have a line more saying, by default this option is off. > Similarly for other options, like for the owner it is by default true, etc. > >> In my initial patches, I used flags like --no-owner and >> --no-tablespace because they are very intuitive for users. However, >> reviewers noted that this style is unique to pg_dump and not used >> elsewhere. Consequently, I switched to the existing name-value pair format >> using VARIADIC arguments. >> >>> >>> Similarly, what is the expected behaviour when defaults option is not >>> provided, >>> +-- With No Owner >>> +SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'owner', >>> false)); >>> + ddl_filter >>> +-------------------------------------------------------------- >>> + CREATE DATABASE regression_utf8 WITH CONNECTION LIMIT = 123; >>> +(1 row) >>> + >>> +-- With No Tablespace >>> +SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'defaults', >>> true, 'tablespace', false)); >>> + >>> ddl_filter >>> >>> >>> +---------------------------------------------------------------------------------------------------------------------------------------------------------- >>> + CREATE DATABASE regression_utf8 WITH OWNER = regress_datdba_after >>> ENCODING = 'UTF8' ALLOW_CONNECTIONS = true CONNECTION LIMIT = 123 >>> IS_TEMPLATE = false; >>> +(1 row) >>> Why is connection_limit present in both of these cases...? >>> >>> Another thing, what is the significance of having defaults option, >>> because I think that knowing non-default values could be more useful, or >>> atleast there should be a way to know the non-default options for the >>> database. >>> >> >> The defaults option is false by default; if a user does not specify >> it, the reconstructed DDL only includes non-default parameters. In my >> opinion, we should retain this flag for users who wish to see all available >> parameters for easier manual editing later. While simple database objects >> have fewer parameters, this flag is essential for complex TABLE or FUNCTION >> syntax where the parameter list is extensive. >> > This information should also be included in the documentation. > >> >> >>> Also, the option strategy is missing in the output, is it deliberate? If >>> yes, why? >>> >> >>> On Thu, 5 Mar 2026 at 01:50, Akshay Joshi <[email protected]> >>> wrote: >>> >>>> I’ve addressed all of your comments. >>>> >>>> Attached is the *v11 patch*, now ready for further review. >>>> >>>> On Wed, Mar 4, 2026 at 7:31 PM Japin Li <[email protected]> wrote: >>>> >>>>> On Wed, 04 Mar 2026 at 18:29, Akshay Joshi < >>>>> [email protected]> wrote: >>>>> > Thanks for the review, Japin. I’ve addressed all of your comments. I >>>>> also added a check to throw an error if an option >>>>> > appears more than once. >>>>> > >>>>> > Attached is the v10 patch, now ready for further review. >>>>> > >>>>> >>>>> Thanks for updating the patch. Here are some comments on v10. >>>>> >>>>> 1. >>>>> + * db_oid - OID/Name of the database for which to generate the DDL. >>>>> >>>>> Should the comment be updated? The code only accepts an OID for >>>>> `db_oid`, >>>>> database names are not supported. >>>>> >>>>> 2. >>>>> + /* Set the OWNER in the DDL if owner is not omitted */ >>>>> + if (OidIsValid(dbform->datdba) && !(ddl_flags & >>>>> PG_DDL_NO_OWNER)) >>>>> + { >>>>> + get_formatted_string(&buf, pretty_flags, 8, "OWNER = >>>>> %s", >>>>> + >>>>> quote_identifier(dbowner)); >>>>> + } >>>>> >>>>> `dbowner` is only needed inside this `if` — how about declaring it >>>>> there to >>>>> reduce its scope? >>>>> >>>>> 3. >>>>> + /* If is_with_defaults is true, then we skip default >>>>> encoding check */ >>>>> + if (is_with_defaults || >>>>> + >>>>> (pg_strcasecmp(pg_encoding_to_char(dbform->encoding), >>>>> + >>>>> DDL_DEFAULTS.DATABASE.ENCODING) != 0)) >>>>> + { >>>>> + get_formatted_string(&buf, pretty_flags, 8, >>>>> "ENCODING = %s", >>>>> + >>>>> quote_literal_cstr( >>>>> + >>>>> pg_encoding_to_char(dbform->encoding))); >>>>> + } >>>>> >>>>> How about cache the result of `pg_encoding_to_char()` in a local >>>>> variable to >>>>> avoid calling it twice? >>>>> >>>>> -- >>>>> Regards, >>>>> Japin Li >>>>> ChengDu WenWu Information Technology Co., Ltd. >>>>> >>>> >>> >>> -- >>> Regards, >>> Rafia Sabih >>> CYBERTEC PostgreSQL International GmbH >>> >> > > -- > Regards, > Rafia Sabih > CYBERTEC PostgreSQL International GmbH >
v12-0001-Add-pg_get_database_ddl-function-to-reconstruct-ddl.patch
Description: Binary data
