Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-18 Thread Peter Smith
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 wrote: > > Thanks Peter for the close look. > >> 1. >> >> + * The status or value of the options 'create_slot' and 'copy_data' not

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-18 Thread Vaibhav Dalvi
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, an

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-17 Thread Peter Smith
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, an

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-17 Thread Vaibhav Dalvi
Hi, Thank you for the review. Please find the attached patch. Regards, Vaibhav EnterpriseDB On Tue, Nov 18, 2025 at 7:28 AM Peter Smith wrote: > Some review comments for v6. > > == > src/backend/utils/adt/ruleutils.c > > 1. > +/* > + * pg_get_subscription_string > + * Build CREATE SUBSCRIP

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-17 Thread Peter Smith
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_oi

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-17 Thread Vaibhav Dalvi
Hi Hackers, Thank you Chao Li, Peter Smith and Alvaro for the review. I have incorporated all your review comments except below ones: 7 > > ``` > + /* Append connection info to the CREATE SUBSCRIPTION statement */ > + appendStringInfo(&buf, "CONNECTION \'%s\'", conninfo); > ``` > A co

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-13 Thread Álvaro Herrera
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 acc

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-12 Thread Peter Smith
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

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-12 Thread Álvaro Herrera
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_subscr

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-11 Thread Chao Li
Hi Vaibhav, I just reviewed the patch and got some comments: > On Nov 11, 2025, at 23:51, Vaibhav Dalvi > wrote: > > > 1. ``` + +/* + * build_subscription_ddl_string - Build CREATE SUBSCRIPTION statement for + * a subscription from its OID. This is internal version which helps + * pg_get_su

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-11 Thread Peter Smith
Hi Vaibhav. Here are some review comments for v5-0001. == doc/src/sgml/func/func-info.sgml 1. + Get Object DDL Functions Should the title just be "Object DDL Functions" (e.g. sans the "Get")? ~~~ 2. + +The functions shown in +print the DDL statements for various database obj

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-11 Thread Vaibhav Dalvi
Hi Alvaro, 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. I also agree with you. I couldn't explain it well so I thought of doing the changes. Please find the attached p

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-11 Thread Álvaro Herrera
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 CreateSubscr

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-10 Thread Vaibhav Dalvi
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(). Please find attached patches. Regards, Vaibhav On Fri, Nov 7, 2025 at 5:41 PM Álvaro Herrera wrote: > On 2025-Nov-0

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-07 Thread Álvaro Herrera
On 2025-Nov-07, Vaibhav Dalvi wrote: > On Thu, Nov 6, 2025 at 9:18 PM Álvaro Herrera 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

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-07 Thread Vaibhav Dalvi
Hi Alvaro, Thanks for your input. On Thu, Nov 6, 2025 at 9:18 PM Álvaro Herrera 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 s

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-06 Thread Álvaro Herrera
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 p

Re: [PATCH] Add pg_get_subscription_ddl() function

2025-11-06 Thread Vaibhav Dalvi
Hi Hackers, Please find the revised patch for the `pg_get_subscription_ddl()` function attached. Based on feedback, this version of the function now supports calling the DDL retrieval using either the subscription name or the OID, as shown in the examples below: ```sql postgres=# SELECT pg_get_s

[PATCH] Add pg_get_subscription_ddl() function

2025-10-31 Thread Vaibhav Dalvi
Hi Hackers, I am submitting a patch as a part of a larger Retail DDL functions project described by Andrew Dunstan here . This patch creates a function pg_get_subscription_ddl, designed to retrieve the full