Re: [PATCH] Add pg_get_subscription_ddl() function

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