Re: [PATCH] Add pg_get_subscription_ddl() function

Thanks Peter for the close look.

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?
>
> It is not consistent but we don't have any other option because though we
explicitly
return them in the DDL string we have to use the default values because
we don't know the exact values for these two parameters. Using default to
explicitly
return them in the DDL string will be a problem because default value may
change
in the future, so better to not include in the ddl string and lets server
decide the
default value at the creation time.

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.

I think it is fine to not follow the documentation order(at-least for these
two options)
because that's not a hard and fast rule and option order doesn't matter.

Please find a revised patch.

Regards,
Vaibhav

On Tue, Nov 18, 2025 at 12:42 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:

> 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
>