Re: [PATCH] Add pg_get_subscription_ddl() function

Hi,

Thank you for the review.
Please find the attached patch.

Regards,
Vaibhav
EnterpriseDB

On Tue, Nov 18, 2025 at 7:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:

> Some review comments for v6.
>
> ======
> src/backend/utils/adt/ruleutils.c
>
> 1.
> +/*
> + * pg_get_subscription_string
> + * Build CREATE SUBSCRIPTION statement for a subscription from its OID.
> + *
> + * This is internal version which helps pg_get_subscription_ddl_by_name()
> and
> + * pg_get_subscription_ddl_by_oid().
> + */
> +static char *
> +pg_get_subscription_string(const Oid suboid)
>
> The comment "This is internal" seemed awkward. Anyway, saying it is
> "internal" is unnecessary now that it is static.
>
> SUGGESTION
> Helper for pg_get_subscription_ddl_by_name() and
> pg_get_subscription_ddl_by_oid().
>
> ~~~
>
> 2.
> + /* Get enabled option */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subenabled);
> + /* Setting 'slot_name' to none must set 'enabled' to false as well */
> + appendStringInfo(&buf, ", enabled=%s",
> + (!DatumGetBool(datum) || isnull) ? "false" : "true");
> +
> + /* Get binary option */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subbinary);
> + appendStringInfo(&buf, ", binary=%s",
> + DatumGetBool(datum) ? "true" : "false");
> +
> + /* Get streaming option */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_substream);
> + if (DatumGetChar(datum) == LOGICALREP_STREAM_OFF)
> + appendStringInfoString(&buf, ", streaming=off");
> + else if (DatumGetChar(datum) == LOGICALREP_STREAM_ON)
> + appendStringInfoString(&buf, ", streaming=on");
> + else
> + appendStringInfoString(&buf, ", streaming=parallel");
> +
> + /* Get sync commit option */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subsynccommit);
> + appendStringInfo(&buf, ", synchronous_commit=%s",
> + TextDatumGetCString(datum));
> +
> + /* Get two-phase commit option */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subtwophasestate);
> + appendStringInfo(&buf, ", two_phase=%s",
> + DatumGetChar(datum) == LOGICALREP_TWOPHASE_STATE_DISABLED ? "off" :
> "on");
> +
> + /* Disable on error? */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subdisableonerr);
> + appendStringInfo(&buf, ", disable_on_error=%s",
> + DatumGetBool(datum) ? "on" : "off");
> +
> + /* Password required? */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subpasswordrequired);
> + appendStringInfo(&buf, ", password_required=%s",
> + DatumGetBool(datum) ? "on" : "off");
> +
> + /* Run as owner? */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subrunasowner);
> + appendStringInfo(&buf, ", run_as_owner=%s",
> + DatumGetBool(datum) ? "on" : "off");
> +
> + /* Get origin */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_suborigin);
> + appendStringInfo(&buf, ", origin=%s", TextDatumGetCString(datum));
> +
> + /* Failover? */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subfailover);
> + appendStringInfo(&buf, ", failover=%s",
> + DatumGetBool(datum) ? "on" : "off");
> +
> + /* Retain dead tuples? */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_subretaindeadtuples);
> + appendStringInfo(&buf, ", retain_dead_tuples=%s",
> + DatumGetBool(datum) ? "on" : "off");
> +
> + /* Max retention duration */
> + datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
> + Anum_pg_subscription_submaxretention);
> + appendStringInfo(&buf, ", max_retention_duration=%d",
> + DatumGetInt32(datum));
>
> 2a.
> It seems strange that almost everything is coded as "true/false" or
> "on/off", except there are a couple ('enabled' and 'two_phase') that
> are the other way around (false/true, off/on). There seemed to be no
> particular reason for that. IMO consistency is better, so use
> true/false for everything.
>
> ~
>
> 2b.
> It's not clear to me how you decided when to use true/false versus
> on/off. I know that it makes no functional difference, but it does
> have the potential to make the result look strange unless there is
> some consistent rule. This review comment would apply to *every* one
> of the get_XXX_ddl() functions.
>
> Personally, I think the DLL functions should only spit out
> "true"/"false" for boolean parameters. It is easy and it is
> predictable.
>
> FWIW, my AI tool agrees with me. viz:
> ------
> Why true/false for DDL:
> * Consistent with PostgreSQL's boolean literals in SQL
> * Matches what pg_dump produces
> * Clear and unambiguous
> * Parser accepts both, but true/false is standard output
> Don't use:
> * on/off in generated DDL (even though parser might accept it)
> * 1/0
> * yes/no
> * t/f (internal catalog representation)
> ------
>
> ~
>
> 2c.
> All those comments about the parameters should be consistent.
> Currently they are:
> * Get xxx
> * xxx?
> * xxx
>
> Choose one common style to make them all look the same.
>
> ~
>
> 2d.
> It's not clear what ordering was chosen for these parameters in the
> generated DDL. Unless there is some meaningful order that eludes me, I
> felt it would be best to generate them in the same order that they
> appear in the documentation [1].
>
> ~~~
>
> 3.
> + /* Finally close parenthesis and add semicolon to the statement */
> + appendStringInfoString(&buf, ");");
> +
>
> This comment seems unnecessary.
>
> ======
> src/test/regress/sql/subscription.sql
>
> 4.
> +-- see the pg_get_subscription_ddl output for a NULL and empty input
>
> /see/Check/
>
> ~~~
>
> 5.
> +-- Check that the subscription ddl is correctly created
>
> /ddl/DDL/
>
> ======
> [1] https://www.postgresql.org/docs/devel/sql-createsubscription.html
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
>