Re: [PATCH] Add pg_get_subscription_ddl() function - Mailing list pgsql-hackers

Some more review comments for v7.

======

1.
+ * The status or value of the options 'create_slot' and 'copy_data' not
+ * available in the catalog table.  We can use default values i.e. TRUE
+ * for both.  This is what pg_dump uses.

Is it consistent to just use defaults for these 2 parameters, and not
even explicitly return them in the DDL string, when IIRC earlier you
said we cannot use defaults for any of the others?

~~~

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 slotname */
+ datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subslotname,
+ &isnull);
+ if (!isnull)
+ appendStringInfo(&buf, ", slot_name=%s",
+ quote_literal_cstr(NameStr(*DatumGetName(datum))));
+ else
+ {
+ appendStringInfoString(&buf, ", slot_name=none");
+ /* Setting slot_name to none must set create_slot to false */
+ appendStringInfoString(&buf, ", create_slot=false");
+ }
+

2a.
The 'enabled' condition should also be checking the slot name none
(aka 'null' check), so I think this code became broken in v7 when you
swapped the order of the parameters without handling the condition.

~

2b.
You'll need different logic to emit the 'create_slot' parameter in the
same order that it appears in the documentation. Previously, I had
suggested assigning all the parameters to variables up-front before
building your DDL string. If that were done, then it should be easy to
reshuffle the order of the DDL parameters without jumping through
hoops.

~~

3.
+ appendStringInfo(&buf, ", enabled=%s",
+ (!DatumGetBool(datum) || isnull) ? "false" : "true");

Can't you rearrange the ternary to be "true" : "false" like all the others?

~~~

4.
+ if (DatumGetChar(datum) == LOGICALREP_STREAM_OFF)
+ appendStringInfoString(&buf, ", streaming=false");
+ else if (DatumGetChar(datum) == LOGICALREP_STREAM_ON)
+ appendStringInfoString(&buf, ", streaming=true");
+ else
+ appendStringInfoString(&buf, ", streaming=parallel");

When I suggested changing all the boolean params to "true" and
"false", I wasn't expecting you would change this one, which is an
enum, not a boolean. The docs for this one refer to values "parallel",
"on" and "off", so it's better to use the same values as the
documentation.

~~

5.
+ appendStringInfo(&buf, ", two_phase=%s",
+ DatumGetChar(datum) == LOGICALREP_TWOPHASE_STATE_DISABLED ? "false" : "true");

Can't you rearrange the ternary to be "true" : "false" like all the others?

======
[1] https://www.postgresql.org/docs/devel/sql-createsubscription.html

Kind Regards,
Peter Smith.
Fujitsu Australia