On Wed, Feb 5, 2025 at 5:31 PM Shlok Kyal <shlok.kyal....@gmail.com> wrote:
>
> On Wed, 5 Feb 2025 at 01:26, Shubham Khanna <khannashubham1...@gmail.com>
> wrote:
> >
> > On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
> > <ashutosh.bapat....@gmail.com> wrote:
> > >
> > > Hi Shubham,
> > >
> > > On Tue, Feb 4, 2025 at 2:10 PM Shubham Khanna
> > > <khannashubham1...@gmail.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 */
>Fixed the given comments. The attached patch contains the suggested changes. Thanks and regards, Shubham Khanna.
v4-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
