On 27/01/26 19:27, Haritabh Gupta wrote:
> Hello,
> Please find below some comments (mostly minor ones):
>
> 1. We need to add the following comma in the docs change. so that it looks same as other functions:
> diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml
> index 25f87b78344..bc01c73f4ea 100644
> --- a/doc/src/sgml/func/func-info.sgml
> +++ b/doc/src/sgml/func/func-info.sgml
> @@ -3861,7 +3861,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
> <primary>pg_get_domain_ddl</primary>
> </indexterm>
> <function>pg_get_domain_ddl</function> ( <parameter>domain</parameter> <type>regtype</type>
> - <optional> <parameter>pretty</parameter> <type>boolean</type> </optional>)
> + <optional>, <parameter>pretty</parameter> <type>boolean</type> </optional>)
> <returnvalue>text</returnvalue>
> </para>
> <para>
>
>
> 2. In the function signature there is `int prettyFlags` argument, while the doc suggests `pretty`:
> +/*
> + * get_formatted_string
> + *
> + * Return a formatted version of the string.
> + *
> + * pretty - If pretty is true, the output includes tabs (\t) and newlines (\n).
> + * noOfTabChars - indent with specified no of tabs.
> + * fmt - printf-style format string used by appendStringInfoVA.
> + */
> +static void
> +get_formatted_string(StringInfo buf, int prettyFlags, int noOfTabChars, const char *fmt,...)
>
>
> 3. In a similar patch
(https://www.postgresql.org/message-id/flat/CANxoLDdJsRJqnjMXV3yjsk07Z5iRWxG-c2hZJC7bAKqf8ZXj_A@mail.gmail.com),author
hasdefined a separate macro to make the usage of `GET_PRETTY_FLAGS` cleaner, We can use the same in function
`pg_get_domain_ddl_ext`:
> +#define GET_DDL_PRETTY_FLAGS(pretty) \
> + ((pretty) ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) \
> + : 0)
>
> +Datum
> +pg_get_policy_ddl(PG_FUNCTION_ARGS)
> +{
> + Oid tableID = PG_GETARG_OID(0);
> + Name policyName = PG_GETARG_NAME(1);
> + bool pretty = PG_GETARG_BOOL(2);
> + int prettyFlags;
> + char *res;
> +
> + prettyFlags = GET_DDL_PRETTY_FLAGS(pretty);
Thanks for reviewing!
I addressed your first 3 suggestions and attached v6.
> 4. Usually the tests for the function to get the DDL definition of an object are present in the same testcase file
wherethe `CREATE...` command exists, e.g. test for `pg_get_indexdef` exists in `create_index.sql` file. Similarly tests
for`pg_get_functiondef` exists in `create_procedure.sql` file and so on. Currently in the patch, the tests for
`pg_get_domain_ddl`are put in a new file `object_ddl.sql` but I guess it can be put in the existing file `domain.sql`
becausethat is where the `CREATE DOMAIN...` tests reside.
For number 4 I think it's better to keep it in a separate file as this
is just one of the "get_object_ddl" functions, and in this `object_ddl`
file we can add more test also for other functions similar to this one.
What do you/others think?
Cheers,
Florin Irion
EDB -- www.enterprisedb.com
