On Thu, Mar 20, 2025 at 5:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > <khannashubham1197@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.
