Обсуждение: Removed unused import modules from tap tests

Hi,

While trying to add some new tests, I found that
PostgreSQL::Test::Utils is not required. I felt
PostgreSQL::Test::Utils can be removed from a lot of tap tests which
do not require it. I removed it, ran the tests and found the tests to
be executing fine. I have made a patch including the changes for it.
If this import can be removed, kindly accept the attached patch for
the same.

Regards,
Vignesh

On Tue, Nov 09, 2021 at 09:48:30PM +0530, vignesh C wrote:
> While trying to add some new tests, I found that
> PostgreSQL::Test::Utils is not required. I felt
> PostgreSQL::Test::Utils can be removed from a lot of tap tests which
> do not require it. I removed it, ran the tests and found the tests to
> be executing fine. I have made a patch including the changes for it.
> If this import can be removed, kindly accept the attached patch for
> the same.

I would not have bothered changing things if the names of the modules
were the same across stable branches to minimize merge conflicts.

However, everything has changed on HEAD, so there is a good argument
for simplifying the tests as you are proposing here.  Any thoughts
from others?
--
Michael

On 2021-Nov-10, Michael Paquier wrote:

> I would not have bothered changing things if the names of the modules
> were the same across stable branches to minimize merge conflicts.
> 
> However, everything has changed on HEAD, so there is a good argument
> for simplifying the tests as you are proposing here.  Any thoughts
> from others?

I agree with your reasoning, but I wonder what's the *benefit* of
removing those includes.  IOW, what's the reason not to simply drop the
patch?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Nov-10, Michael Paquier wrote:
>
>> I would not have bothered changing things if the names of the modules
>> were the same across stable branches to minimize merge conflicts.
>>
>> However, everything has changed on HEAD, so there is a good argument
>> for simplifying the tests as you are proposing here.  Any thoughts
>> from others?
>
> I agree with your reasoning..

Agreed, if we are ever to do it now would be the time.

> ..but I wonder what's the *benefit* of removing those includes.  IOW, what's
> the reason not to simply drop the patch?


I think the value is mostly neatnikism, the actual effect on runtime is
unlikely to be measureable.  I won't argue against doing it, but I suspect
we'll just slowly add a lot of these back as tests evolve making excercise
less useful.

--
Daniel Gustafsson        https://vmware.com/

On Wed, Nov 10, 2021 at 6:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Nov-10, Michael Paquier wrote:
>
> > I would not have bothered changing things if the names of the modules
> > were the same across stable branches to minimize merge conflicts.
> >
> > However, everything has changed on HEAD, so there is a good argument
> > for simplifying the tests as you are proposing here.  Any thoughts
> > from others?
>
> I agree with your reasoning, but I wonder what's the *benefit* of
> removing those includes.  IOW, what's the reason not to simply drop the
> patch?

The idea was to clean up the unused import. I noticed that generally
while adding new test files we copy from existing files, this results
in existing unused imports also being added when new tests are added.
It is just a cleanup activity to remove it from the existing code and
probably can be taken care during the review so that it does not get
added in the new tests.

Regards,
Vignesh

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> ..but I wonder what's the *benefit* of removing those includes.  IOW, what's
>> the reason not to simply drop the patch?

> I think the value is mostly neatnikism, the actual effect on runtime is
> unlikely to be measureable.  I won't argue against doing it, but I suspect
> we'll just slowly add a lot of these back as tests evolve making excercise
> less useful.

Yeah, that last was pretty much my reaction.  I don't know enough about
Perl to be sure how much an unused import costs, but I suspect you're
right that it won't be measurable in context, considering that most of
these test scripts run at least one initdb.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>> ..but I wonder what's the *benefit* of removing those includes.  IOW, what's
>>> the reason not to simply drop the patch?
>
>> I think the value is mostly neatnikism, the actual effect on runtime is
>> unlikely to be measureable.  I won't argue against doing it, but I suspect
>> we'll just slowly add a lot of these back as tests evolve making excercise
>> less useful.
>
> Yeah, that last was pretty much my reaction.  I don't know enough about
> Perl to be sure how much an unused import costs, but I suspect you're
> right that it won't be measurable in context, considering that most of
> these test scripts run at least one initdb.

On my laptop (using the "performance" CPU frequency governor to avoid
scaling noise) loading PostgreSQL::Test::Utils takes about 80ms:

$ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Utils;'

real    0m0.079s
user    0m0.078s
sys     0m0.001s

However, if we're also loading PostgreSQL::Test::Cluster, the difference
is negligible, because most of the time is spent loading modules used by
both:

$ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils;'

real    0m0.092s
user    0m0.088s
sys     0m0.004s

$ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Cluster;'

real   0m0.090s
user   0m0.081s
sys    0m0.008s

- ilmari

On Wed, Nov 10, 2021 at 9:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, that last was pretty much my reaction.  I don't know enough about
> Perl to be sure how much an unused import costs, but I suspect you're
> right that it won't be measurable in context, considering that most of
> these test scripts run at least one initdb.

I think focusing on the runtime might be missing the point to some
extent. Removing used #include directives from C source files is
generally good practice just for cleanliness, and this is the same
kind of thing. That said, the argument that future changes are likely
to just put the same 'use' directive back seems to me to have some
merit. But we've probably spent more energy debating this than the
topic deserves. It wouldn't hurt to remove these, and it also won't
hurt if we don't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 11/10/21 09:53, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>> ..but I wonder what's the *benefit* of removing those includes.  IOW, what's
>>> the reason not to simply drop the patch?
>> I think the value is mostly neatnikism, the actual effect on runtime is
>> unlikely to be measureable.  I won't argue against doing it, but I suspect
>> we'll just slowly add a lot of these back as tests evolve making excercise
>> less useful.
> Yeah, that last was pretty much my reaction.  I don't know enough about
> Perl to be sure how much an unused import costs, but I suspect you're
> right that it won't be measurable in context, considering that most of
> these test scripts run at least one initdb.
>
>             


:Cluster uses :Utils, and perl is smart enough not to try to reprocess
the module. Thus the extra cost here is almost certainly very close to zero.


This is a perfectly reasonable piece of boilerplate to use at the top of
a TAP test:


    use strict;

    use warnings;

    use PostgreSQL:Test::Cluster;

    use PostgreSQL::Test::Utils;

    use Test::More;


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com