Re: [PATCH] Add pg_get_subscription_ddl() function

Hi Vaibhav.

Here are some review comments for v5-0001.

======
doc/src/sgml/func/func-info.sgml

1.
+ <title>Get Object DDL Functions</title>

Should the title just be "Object DDL Functions" (e.g. sans the "Get")?

~~~

2.
+ <para>
+ The functions shown in <xref linkend="functions-get-object-ddl-table"/>
+ print the DDL statements for various database objects.
+ (This is a decompiled reconstruction, not the original text
+ of the command.)
+ </para>

/print the DDL/return the DDL/

~~~

3.
+ <table id="functions-get-object-ddl-table">
+ <title>Get Object DDL Functions</title>

Ditto above ("Get" is not really needed).

~~~

4.
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_get_subscription_ddl</primary>
+ </indexterm>
+ <function>pg_get_subscription_ddl</function> (
<parameter>subscription</parameter> <type>text</type> )
+ <returnvalue>text</returnvalue>
+ </para>

The name of the parameter could be more descriptive:

/<parameter>subscription</parameter>/<parameter>sub_name</parameter>/

~~~

5.
+ Reconstructs the creating command for a subscription.
+ The result is a complete <command>CREATE SUBSCRIPTION</command>
+ statement. The <literal>connect</literal> option set to
+ <literal>false</literal>.
+ </para>

I felt those first two sentences could be combined:

SUGGESTION
Returns the <command>CREATE SUBSCRIPTION</command> command that would
create this subscription.

======
src/backend/utils/adt/ruleutils.c

build_subscription_ddl_string:

6.
+/*
+ * build_subscription_ddl_string - Build CREATE SUBSCRIPTION statement for
+ * a subscription from its OID. This is internal version which helps
+ * pg_get_subscription_ddl_name() and pg_get_subscription_ddl_oid().
+ */
+char *
+build_subscription_ddl_string(const Oid suboid)

Typo? "This is internal version"

Also, if it is only an internal helper, then why isn't it declared static?

~~~

7.
+ /* Setting 'slot_name' to none must set 'enabled' to false as well */
+ if (!DatumGetBool(datum) || isnull)
+ appendStringInfoString(&buf, ", enabled = false");
+ else
+ appendStringInfoString(&buf, ", enabled = true");

Mostly code is using ternary for the simple bool options. But some are
not. Consistent use of ternary for all of them might be better

~~~

8.
+ /* Get two-phase commit option */
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subtwophasestate);
+ if (DatumGetChar(datum) == LOGICALREP_TWOPHASE_STATE_DISABLED)
+ appendStringInfoString(&buf, ", two_phase = off");
+ else
+ appendStringInfoString(&buf, ", two_phase = on");

Here's another one that could have been a ternary.

~~~

9.
Is all the spacing between options and their values necessary? For
example, in "subscription.sql", you didn't put spaces everywhere in
the CREATE SUBSCRIPTION command. So why does the DDL generation put
them there? Anyway, I guess all the new DDL routines should be using
the same convention for option spacing, but I don't know what that is.

~~~

pg_get_subscription_ddl_name:

10.
+{
+ List *result = NIL;
+ Datum *elems;
+ int nelems,
+ i;
+
+ deconstruct_array_builtin(text_array, TEXTOID, &elems, NULL, &nelems);
+
+ if (nelems == 0)
+ return NIL;
+
+ for (i = 0; i < nelems; i++)
+ result = lappend(result, makeString(TextDatumGetCString(elems[i])));
+
+ return result;
+}

10a.
The early exit "if (nelems == 0)" is unnecessary; IMO just remove it.
The result = NIL already, and the loop does nothing when nelems is 0.

~

10b.
Variable 'i' can be declared as a for-loop variable

======
src/test/regress/expected/subscription.out

11.
+WARNING: subscription was created, but is not connected
+HINT: To initiate replication, you must manually create the
replication slot, enable the subscription, and alter the subscription
to refresh publications.

I wonder if the tests should suppress WARNINGS because you don't
really care about output like this, right?

======
src/test/regress/sql/subscription.sql

12.
+-- Non-superusers and which don't have pg_create_subscription and/or
+-- pg_read_all_data permission can't get ddl

typo: /and which/

Maybe something like this:
-- Non-superusers without pg_create_subscription and/or
pg_read_all_data permissions cannot retrieve the DDL.

~~~

13.
+CREATE SUBSCRIPTION "regress_TestSubddL2" CONNECTION 'host=unknown
user=dvd password=pass123'
+ PUBLICATION "testpub2", "TestPub3" WITH (connect=false, slot_name='slot1',
+ enabled=off);
+SELECT pg_get_subscription_ddl('regress_TestSubddL2');

Typo? Why the capital "L" in "regress_TestSubddL2"

Also, why is it called "regress_TestSubddL2"; there wasn't a
"regress_TestSubddL1"?

======
Kind Regards,
Peter Smith.
Fujitsu Australia