On Thu, Mar 20, 2025 at 5:54 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> <ashutosh.bapat....@gmail.com> wrote:
> >
> > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > <khannashubham1...@gmail.com> wrote:
> >
> >
> > +# run pg_createsubscriber with '--database' and '--all' without '--dry-run'
> > +# and verify the failure
> > +command_fails_like(
> > + [
> > + 'pg_createsubscriber',
> > + '--verbose',
> > + '--pgdata' => $node_s->data_dir,
> > + '--publisher-server' => $node_p->connstr($db1),
> > + '--socketdir' => $node_s->host,
> > + '--subscriber-port' => $node_s->port,
> > + '--database' => $db1,
> > + '--all',
> > + ],
> > + qr/--database cannot be used with --all/,
> > + 'fail if --database is used with --all');
> >
> >
> > Why does only this test not use --dry-run, but all other such tests
> > use --dry-run? Either they should all use it or not use it.
> >
>
> I think this patch is adding a lot of extra tests which I don't think
> are required. We should have one positive test in --dry-run mode
> similar to ('run pg_createsubscriber without --databases') and
> probably verify in the LOG that it has expanded commands for all
> databases. Also, for negative tests, one or two tests are sufficient
> instead of testing all possible combinations. I don't expect this new
> option to add a noticeable testing time.
>As per your suggestions, I have updated the test cases in the v17-0001 patch. The primary tests now focus on a positive test in --dry-run mode, similar to the one for run pg_createsubscriber without --databases, along with verification in the log to ensure that commands for all databases are expanded. Also, I have limited the negative tests to just one or two representative cases, avoiding unnecessary combinations. I have moved the additional test cases to the v17-0002 patch to ensure that the testing time for the new option remains negligible. > * <refsynopsisdiv> > + <cmdsynopsis> > + <command>pg_createsubscriber</command> > + <arg rep="repeat"><replaceable>option</replaceable></arg> > + <group choice="plain"> > + <group choice="req"> > + <arg choice="plain"><option>-a</option></arg> > + <arg choice="plain"><option>--all</option></arg> > + </group> > + <group choice="req"> > + <arg choice="plain"><option>-D</option> </arg> > + <arg choice="plain"><option>--pgdata</option></arg> > + </group> > + <replaceable>datadir</replaceable> > + <group choice="req"> > + <arg choice="plain"><option>-P</option></arg> > + <arg choice="plain"><option>--publisher-server</option></arg> > + </group> > + <replaceable>connstr</replaceable> > > Most of this is unrelated to this patch. I suggest making a top-up > patch, we can commit it separately. > I have created a separate v17-0003 patch that includes the synopsis for the '--all' option. The attached patch at [1] contains the suggested changes. [1] - https://www.postgresql.org/message-id/CAHv8RjLj0KxVHbxaPZHzudGS1igzDMccFE8LDh4LHNJR_2Aqug%40mail.gmail.com Thanks and regards, Shubham Khanna.
