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

On Wed, Mar 26, 2025 at 10:21 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Mar 26, 2025 at 2:05 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> >
> > On Tue, Mar 25, 2025, at 8:07 AM, Shubham Khanna wrote:
> >
> > Apologies for the oversight. I have attached the patches now. Please
> > find them included here.
> >
> >
> > I started looking at this patch. When I started reading the commit message, I
> > didn't understand why the options that provide names to objects are
> > incompatible with --all option. I agree that --all and --database should be
> > incompatible but the others shouldn't. We already have a check saying that the
> > number of specified objects cannot be different from the number of databases.
> > Why don't rely on it instead of forbidding these options?
> >
>
> We must ensure to match the order of objects specified with the
> database as well (The doc says: The order of the multiple publication
> name switches must match the order of database switches.). It is easy
> for users to do that when explicitly specifying databases with -d
> switches but not with --all case. I can see a way to extend the
> current functionality to allow just fetching databases from the
> publisher and displaying them to the user, then we can expect the user
> to specify the names of other objects in the same order but not
> otherwise.
>
> >
> > What happens if you don't specify the dbname in -P option?
> >
> > + /* Establish a connection to the source server */
> > + conn = connect_database(opt->pub_conninfo_str, true);
> >
> > It defaults to user name. If you are using another superuser (such as admin)
> > the connection might fail if there is no database named 'admin'. vacuumdb that
> > has a similar --all option, uses another option (--maintenance-db) to discover
> > which databases should be vacuumed. I'm not suggesting to add the
> > --maintenance-db option. I wouldn't like to hardcode a specific database
> > (template1, for example) if there is no dbname in the connection string.
> > Instead, suggest the user to specify dbname into -P option. An error message
> > should be provided saying the exact reason: no dbname was specified.
> >
>
> But why shouldn't we use the same specification as vacuumdb, which is:
> "If not specified, the postgres database will be used, or if that does
> not exist, template1 will be used."?
>

I agree that ensuring the correct order of objects (like publications)
matching with databases is crucial, especially when explicitly
specifying databases using -d switches.
To address this, I have created a 0002 patch that aligns object
creation order with the corresponding databases.

> >
> > I checked the applications that provide multiple synopsis using the following command.
> >
> > grep '<cmdsynopsis' doc/src/sgml/ref/*.sgml | awk '{print $1}' | sort | uniq -c
> >
> > There are just 3 applications that have multiple cmdsynopsis. pgbench has 2
> > distinct tasks (load and run) so it makes sense to have 2 synopsis. pg_ctl has
> > multiple tasks and also deserves multiple synopsis. The complexity introduced
> > into vacuumdb (per table, per schema) seems to justify multiple synopsis too.
> > However, the same logic doesn't apply here. IMO 0002 shouldn't be applied
> > because the additional option (--all) doesn't justify multiple synopsis for
> > syntax clarification.
> >
>
> Yeah, also, vacuumdb doesn't have a separate line for --all in
> synopsis. So, I agree with you about not adding '--all' option in the
> synopsis.
>
> --

The attached patches contain the suggested changes. They also address
the comments provided by Ashutosh (at [1]) and Euler (at [2]).

[1] - https://www.postgresql.org/message-id/CAExHW5uvp6LWfgcysohDaOaNhqAbmuc%3D9BwWke%3D6KPRZ%2BwVOkA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/e32c358b-95b5-426c-9baa-281812821588%40app.fastmail.com

Thanks and regards,
Shubham Khanna.