Thread: Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2016-02-04 21:43:14 +0000, Robert Haas wrote:
>> Change the way that LWLocks for extensions are allocated.
>>
>> The previous RequestAddinLWLocks() method had several disadvantages.
>> First, the locks would be in the main tranche; we've recently decided
>> that it's useful for LWLocks used for separate purposes to have
>> separate tranche IDs.  Second, there wasn't any correlation between
>> what code called RequestAddinLWLocks() and what code called
>> LWLockAssign(); when multiple modules are in use, it could become
>> quite difficult to troubleshoot problems where LWLockAssign() ran out
>> of locks.  To fix, create a concept of named LWLock tranches which
>> can be used either by extension or by core code.
>>
>> Amit Kapila and Robert Haas
>
> I noticed that this code has no test coverage:
>
> http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
>
> It'd be good to add some, although I'm not entirely sure what the best
> way is. Maybe add a simple pg_stat_statements test?

That would also have the advantage of improving the test coverage for
pg_stat_statements, which is at the moment quite a bit thinner than
the coverage for lwlock.c.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

> That would also have the advantage of improving the test coverage for
> pg_stat_statements, which is at the moment quite a bit thinner than
> the coverage for lwlock.c.

Hmm, the coverage-html report is not currently covering contrib ... I
suppose that's an easily fixable oversight.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Aug 29, 2016 at 10:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund <andres@anarazel.de> wrote:
>> Hi,
>>
>> On 2016-02-04 21:43:14 +0000, Robert Haas wrote:
>>> Change the way that LWLocks for extensions are allocated.
>>>
>>> The previous RequestAddinLWLocks() method had several disadvantages.
>>> First, the locks would be in the main tranche; we've recently decided
>>> that it's useful for LWLocks used for separate purposes to have
>>> separate tranche IDs.  Second, there wasn't any correlation between
>>> what code called RequestAddinLWLocks() and what code called
>>> LWLockAssign(); when multiple modules are in use, it could become
>>> quite difficult to troubleshoot problems where LWLockAssign() ran out
>>> of locks.  To fix, create a concept of named LWLock tranches which
>>> can be used either by extension or by core code.
>>>
>>> Amit Kapila and Robert Haas
>>
>> I noticed that this code has no test coverage:
>>
>> http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
>>
>> It'd be good to add some, although I'm not entirely sure what the best
>> way is. Maybe add a simple pg_stat_statements test?
>
> That would also have the advantage of improving the test coverage for
> pg_stat_statements, which is at the moment quite a bit thinner than
> the coverage for lwlock.c.
>

I will write such a test case either in this week or early next week.
I hope this is not super urgent, let me know if you think otherwise.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
> I will write such a test case either in this week or early next week.

Great.

> I hope this is not super urgent, let me know if you think otherwise.

It's not urgent, no.

Thanks!

Andres

On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
>> I will write such a test case either in this week or early next week.
>
> Great.
>

Okay, attached patch adds some simple tests for pg_stat_statements.
One thing to note is that we can't add pg_stat_statements tests for
installcheck as this module requires shared_preload_libraries to be
set.  Hope this suffices the need.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
>>> I will write such a test case either in this week or early next week.
>>
>> Great.
>>
>
> Okay, attached patch adds some simple tests for pg_stat_statements.
> One thing to note is that we can't add pg_stat_statements tests for
> installcheck as this module requires shared_preload_libraries to be
> set.  Hope this suffices the need.
>

Registered this patch for next CF.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

I have started with the review for this patch and would like to share
some of my initial review comments that requires author's attention.

1) I am getting some trailing whitespace errors when trying to apply
this patch using git apply command. Following are the error messages i
am getting.

[edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch
test_pg_stat_statements-v1.patch:28: trailing whitespace.   $(MAKE) -C $(top_builddir)/src/test/regress all
test_pg_stat_statements-v1.patch:41: space before tab in indent.    $(REGRESSCHECKS)
warning: 2 lines add whitespace errors.

2) The test-case you have added is failing that is because queryid is
going to be different every time you execute the test-case. I think it
would be better to remove the queryid column from "SELECT queryid,
query, calls, rows from pg_stat_statements ORDER BY rows". Below is
the output i got upon test-case execution.

make regresscheck

============== running regression test queries        ==============
test pg_stat_statements       ... FAILED
============== shutting down postmaster               ==============

3) Ok. As you mentioned upthread, I do agree with the fact that we
can't add pg_stat_statements tests for installcheck as this module
requires shared_preload_libraries to be set. But, if there is an
already existing installation with pg_stat_statements module loaded we
should allow the test to run on this installation if requested
explicitly by the user. I think we can add some target like
'installcheck-force' in the MakeFile that would help the end users to
run the test on his own installation. Thoughts?


Also, I am changed the status in the commit-fest from "Needs review"
to "waiting on Author".

On Mon, Sep 19, 2016 at 6:26 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
>>> On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
>>>> I will write such a test case either in this week or early next week.
>>>
>>> Great.
>>>
>>
>> Okay, attached patch adds some simple tests for pg_stat_statements.
>> One thing to note is that we can't add pg_stat_statements tests for
>> installcheck as this module requires shared_preload_libraries to be
>> set.  Hope this suffices the need.
>>
>
> Registered this patch for next CF.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 2, 2016 at 12:48 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Hi,
>
> I have started with the review for this patch and would like to share
> some of my initial review comments that requires author's attention.
>

Thanks.

> 1) I am getting some trailing whitespace errors when trying to apply
> this patch using git apply command. Following are the error messages i
> am getting.
>
> [edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch
> test_pg_stat_statements-v1.patch:28: trailing whitespace.
>     $(MAKE) -C $(top_builddir)/src/test/regress all
> test_pg_stat_statements-v1.patch:41: space before tab in indent.
>      $(REGRESSCHECKS)
> warning: 2 lines add whitespace errors.
>

Fixed.

> 2) The test-case you have added is failing that is because queryid is
> going to be different every time you execute the test-case.
>

It shouldn't be different each time you execute test as this is a hash
code, but I agree that it is not stable and it can vary across
platforms, so removed it from test.

>
> 3) Ok. As you mentioned upthread, I do agree with the fact that we
> can't add pg_stat_statements tests for installcheck as this module
> requires shared_preload_libraries to be set. But, if there is an
> already existing installation with pg_stat_statements module loaded we
> should allow the test to run on this installation if requested
> explicitly by the user. I think we can add some target like
> 'installcheck-force' in the MakeFile that would help the end users to
> run the test on his own installation.
>

I had also thought about it while preparing patch, but I couldn't find
any clear use case.  I think it could be useful during development of
a module, but not sure if it is worth to add a target.  I think there
is no harm in adding such a target, but lets take an opinion from
committer or others as well before adding this target.  What do you
say?


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

> I had also thought about it while preparing patch, but I couldn't find
> any clear use case.  I think it could be useful during development of
> a module, but not sure if it is worth to add a target.  I think there
> is no harm in adding such a target, but lets take an opinion from
> committer or others as well before adding this target.  What do you
> say?

Ok. We can do that.

I have verified the updated v2 patch. The patch looks good to me. I am
marking it  as ready for committer. Thanks.

With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com

On Tue, Nov 8, 2016 at 4:23 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Hi,
>
>> I had also thought about it while preparing patch, but I couldn't find
>> any clear use case.  I think it could be useful during development of
>> a module, but not sure if it is worth to add a target.  I think there
>> is no harm in adding such a target, but lets take an opinion from
>> committer or others as well before adding this target.  What do you
>> say?
>
> Ok. We can do that.
>
> I have verified the updated v2 patch. The patch looks good to me. I am
> marking it  as ready for committer.

Thanks for the review.



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

On 2016-11-08 10:25:11 +0530, Amit Kapila wrote:
>  ifdef USE_PGXS
>  PG_CONFIG = pg_config
>  PGXS := $(shell $(PG_CONFIG) --pgxs)
> @@ -21,3 +25,29 @@ top_builddir = ../..
>  include $(top_builddir)/src/Makefile.global
>  include $(top_srcdir)/contrib/contrib-global.mk
>  endif
> +
> +# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
> +# which typical installcheck users do not have (e.g. buildfarm clients).

> +installcheck:;
> +
> +check: regresscheck
> +
> +submake-regress:
> +    $(MAKE) -C $(top_builddir)/src/test/regress all
> +
> +submake-pg_stat_statements:
> +    $(MAKE) -C $(top_builddir)/contrib/pg_stat_statements

Why is this a submake? That seems to make little sense?   But stepping
back one step further: I don't think we need all the remade rules in the
first place.

REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
and then
installcheck: REGRESS=
to prevent installcheck from doing anything ought to be enough these days.

Committed after simplifying the Makefile.

We can't simplify test_decoding's makefile to that extent, because it
uses isolationtester, which we don't provide for contrib yet...

Andres

Andres Freund <andres@anarazel.de> writes:
> Committed after simplifying the Makefile.

While I have no particular objection to adding these tests, the
commit log's claim that this will improve buildfarm testing is
quite wrong.  The buildfarm runs "make installcheck" not
"make check" in contrib.  What I'm actually seeing in the
buildfarm reports is

make -C pg_stat_statements installcheck
make -C ../../src/test/regress pg_regress
make -C ../../../src/port all
make -C ../backend submake-errcodes
make[4]: Nothing to be done for `submake-errcodes'.
make -C ../../../src/common all
make -C ../backend submake-errcodes
make[4]: Nothing to be done for `submake-errcodes'.
../../src/test/regress/pg_regress --inputdir=. --bindir='/Users/buildfarm/bf-data/HEAD/inst/bin'   --port=5678
--temp-config../../contrib/pg_stat_statements/pg_stat_statements.conf --dbname=contrib_regression_pg_stat_statements  
(using postmaster on Unix socket, port 5678)
============== dropping database "contrib_regression_pg_stat_statements" ==============
NOTICE:  database "contrib_regression_pg_stat_statements" does not exist, skipping
DROP DATABASE
============== creating database "contrib_regression_pg_stat_statements" ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries        ==============

=====================All 0 tests passed.
=====================

which is a rather blatant waste of cycles.  I would suggest an explicit
do-nothing installcheck rule rather than the hack you came up with here.
        regards, tom lane

On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Committed after simplifying the Makefile.
> 
> While I have no particular objection to adding these tests, the
> commit log's claim that this will improve buildfarm testing is
> quite wrong.  The buildfarm runs "make installcheck" not
> "make check" in contrib.

Gah.  It was more aimed at the coverage stuff, but that might work the
same. Alvaro?

> which is a rather blatant waste of cycles. I would suggest an explicit
> do-nothing installcheck rule rather than the hack you came up with here.

I had that at first, but that generates a warning about overwriting the
makefile target - which afaics cannot be fixed.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
>> which is a rather blatant waste of cycles. I would suggest an explicit
>> do-nothing installcheck rule rather than the hack you came up with here.

> I had that at first, but that generates a warning about overwriting the
> makefile target - which afaics cannot be fixed.

Hm.  What about inventing an additional macro NO_INSTALLCHECK that
prevents pgxs.mk from generating an installcheck rule?  It's not
like we don't have similar issues elsewhere, eg contrib/sepgsql.
        regards, tom lane

On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
> >> which is a rather blatant waste of cycles. I would suggest an explicit
> >> do-nothing installcheck rule rather than the hack you came up with here.
> 
> > I had that at first, but that generates a warning about overwriting the
> > makefile target - which afaics cannot be fixed.
> 
> Hm.  What about inventing an additional macro NO_INSTALLCHECK that
> prevents pgxs.mk from generating an installcheck rule?

That'd work. I'd also be ok with living with the warning.  I have to say
I find it hard to be concerned about the cycles here. It's not like
anybody complained about make check unconditionally creating a test
installation, even if there's tests in a contrib module...

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
>> Hm.  What about inventing an additional macro NO_INSTALLCHECK that
>> prevents pgxs.mk from generating an installcheck rule?

> That'd work. I'd also be ok with living with the warning.  I have to say
> I find it hard to be concerned about the cycles here. It's not like
> anybody complained about make check unconditionally creating a test
> installation, even if there's tests in a contrib module...

[ shrug... ]  The reason why the buildfarm doesn't do "make check" here is
precisely the extra cycles it would take.  Those add up over hundreds of
runs, particularly on slow machines.  So I'm not excited about running
completely useless operations.
        regards, tom lane

On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
> >> which is a rather blatant waste of cycles. I would suggest an explicit
> >> do-nothing installcheck rule rather than the hack you came up with here.
> 
> > I had that at first, but that generates a warning about overwriting the
> > makefile target - which afaics cannot be fixed.
> 
> Hm.  What about inventing an additional macro NO_INSTALLCHECK that
> prevents pgxs.mk from generating an installcheck rule?  It's not
> like we don't have similar issues elsewhere, eg contrib/sepgsql.

Well, that one seems a bit different.  Seems easy enough to do. Do we
want to make that macro that prevents installcheck from being defined,
or one that forces it to be empty? The former has the disadvantage that
one has to be careful to define a target, to avoid breaking recursion
from the upper levels.

Greetings,

Andres Freund

On 2016-11-14 12:14:10 -0800, Andres Freund wrote:
> On 2016-11-12 11:42:12 -0500, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
> > >> which is a rather blatant waste of cycles. I would suggest an explicit
> > >> do-nothing installcheck rule rather than the hack you came up with here.
> >
> > > I had that at first, but that generates a warning about overwriting the
> > > makefile target - which afaics cannot be fixed.
> >
> > Hm.  What about inventing an additional macro NO_INSTALLCHECK that
> > prevents pgxs.mk from generating an installcheck rule?  It's not
> > like we don't have similar issues elsewhere, eg contrib/sepgsql.
>
> Well, that one seems a bit different.  Seems easy enough to do. Do we
> want to make that macro that prevents installcheck from being defined,
> or one that forces it to be empty? The former has the disadvantage that
> one has to be careful to define a target, to avoid breaking recursion
> from the upper levels.

Oh, that disadvantage doesn't actually exist, because installcheck is a
.PHONY target...

I've for now not added a variant that prevents plain 'make check' from
doing anything. I guess it could be useful when a contrib module wants
to run tests via something else than pg_regress?

Patch attached.

Andres

Andres Freund wrote:
> On 2016-11-12 11:30:42 -0500, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > Committed after simplifying the Makefile.
> > 
> > While I have no particular objection to adding these tests, the
> > commit log's claim that this will improve buildfarm testing is
> > quite wrong.  The buildfarm runs "make installcheck" not
> > "make check" in contrib.
> 
> Gah.  It was more aimed at the coverage stuff, but that might work the
> same. Alvaro?

Already replied on IM, but for the record, coverage.postgresql.org
currently runs this:

make -j4 >> $LOG 2>&1
make check-world >> $LOG 2>&1
make -C src/test/ssl check >> $LOG 2>&1

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services