Обсуждение: [PATCH] Add pg_get_subscription_ddl() function

Hello Vaibhav,

I wonder why is Subscription->publications a list of String rather than
a list of C strings.  That's something you'd see in a Node structure,
but Subscription is not a node, so this seems wasteful and pointless.
This is of course not the fault of your patch, but the fact that your
patch feels the need to expose the textarray_to_stringlist() auxiliary
function made me wonder about it.  I think that's not a great function
to expose, at least not from pg_subscription.h, so maybe we should
instead think about getting rid of the String nodes from there and see
about making this whole thing simpler.  On the other hand, we already
have function strlist_to_textarray() declared in objectaddress.h, which
is kinda the inverse of this ...  Looking further, I wonder why we have
publicationListToArray() when it seems strlist_to_textarray() is likely
to fit the bill, with a couple of tweaks  --- assuming we want to keep
using String nodes in Subscription, which I doubt.

Oh, we also have textarray_to_strvaluelist() which is essentially
identical, but also static.  If we're making one of them non-static,
then for sure let's remove the other one.  But maybe what we really need
is a third one to use in ruleutils, and expose neither?  (I think if we
get rid of the String around Subscription->publications, that's likely
what I'd do, since they'd be mostly trivial wrappers around
deconstruct_array_builtin.)

Anyway, I guess this is a long-winded way of saying that I don't think
making textarray_to_stringlist() non-static is a great idea.  At least
not where you're doing it.  I would start this with a 0001 patch that
gets rid of String usage there, and then the rest of your function on
top of that.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

On 2025-Nov-07, Vaibhav Dalvi wrote:

> On Thu, Nov 6, 2025 at 9:18 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> > Hello Vaibhav,
> >
> > I wonder why is Subscription->publications a list of String rather than
> > a list of C strings.  That's something you'd see in a Node structure,
> > but Subscription is not a node, so this seems wasteful and pointless.
> >
> 
> I looked more into this and came to know that we can't make
> Subscription->publications a list of C strings because input publications
> list is also in the list of String from the parser:
> 
> CreateSubscriptionStmt:
> CREATE SUBSCRIPTION name CONNECTION Sconst PUBLICATION name_list
> opt_definition
> {
> CreateSubscriptionStmt *n =
> makeNode(CreateSubscriptionStmt);
> n->subname = $3;
> n->conninfo = $5;
> n->publication = $7;
> n->options = $8;
> $$ = (Node *) n;
> };

But this is a CreateSubscriptionStmt (which is indeed a Node), not a
Subscription (which isn't).  Different thing.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle."  (Larry Wall, Apocalypse 6)

On 2025-Nov-11, Vaibhav Dalvi wrote:

> Hi Alvaro,
> 
> Thanks for the explanation.
> 
> I tried to get rid of String usage in 0001 patch.
> Prepared 0002 patch for actual implementation of the
> function p_get_subscription_ddl().

OK, now I understand what you meant when you mentioned
CreateSubscriptionStmt: the problem is that the same code is being used
for the purposes of processing the subscription lists of both
Subscription and SubscriptionStmt.  I think this whole code is a bit
ugly TBH -- not yours, but what was there before -- but now that I've
realized this, I think stripping those values out of the String wrapping
is the wrong direction to go in, because it leads to more contortions
rather than less.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"They proved that being American is not just for some people"
                                               (George Takei)

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

Hi Vaibhav,

I just reviewed the patch and got some comments:

> On Nov 11, 2025, at 23:51, Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com> wrote:
>
>
> <v5-Add-pg_get_subscription_ddl-function.patch>

1.
```
+
+/*
+ * 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)
```

There are several existing similar functions that take an oid as input and return a string, for example:

* extern char *pg_get_indexdef_string(Oid indexrelid);
* extern char *pg_get_constraintdef_command(Oid constraintId);

So, can we keep the same naming convention and rename the function to pg_get_subscription_string().

2
```
+    publist = text_array_to_string_list(DatumGetArrayTypeP(datum));
+    pubnames = makeStringInfo();
```

Recently there are some efforts done to replace usages of StringInfo with StringInfoData, so I guess you may apply the
practiceas well. See [1].  

3
```
+    /* 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");
+
+    /* Get binary option */
+    datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
+                                   Anum_pg_subscription_subbinary);
+    appendStringInfo(&buf, ", binary = %s",
+                     DatumGetBool(datum) ? "true" : "false”);
```

Logic of handling these two fields are the same, but you implement in two different ways, can we keep consistent?

4
```
+/*
+ * pg_get_subscription_ddl_name
+ *        Get CREATE SUBSCRIPTION statement for a subscription.
+ *
+ * This takes name as parameter for pg_get_subscription_ddl().
+ */
+Datum
+pg_get_subscription_ddl_name(PG_FUNCTION_ARGS)
+{
```

This function name is quite confusing, I think it should be pg_get_subscription_ddl_by_name().

5
```
+/*
+ * pg_get_subscription_ddl_oid
+ *        Get CREATE SUBSCRIPTION statement for a subscription.
+ *
+ * This takes oid as parameter for pg_get_subscription_ddl().
+ */
+Datum
+pg_get_subscription_ddl_oid(PG_FUNCTION_ARGS)
```

Similar to 4. I think the function name should be pg_get_subscription_ddl_by_oid().

6
```
+                 errdetail("Only roles with privileges of the \"%s\" and/or \"%s\" role may get ddl.",

```

“May get ddl” maybe change to “may view subscription DDL”.

7
```
+    /* Append connection info to the CREATE SUBSCRIPTION statement */
+    appendStringInfo(&buf, "CONNECTION \'%s\'", conninfo);
```

A connection string contains a db access credential, and ROLE_PG_READ_ALL_DATA (not a high privilege) can view the DDL,
isthere a concern of leaking the secret? Should we redact the password in connection string? 

8
```
+        appendStringInfo(&buf, ", slot_name = \'%s\'",
+                         NameStr(*DatumGetName(datum)));
```

Instead of hardcode single-quotes, can we consider using quote_literal_cstr(), for example, in slotsync.c:
```
    appendStringInfo(&cmd,
        "SELECT pg_is_in_recovery(), count(*) = 1”
        " FROM pg_catalog.pg_replication_slots”
        " WHERE slot_type='physical' AND slot_name=%s”,
                 quote_literal_cstr(PrimarySlotName));
```

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=6d0eba66275b125bf634bbdffda90c70856e3f93

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

If build_subscription_ddl_string is "internal" as its comment claims,
why is it declared extern in ruleutils.h?  I think it should be a static
function instead.  If you want to make it extern, it should live in
src/backend/catalog/pg_subscription.c and its prototype in
src/include/catalog/pg_subscription.h.  And if you do move it to
pg_subscription.c (but I don't necessarily agree with that), then you
don't need a third copy of textarray_to_stringlist.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)

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

On 2025-Nov-13, Peter Smith wrote:

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

Yeah, I was asking myself the same.  I think we definitely want options
to be printed when there are GUCs that can affect the outcome (e.g.,
something that is considered default in this server but not on a
differently- configured one would give different results).  But for
those that are just hardcoded defaults, omitting them would make sense.

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

There are a couple of these patches that have an auxiliary
pretty-printing helper function to add newline-tabs instead of
individual spaces.  I think that wouldn't work as nicely if you tried to
condense the printing in the way you suggest.  On the other hand, if you
have a long format string, it's harder to visually match each specifier
to its corresponding argument.  If this was performance-critical code I
would agree to use denser code and avoid function calls, but for this
usage I don't think we care much.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

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

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

Hi Vaibhav.

Thanks for the updates. I don't have any new review comments for v8 --
just a couple of the same ones as before.

On Wed, Nov 19, 2025 at 12:00 AM vDalvi <vaibhav.dalvi@enterprisedb.com> wrote:
>
> 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.
>

I don't understand "... will be a problem because default value may
change in the future". Why do we even care if some defaults might
change for some unknown future version of Postgres? Isn't the purpose
of the function to return a DDL string that, when executed, would
re-create the specified subscription *EXACTLY* as it is defined
here-and-now? That's why I thought even "copy_data" and "create_slot"
values ought to be in the DDL.

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

Yeah, my comments about ordering are not really directed specifically
at this get_subscription_ddl() function implementation. I was thinking
more of the bigger picture -- AFAIK, there are going to be many of
these get_xxx_ddl() functions, so IMO you really do need to have in
place some common guidelines (e.g. "use the same option ordering as in
the docs"), and follow those rules even when they are not strictly
needed. Otherwise, consistency/predictability goes out the window when
every function gets implemented on the whim of different people. Each
function implementation taken individually may be fine, but I felt the
lack of consistency across many similar functions would not be a good
result. YMMV.

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