Hi Vaibhav, I just reviewed the patch and got some comments:
> On Nov 11, 2025, at 23:51, Vaibhav Dalvi <[email protected]> > 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 practice as 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, is there 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/
