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

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/