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
