Re: [PATCH] Add pg_get_subscription_ddl() function

Hi,

A couple more comments.

These seem like general questions that might apply to other DDL
funtion implementations -- not only this one.

======

1.
Question - is it deliberate to *always* return DLL with every possible
option assigned, even if those are just the option default values?
e.g. For something like the CREATE PUBLICATION command the string
returned could be only half the size if it accounts for default.

Following on from that, is there any plan for the function to take
some true/false flag param to tell it to return a full/condensed DDL?

~~~

2.
I was also wondering if it was really necessary to have so many
appendStringInfoString() calls to reconstruct the command.

I felt something like below (where every option value is assigned a
variable) is more readable:

SUGGESTION:
{

/* Get slotname */
datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subslotname, &isnull);
char *val_slot_name = isnull ? NULL : strdup(NameStr(*DatumGetName(datum)));

/* Get enabled option */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subenabled);
/* Setting 'slot_name' to none must set 'enabled' to false as well */
bool val_enabled = DatumGetBool(datum) && (val_slot_name != NULL);

/* Get binary option */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subbinary);
bool val_binary = DatumGetBool(datum);

/* Get streaming option */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_substream);
char val_streaming = DatumGetChar(datum);

/* Get sync commit option */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subsynccommit);
char *val_synchronous_commit = strdup(TextDatumGetCString(datum));

/* Get two-phase commit option */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subtwophasestate);
bool val_two_phase = DatumGetChar(datum) != LOGICALREP_TWOPHASE_STATE_DISABLED;

/* Disable on error? */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subdisableonerr);
bool val_disable_on_error = DatumGetBool(datum);

/* Password required? */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subpasswordrequired);
bool val_password_required = DatumGetBool(datum);

/* Run as owner? */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subrunasowner);
bool val_run_as_owner = DatumGetBool(datum);

/* Get origin */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_suborigin);
char *val_origin = strdup(TextDatumGetCString(datum));

/* Failover? */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subfailover);
bool val_failover = DatumGetBool(datum);

/* Retain dead tuples? */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_subretaindeadtuples);
bool val_retain_dead_tuples = DatumGetBool(datum);

/* Max retention duration */
datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
Anum_pg_subscription_submaxretention);
int val_max_retention_duration = DatumGetInt32(datum);

appendStringInfo(&buf,
" WITH (connect = false"
", slot_name = %s"
", enabled = %s"
", binary = %s"
", streaming = %s"
", synchronous_commit = %s"
", two_phase = %s"
", disable_on_error = %s"
", password_required = %s"
", run_as_owner = %s"
", origin = %s"
", failover = %s"
", retain_dead_tuples = %s"
", max_retention_duration = %d"
");",
val_slot_name == NULL ? "none, create_slot = false" :
quote_literal_cstr(val_slot_name),
val_enabled ? "true" : "false",
val_binary ? "true" : "false",
val_streaming == LOGICALREP_STREAM_OFF ? "off" : val_streaming ==
LOGICALREP_STREAM_ON ? "on" : "parallel",
val_synchronous_commit,
val_two_phase ? "on" : "off",
val_disable_on_error ? "on" : "off",
val_password_required ? "on" : "off",
val_run_as_owner ? "on" : "off",
val_origin,
val_failover ? "on" : "off",
val_retain_dead_tuples ? "on" : "off",
val_max_retention_duration);

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