Hi Peter, Thank you for your latest review comments. I will let the committer decide on those two points.
To all reviewers, thank you for your feedback. Given that the CFBot is green for v8 at https://commitfest.postgresql.org/patch/6208/ ( https://commitfest.postgresql.org/patch/6208/), I believe we are now in a position to move this patch to 'Ready for Committer' status. Hi Andrew, We are moving this thread to 'Ready for Committer' for your final review and patch finalization. Thanks, Vaibhav Dalvi EnterpriseDB On Wed, Nov 19, 2025 at 8:19 AM Peter Smith <[email protected]> wrote: > 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 <[email protected]> > 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 >
