Re: dropdb --force

On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for fixing the comments. The changes looks fine to me. I have
> > fixed the first comment, attached patch has the changes for the same.
> >
>
> Few comments:
> --------------------------
> 1. There is a lot of duplicate code in this test.  Basically, almost
> the same code is used once to test Drop Database command and dropdb.
> I think we can create a few sub-functions to avoid duplicate code, but
> do we really need to test the same thing once via command and then by
> dropdb utility?   I don't think it is required, but even if we want to
> do so, then I am not sure if this is the right test script.  I suggest
> removing the command related test.
>

Pavel: What is your opinion on this?

> 2.
> ok( TestLib::pump_until(
> + $killme,
> + $psql_timeout,
> + \$killme_stderr,
> + qr/FATAL:  terminating connection due to administrator command/m
> + ),
> + "psql query died successfully after SIGTERM");
>
> Extra space before TestLib.

Ran perltidy, perltidy adds an extra space. I'm not sure which version
is right whether to include space or without space. I had noticed
similarly in 001_stream_rep.pl, in few places space is present and in
few places it is not present. If required I can update based on
suggestion.

>
> 3.
> +=item pump_until(proc, psql_timeout, stream, untl)
>
> I think moving pump_until to TestLib is okay, but if you are making it
> a generic function to be used from multiple places, it is better to
> name the variable as 'timeout' instead of 'psql_timeout'
>

Fixed.

> 4. Have you ran perltidy, if not, can you please run it?  See
> src/test/perl/README for the recommendation.
>

I have verified by running perltidy.

Please find the updated patch attached. 1st patch is same as previous,
2nd patch includes the fix for comments 2,3 & 4.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com