Re: [PATCH] Add pg_get_subscription_ddl() function

Hi Hackers,

Thank you Chao Li, Peter Smith and Alvaro for the review.
I have incorporated all your review comments except below ones:

7
>
> ```
> + /* Append connection info to the CREATE SUBSCRIPTION statement */
> + appendStringInfo(&buf, "CONNECTION \'%s\'", conninfo);
> ```
> A connection string contains a db access credential, and
> ROLE_PG_READ_ALL_DATA (not a high privilege) can view the DDL, is there a
> concern of leaking the secret? Should we redact the password in connection
> string?
>
> If the user installs a password in the conninfo, I think they are being
dumb about it,
and it's not this function's job to educate them on that. Restricting the
function to
users that have the pg_read_all_data and/or pg_create_subscription privilege
(which applies to superusers, but also if the DBA grants that to other
users,
it'd work for those also) is a better idea.

> 1. Question - is it deliberate to *always* return DLL with every
>
> > possible option assigned, even if those are just the option default
> > values? e.g. For something like the CREATE PUBLICATION command the
> > string returned could be only half the size if it accounts for
> > default.
> Yeah, I was asking myself the same. I think we definitely want options
> to be printed when there are GUCs that can affect the outcome (e.g.,
> something that is considered default in this server but not on a
> differently- configured one would give different results). But for
> those that are just hardcoded defaults, omitting them would make sense.
>
> In future, the default value of any of the parameters may change so this
function
would create the wrong ddl. Also, having lengthy DDL doesn't create any
problem
and provides values for all the options.

> 2. I was also wondering if it was really necessary to have so many
>
> > appendStringInfoString() calls to reconstruct the command.
> There are a couple of these patches that have an auxiliary
> pretty-printing helper function to add newline-tabs instead of
> individual spaces. I think that wouldn't work as nicely if you tried to
> condense the printing in the way you suggest. On the other hand, if you
> have a long format string, it's harder to visually match each specifier
> to its corresponding argument. If this was performance-critical code I
> would agree to use denser code and avoid function calls, but for this
> usage I don't think we care much.
>
> +1.

Please find a revised patch.

Thanks,
Vaibhav Dalvi
EnterpriseDB

On Thu, Nov 13, 2025 at 1:50 PM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:

> On 2025-Nov-13, Peter Smith wrote:
>
> > 1. Question - is it deliberate to *always* return DLL with every
> > possible option assigned, even if those are just the option default
> > values? e.g. For something like the CREATE PUBLICATION command the
> > string returned could be only half the size if it accounts for
> > default.
>
> Yeah, I was asking myself the same. I think we definitely want options
> to be printed when there are GUCs that can affect the outcome (e.g.,
> something that is considered default in this server but not on a
> differently- configured one would give different results). But for
> those that are just hardcoded defaults, omitting them would make sense.
>
> > 2. I was also wondering if it was really necessary to have so many
> > appendStringInfoString() calls to reconstruct the command.
>
> There are a couple of these patches that have an auxiliary
> pretty-printing helper function to add newline-tabs instead of
> individual spaces. I think that wouldn't work as nicely if you tried to
> condense the printing in the way you suggest. On the other hand, if you
> have a long format string, it's harder to visually match each specifier
> to its corresponding argument. If this was performance-critical code I
> would agree to use denser code and avoid function calls, but for this
> usage I don't think we care much.
>
> --
> Álvaro Herrera Breisgau, Deutschland —
> https://www.EnterpriseDB.com/
> Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
> No dijo "Hello New Jersey\n", ni "Hello USA\n".
>