Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

On Wed, 5 Feb 2025 at 01:26, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
>
> On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi Shubham,
> >
> > On Tue, Feb 4, 2025 at 2:10 PM Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > > > >
> > > >
> > > > It could be a bit tricky to find that for users but they can devise a
> > > > query to get the names and numbers of databases matching the given
> > > > pattern. OTOH, I am not sure there is a clear need at this stage for
> > > > pattern matching for this tool. So, we can go with a simple switch as
> > > > you are proposing at this stage.
> > > >
> > >
> > > After reconsidering the idea of supporting '--all-databases' switch is
> > > the better approach at this stage, I have added the new switch in the
> > > latest patch.
> > > The attached patch contains the suggested changes.
> >
> > + If neither <option>-d</option> nor <option>-a</option> is
> > + specified, <application>pg_createsubscriber</application> will use
> > + <option>--all-databases</option> by default.
> >
> > As pointed upthread by me and Peter, using --all-databases by default
> > is not a good behaviour.
> >
> > But the code doesn't behave like --all-databases by default. Looks
> > like we need to fix the documentation.
>
> Updated the documentation accordingly and added the current behaviour
> of -a/--all-databases option.
>
> > + /* Generate publication and slot names if not specified */
> > + SimpleStringListCell *cell;
> > +
> > + fetch_all_databases(opt);
> > +
> > + cell = opt->database_names.head;
> >
> > We don't seem to check existence of publication and slot name
> > specification as the comment indicates. Do we need to check that those
> > names are not specified at all? and also mention in the documentation
> > that those specifications are required when using -a/--all-databases
> > option?
> >
>
> Added a check to verify that publication and slot names are not
> manually specified when using -a/--all-databases option and updated
> the documentation accordingly.
>
> The attached patch contains the suggested changes.
>
Hi Shubham,

Here are some of my comments:

1. We should start the comment text from the next line
+
+/* Function to validate all the databases and generate publication/slot names
+ * when using '--all-databases'.
+ */

2. Here in error message it should be '--database'
+ /* Check for conflicting options */
+ if (opt->all_databases && opt->database_names.head != NULL)
+ {
+ pg_log_error("cannot specify --dbname when using --all-databases");
+ exit(1);
+ }
+

3. I think checking 'opt->all_databases' in if conditions in function
'validate_databases' is not required
+static void
+validate_databases(struct CreateSubscriberOptions *opt)
+{
+ /* Check for conflicting options */
+ if (opt->all_databases && opt->database_names.head != NULL)
+ {
+ pg_log_error("cannot specify --dbname when using --all-databases");
+ exit(1);
+ }

as we are already checking it while calling the function
+ /* Validate and process database options */
+ if (opt.all_databases)
+ validate_databases(&opt);
+

4. Here we should update the count of 'num_replslots' and 'num_pubs'

+ while (cell != NULL)
+ {
+ char slot_name[NAMEDATALEN];
+
+ snprintf(slot_name, sizeof(slot_name), "%s_slot", cell->val);
+ simple_string_list_append(&opt->replslot_names, slot_name);
+
+ snprintf(slot_name, sizeof(slot_name), "%s_pub", cell->val);
+ simple_string_list_append(&opt->pub_names, slot_name);
+
+ cell = cell->next;
+ }
Since we are not updating these counts, the names are reflected as expected.

Should also store subscription names similarly? Or maybe store
subscription names and assign slot names same as subscription names?

5. Since --all-databases option is added we should update the comment:
/*
* If --database option is not provided, try to obtain the dbname from
* the publisher conninfo. If dbname parameter is not available, error
* out.
*/

6. Extra blank lines should be removed
}
}

+
+
/* Number of object names must match number of databases */

Thanks and Regards,
Shlok Kyal