On Wed, Mar 26, 2025 at 10:21 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Mar 26, 2025 at 2:05 AM Euler Taveira <eu...@eulerto.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.
v22-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
v22-0004-Additional-test-cases.patch
Description: Binary data
v22-0003-Synopsis-for-all-option.patch
Description: Binary data
v22-0002-Fetch-databases-from-publisher.patch
Description: Binary data
