Re: dropdb --force

On Tue, Nov 26, 2019 at 11:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Nov 25, 2019 at 11:22 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> > >
> > >
> > >
> > > ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignesh21(at)gmail(dot)com> napsal:
> > >>
> > >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >> >
> > >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21(at)gmail(dot)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?
> > >
> > >
> > > I have not any problem with this reduction.
> > >
> > > We will see in future years what is benefit of this test.
> > >
> >
> > Fixed, removed dropdb utility test.
> >
>
> Hmm, you have done the opposite of what I have asked. This test file
> is in rc/bin/scripts/t/ where we generally keep the tests for
> utilities. So, retaining the dropdb utility test in that file seems
> more sensible.
>

Fixed. Retained the test of dropdb utility and removed drop database
sql command test.

> +ok( TestLib::pump_until(
> + $killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
> + 'acquired pid');
>
> How about changing 'acquired pid' to 'acquired pid for SIGTERM'?
>

Fixed. Changed as suggested.

> > >> >
> > >>
> > >> I have verified by running perltidy.
> > >>
>
> I think we don't need to run perltidy on the existing code. So, I am
> not sure if it is a good idea to run it for file 013_crash_restart.pl
> as it changes some existing code formatting.
>

I have retained the format same as old format, one additional change I
added was to break the line if the line is lengthy in the modified
code.

Attached patch has the fixes for the above comments.

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