Thread: 9.6 TAP tests and extensions

9.6 TAP tests and extensions

From
Craig Ringer
Date:
Hi all

While updating an extension for 9.6 I noticed that while the
$(prove_check) definition is exposed for use by PGXS in
Makefile.global, extensions can't actually use the TAP tests because
we don't install the required Perl modules like PostgresNode.pm.

I don't see any reason not to make this available to extension authors
and doing so is harmless, so here's a small patch to install it. I
think it's reasonable to add this to 9.6 even at this late stage; IMO
it should've been installed from the beginning. They're only installed
if --enable-tap-tests is set, since otherwise $(prove_check) will just
error out with "TAP tests not enabled" anyway.

Not having this in 9.6 will make it way harder for extension authors
to benefit from the new TAP tooling.

Another patch allows the isolation tester to be installed too, again
so that extensions can use it.

The final patch just adds a comment to src/test/Makefile to warn that
src/Makefile doesn't call it directly, because I was confused as to
why 'make -C src/test install' installed everything, but 'make
install' did not.

Sorry this is so late in the piece. It only came up when I switched
from 10.0 dev/review to updating extensions for 9.6. But it's just
adding installed files and I think it's worth doing.

Another small patch (pending) will be needed because we look for
pg_regress in the wrong place when running out-of-tree with
$(prove_installcheck).

Can't exec
"/home/craig/projects/2Q/postgres-bdr-extension//home/craig/pg/96/lib/postgresql/pgxs/src/makefiles/../../src/test/regress/pg_regress":
No such file or directory at
/home/craig/pg/96/lib/postgresql/pgxs/src/makefiles/../../src/test/perl/TestLib.pm
line 151.

$(prove_check) won't be usable because it assumes a temp install in
./tmp_install but that's to be expected.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
This was wrong for out-of-tree builds, updated.

Still pending fix for PG_REGRESS path when invoked using
$(prove_check) from PGXS


--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 13 September 2016 at 13:27, Craig Ringer <craig@2ndquadrant.com> wrote:
> This was wrong for out-of-tree builds, updated.
>
> Still pending fix for PG_REGRESS path when invoked using
> $(prove_check) from PGXS

Looking further at this, I think a pgxs-specific patch to add support
for prove tests and isolation tests would be best, and can be done
separately. Possibly even snuck into a point release, but if not, at
least extension authors can invoke prove in their own Makefile if the
required modules get installed. It just needs an adaptation of the
command used in the $(prove_check) definition.

Extension makefiles run tests by listing the tests in REGRESS .
Something similar would make sense for isolation checks. For prove,
probably just a macro that can be invoked to enable prove tests in
pgxs makefiles.

I suggest that the above patches be applied to 9.6 and v10. Then for
v10 I'll look at enhancing PGXS to make it easier to use isolation
tests and prove tests; extensions that want to use them in 9.6 can
just add something like:


prove_check:       rm -rf $(CURDIR)/tmp_check/log       cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell pg_config
--bindir):$$PATH" PGPORT='6$(DEF_PGPORT)'
top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='pg_regress'
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl

.PHONY: prove_check



to their Makefile , so it's not necessary to have PGXS support for
this for it to be useful in 9.6.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 13 September 2016 at 14:36, Craig Ringer <craig@2ndquadrant.com> wrote:

>
>
> prove_check:
>         rm -rf $(CURDIR)/tmp_check/log
>         cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell pg_config
> --bindir):$$PATH" PGPORT='6$(DEF_PGPORT)'
> top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='pg_regress'
> $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
>
> .PHONY: prove_check

Actually, that should be


prove_check:       rm -rf $(CURDIR)/tmp_check/log       cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(shell $(PG_CONFIG)
--bindir):$$PATH" PGPORT='6$(DEF_PGPORT)'
top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(pgxsdir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl


for a typical install.

If PGXS defined that as a var that can be invoked with $(PGXS_PROVE)
or something that'd be handy, but can be done later. It's trivial to
do in an extension Makefile so long as the required files actually get
installed.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: 9.6 TAP tests and extensions

From
Alvaro Herrera
Date:
Craig Ringer wrote:

> I suggest that the above patches be applied to 9.6 and v10. Then for
> v10

I don't object to patching 9.6 in this way, but kindly do not pollute
this thread with future ideas on what to do on pg10, at least until the
current release is sorted out.  You'll only distract people from your
present objective.

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



Re: 9.6 TAP tests and extensions

From
Tom Lane
Date:
Craig Ringer <craig@2ndquadrant.com> writes:
> While updating an extension for 9.6 I noticed that while the
> $(prove_check) definition is exposed for use by PGXS in
> Makefile.global, extensions can't actually use the TAP tests because
> we don't install the required Perl modules like PostgresNode.pm.

> I don't see any reason not to make this available to extension authors
> and doing so is harmless, so here's a small patch to install it. I
> think it's reasonable to add this to 9.6 even at this late stage; IMO
> it should've been installed from the beginning.

Without taking a position on the merits of this patch per se, I'd like
to say that I find the argument for back-patching into 9.6 and not
further than that to be pretty dubious.  $(prove_check) has been there
since 9.4, and in the past we've often regretted it when we failed
to back-patch TAP infrastructure fixes all the way back to 9.4.

Or to be concrete: how is an extension author, or more to the point an
extension Makefile, supposed to know whether it can use $(prove_check)?
How would this patch change that, and how would extension authors cope
with building against both patched and unpatched trees?  
        regards, tom lane



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> While updating an extension for 9.6 I noticed that while the
>> $(prove_check) definition is exposed for use by PGXS in
>> Makefile.global, extensions can't actually use the TAP tests because
>> we don't install the required Perl modules like PostgresNode.pm.

Whoops, I managed to misplace this thread.

>> I don't see any reason not to make this available to extension authors
>> and doing so is harmless, so here's a small patch to install it. I
>> think it's reasonable to add this to 9.6 even at this late stage; IMO
>> it should've been installed from the beginning.
>
> Without taking a position on the merits of this patch per se, I'd like
> to say that I find the argument for back-patching into 9.6 and not
> further than that to be pretty dubious.  $(prove_check) has been there
> since 9.4, and in the past we've often regretted it when we failed
> to back-patch TAP infrastructure fixes all the way back to 9.4.

No objection to backpatching, I just thought I'd be more intrusive to
do that than just 9.6.

Since 9.5 and older have more limited versions of PostgresNode which
lack safe_psql, etc, I'm not sure it's very practical for extensions
to bother running TAP tests on 9.4 and 9.5 anyway.

I'd love to be able to, but unless we backport the new src/test/perl
stuff and the changes to the rest of the TAP tests to make them work
with it I don't see it being very useful. Since that's really not
going to fly, I say this should just go in 9.6.

Extension authors can just use:

ifeq ($(MAJORVERSION),9.6)
endif

when defining their prove rules.

Not beautiful, but when it wasn't really designed from the start to
work with PGXS I don't see much alternative.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: 9.6 TAP tests and extensions

From
Tom Lane
Date:
Craig Ringer <craig@2ndquadrant.com> writes:
> On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Without taking a position on the merits of this patch per se, I'd like
>> to say that I find the argument for back-patching into 9.6 and not
>> further than that to be pretty dubious.  $(prove_check) has been there
>> since 9.4, and in the past we've often regretted it when we failed
>> to back-patch TAP infrastructure fixes all the way back to 9.4.

> No objection to backpatching, I just thought I'd be more intrusive to
> do that than just 9.6.

> Since 9.5 and older have more limited versions of PostgresNode which
> lack safe_psql, etc, I'm not sure it's very practical for extensions
> to bother running TAP tests on 9.4 and 9.5 anyway.

Certainly there are restrictions, but I'd imagine that every new release
will be adding features to the TAP test infrastructure for some time to
come.  I think it's silly to claim that 9.6 is the first branch where
TAP testing is usable at all.

> Extension authors can just use:
> ifeq ($(MAJORVERSION),9.6)
> endif
> when defining their prove rules.

That will break as soon as 10 comes out.  And numerical >= tests aren't
all that convenient in Make.  It'd be much better if a test on whether
$(prove_check) is defined would be sufficient.
        regards, tom lane



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 23 September 2016 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> On 13 September 2016 at 22:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Without taking a position on the merits of this patch per se, I'd like
>>> to say that I find the argument for back-patching into 9.6 and not
>>> further than that to be pretty dubious.  $(prove_check) has been there
>>> since 9.4, and in the past we've often regretted it when we failed
>>> to back-patch TAP infrastructure fixes all the way back to 9.4.
>
>> No objection to backpatching, I just thought I'd be more intrusive to
>> do that than just 9.6.
>
>> Since 9.5 and older have more limited versions of PostgresNode which
>> lack safe_psql, etc, I'm not sure it's very practical for extensions
>> to bother running TAP tests on 9.4 and 9.5 anyway.
>
> Certainly there are restrictions, but I'd imagine that every new release
> will be adding features to the TAP test infrastructure for some time to
> come.  I think it's silly to claim that 9.6 is the first branch where
> TAP testing is usable at all.

OK.

I didn't intend to claim 9.6 is the first branch where it's usable,
just that the differences between 9.4/9.5 and 9.6 mean that supporting
both in extensions will likely be more pain than it is worth. Mainly
due to the safe_psql change in 2c83f435. It might've been good to
backport that, but it was pretty wide reaching and at the time I
wasn't thinking in terms of using TAP tests in extensions so it didn't
occur to me.

Extension Perl code can always use some adapter/glue code to handle
9.4 and 9.5 if they want, or error if they don't, so it's not a fatal
barrier.

Also, despite what I said upthread, there's no need for normal
PGXS-using extensions to define their $(prove_installcheck)
replacement. It works as-is. The problems I was having stemmed from
the fact that I was working with a pretty nonstandard Makefile without
realising that the changes were going to affect prove. $(prove_check)
isn't useful from extensions of course since it expects a temp install
and PGXS doesn't know how to make one, but $(prove_installcheck) does
the job fine.

It's thus sufficient to apply the patch to install the perl modules to
9.4, 9.5 and 9.6. Nothing else is needed. I've attached backports for
9.4 and 9.5.

If you were really keen, we could actually backport the new TAP code
to 9.4 and 9.5 pretty much wholesale. 9.4 and 9.5 don't have the psql
method, PostgresNode, etc, so there's nothing to break. But that's for
separate consideration.

>> Extension authors can just use:
>> ifeq ($(MAJORVERSION),9.6)
>> endif
>> when defining their prove rules.
>
> That will break as soon as 10 comes out.  And numerical >= tests aren't
> all that convenient in Make.  It'd be much better if a test on whether
> $(prove_check) is defined would be sufficient.

Fair enough. I forgot how much numeric range tests sucked in Make.

In that case we should backpatch the installation of the Perl modules
right back to 9.4. There's not even a need to test if
$(prove_installcheck) is defined then, just need a semicolon so it
evaluates to empty e.g.

prove_installcheck:
    $(prove_installcheck) ;

check: prove_check



So ... patches attached. The 9.6 patch applies to head too.


--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: 9.6 TAP tests and extensions

From
Tom Lane
Date:
Craig Ringer <craig@2ndquadrant.com> writes:
> On 23 September 2016 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Certainly there are restrictions, but I'd imagine that every new release
>> will be adding features to the TAP test infrastructure for some time to
>> come.  I think it's silly to claim that 9.6 is the first branch where
>> TAP testing is usable at all.

> Also, despite what I said upthread, there's no need for normal
> PGXS-using extensions to define their $(prove_installcheck)
> replacement. It works as-is. The problems I was having stemmed from
> the fact that I was working with a pretty nonstandard Makefile without
> realising that the changes were going to affect prove. $(prove_check)
> isn't useful from extensions of course since it expects a temp install
> and PGXS doesn't know how to make one, but $(prove_installcheck) does
> the job fine.

> It's thus sufficient to apply the patch to install the perl modules to
> 9.4, 9.5 and 9.6. Nothing else is needed. I've attached backports for
> 9.4 and 9.5.

Pushed with cosmetic adjustments --- mostly, I followed the project
standard that makefiles are named Makefile.  (We use a GNUmakefile
only in directories where it seems likely that non-developers would
invoke make by hand, and there's supposed to be a Makefile beside them
that throws an error whining about how you're not using GNU make.
I see no need for that in src/test/perl.)

Looking back over the thread, I see that you also proposed installing
isolationtester and pg_isolation_regress for the benefit of extensions.
I'm very much less excited about that idea.  It'd be substantially more
dead weight in typical installations, and I'm not sure that it'd be useful
to common extensions, and I'm not eager to treat isolationtester's API
and behavior as something we need to hold stable for extension use.
        regards, tom lane



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
<p dir="ltr"><p dir="ltr">On 24 Sep. 2016 04:04, "Tom Lane" <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:.<br /> ><br /> > > <br /> > > It's thus
sufficientto apply the patch to install the perl modules to<br /> > > 9.4, 9.5 and 9.6. Nothing else is needed.
I'veattached backports for<br /> > > 9.4 and 9.5.<br /> ><br /> > Pushed with cosmetic adjustments --- <p
dir="ltr">Thanks.<pdir="ltr">> Looking back over the thread, I see that you also proposed installing<br /> >
isolationtesterand pg_isolation_regress for the benefit of extensions.<br /> > I'm very much less excited about that
idea. It'd be substantially more<br /> > dead weight in typical installations, and I'm not sure that it'd be
useful<br/> > to common extensions, and I'm not eager to treat isolationtester's API<br /> > and behavior as
somethingwe need to hold stable for extension use.<p dir="ltr">Fine by me. I have no particular problem expecting those
tobe installed explicitly by ext devs who want to use them.<br /> 

Re: 9.6 TAP tests and extensions

From
Andres Freund
Date:
On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
> Looking back over the thread, I see that you also proposed installing
> isolationtester and pg_isolation_regress for the benefit of extensions.
> I'm very much less excited about that idea.  It'd be substantially more
> dead weight in typical installations, and I'm not sure that it'd be useful
> to common extensions, and I'm not eager to treat isolationtester's API
> and behavior as something we need to hold stable for extension use.

FWIW, I'd be quite happy if it were installed. Running isolationtester
when compiling extensions against distribution postgres packages would
be quite useful.

Andres



Re: 9.6 TAP tests and extensions

From
Robert Haas
Date:
On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
>> Looking back over the thread, I see that you also proposed installing
>> isolationtester and pg_isolation_regress for the benefit of extensions.
>> I'm very much less excited about that idea.  It'd be substantially more
>> dead weight in typical installations, and I'm not sure that it'd be useful
>> to common extensions, and I'm not eager to treat isolationtester's API
>> and behavior as something we need to hold stable for extension use.
>
> FWIW, I'd be quite happy if it were installed. Running isolationtester
> when compiling extensions against distribution postgres packages would
> be quite useful.

+1.

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



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
>>> Looking back over the thread, I see that you also proposed installing
>>> isolationtester and pg_isolation_regress for the benefit of extensions.
>>> I'm very much less excited about that idea.  It'd be substantially more
>>> dead weight in typical installations, and I'm not sure that it'd be useful
>>> to common extensions, and I'm not eager to treat isolationtester's API
>>> and behavior as something we need to hold stable for extension use.
>>
>> FWIW, I'd be quite happy if it were installed. Running isolationtester
>> when compiling extensions against distribution postgres packages would
>> be quite useful.
>
> +1.

Patch attached.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 14 November 2016 at 14:55, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:
>>> On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
>>>> Looking back over the thread, I see that you also proposed installing
>>>> isolationtester and pg_isolation_regress for the benefit of extensions.
>>>> I'm very much less excited about that idea.  It'd be substantially more
>>>> dead weight in typical installations, and I'm not sure that it'd be useful
>>>> to common extensions, and I'm not eager to treat isolationtester's API
>>>> and behavior as something we need to hold stable for extension use.
>>>
>>> FWIW, I'd be quite happy if it were installed. Running isolationtester
>>> when compiling extensions against distribution postgres packages would
>>> be quite useful.
>>
>> +1.
>
> Patch attached.

As Andres pointed out elsewhere this is only half the picture. It
should really add PGXS support for ISOLATION_TESTS and bringing up the
test harness too.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: 9.6 TAP tests and extensions

From
Alvaro Herrera
Date:
Craig Ringer wrote:
> On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:
> >> On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
> >>> Looking back over the thread, I see that you also proposed installing
> >>> isolationtester and pg_isolation_regress for the benefit of extensions.
> >>> I'm very much less excited about that idea.  It'd be substantially more
> >>> dead weight in typical installations, and I'm not sure that it'd be useful
> >>> to common extensions, and I'm not eager to treat isolationtester's API
> >>> and behavior as something we need to hold stable for extension use.
> >>
> >> FWIW, I'd be quite happy if it were installed. Running isolationtester
> >> when compiling extensions against distribution postgres packages would
> >> be quite useful.
> >
> > +1.
> 
> Patch attached.

Hmm but this only installs isolationtester itself ... don't you need
pg_isolation_regress too?

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



Re: 9.6 TAP tests and extensions

From
Craig Ringer
Date:
On 25 November 2016 at 02:47, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Craig Ringer wrote:
>> On 27 October 2016 at 00:42, Robert Haas <robertmhaas@gmail.com> wrote:
>> > On Wed, Oct 26, 2016 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:
>> >> On 2016-09-23 16:04:32 -0400, Tom Lane wrote:
>> >>> Looking back over the thread, I see that you also proposed installing
>> >>> isolationtester and pg_isolation_regress for the benefit of extensions.
>> >>> I'm very much less excited about that idea.  It'd be substantially more
>> >>> dead weight in typical installations, and I'm not sure that it'd be useful
>> >>> to common extensions, and I'm not eager to treat isolationtester's API
>> >>> and behavior as something we need to hold stable for extension use.
>> >>
>> >> FWIW, I'd be quite happy if it were installed. Running isolationtester
>> >> when compiling extensions against distribution postgres packages would
>> >> be quite useful.
>> >
>> > +1.
>>
>> Patch attached.
>
> Hmm but this only installs isolationtester itself ... don't you need
> pg_isolation_regress too?

Yeah, as Andres pointed out offlist after I posted this. Meant to
follow up but got side-tracked.

It needs PGXS support to be properly useful.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services