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 <[email protected]> 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 >
v7-Add-pg_get_subscription_ddl-function.patch
Description: Binary data
