Re: no test programs in contrib

What's the general opinion on having test programs somewhere other than
contrib/ ?

We currently have a number of subdirectories for test-only programs:

test_parser (a toy text search parser, added in 2007)
dummy_seclabel (for SECURITY LABEL regression testing, added Sept 2010)
worker_spi (for bgworkers, added Dec 2012)
test_shm_mq (test program for shared memory queues, added Jan 2014)
test_decoding (test program for logical decoding, added March 2014)

I'm now contemplating the addition on a new one in the commit-timestamps
patch, and I'm starting to feel that these are all misplaced. I think
we have been dumping them to contrib not because they really belong
there, but because of the lack of a better place. As opposed to the
rest of the stuff in contrib/, they don't serve any useful purpose on
themselves; they are just demonstrating some coding techniques, or
testing that some framework work as intended. It seems impolite to
continue to pollute contrib with these; and my crystal ball says they
will continue to grow much more rapidly than normal, useful contrib
modules.

What would you say if we were to move them to src/test/? I could also
see putting them in a brand new top-level directory, say testing/ or
testprg/.

Now, I know there is some resistance to the idea of moving source code
around. If this proposal is objected to, would people object the idea
of putting the commit timestamp test module in src/test/commit_ts
instead of the patch author's proposal, contrib/test_committs?

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

On 24/11/14 14:49, Alvaro Herrera wrote:
> I'm now contemplating the addition on a new one in the commit-timestamps
> patch, and I'm starting to feel that these are all misplaced. I think
> we have been dumping them to contrib not because they really belong
> there, but because of the lack of a better place. As opposed to the
> rest of the stuff in contrib/, they don't serve any useful purpose on
> themselves; they are just demonstrating some coding techniques, or
> testing that some framework work as intended. It seems impolite to
> continue to pollute contrib with these; and my crystal ball says they
> will continue to grow much more rapidly than normal, useful contrib
> modules.
>

Completely agree.

> What would you say if we were to move them to src/test/? I could also
> see putting them in a brand new top-level directory, say testing/ or
> testprg/.
>
> Now, I know there is some resistance to the idea of moving source code
> around. If this proposal is objected to, would people object the idea
> of putting the commit timestamp test module in src/test/commit_ts
> instead of the patch author's proposal, contrib/test_committs?
>

I'd go for src/test, but I think common subdirectory there is needed
(src/test/<something>/commit_ts). Not sure what the <something> could
be, maybe something like "standalone" as those tests get their own pg
instance?

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

Hi,

On 2014-11-24 10:49:45 -0300, Alvaro Herrera wrote:
> I'm now contemplating the addition on a new one in the commit-timestamps
> patch, and I'm starting to feel that these are all misplaced. I think
> we have been dumping them to contrib not because they really belong
> there, but because of the lack of a better place.

Agreed.

> As opposed to the
> rest of the stuff in contrib/, they don't serve any useful purpose on
> themselves; they are just demonstrating some coding techniques, or
> testing that some framework work as intended.

I actually think that test_decoding is somewhat useful in other cases as
well, so it might be prudent to leave it there.

> What would you say if we were to move them to src/test/? I could also
> see putting them in a brand new top-level directory, say testing/ or
> testprg/.

src/test/ is good, but I think there should be another subdirectory
inside. testcases/?

Greetings,

Andres Freund

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> We currently have a number of subdirectories for test-only programs:

> test_parser (a toy text search parser, added in 2007)
> dummy_seclabel (for SECURITY LABEL regression testing, added Sept 2010)
> worker_spi (for bgworkers, added Dec 2012)
> test_shm_mq (test program for shared memory queues, added Jan 2014)
> test_decoding (test program for logical decoding, added March 2014)

> What would you say if we were to move them to src/test/? I could also
> see putting them in a brand new top-level directory, say testing/ or
> testprg/.

I think that test_parser is arguably useful as a skeleton/example for
user-written TS parsers, so I'd lean towards leaving it where it is,
but the others could move to src/test/ IMO.

> Now, I know there is some resistance to the idea of moving source code
> around.

Usually that's when there is (a) a lot of history and (b) concern about
back-patching fixes. Neither of those arguments seem real strong for
these modules, with the possible exception of test_parser.

regards, tom lane

On Mon, Nov 24, 2014 at 10:49:45AM -0300, Alvaro Herrera wrote:
> What's the general opinion on having test programs somewhere other than
> contrib/ ?

General opinion: slightly favorable.

> We currently have a number of subdirectories for test-only programs:
>
> test_parser (a toy text search parser, added in 2007)
> dummy_seclabel (for SECURITY LABEL regression testing, added Sept 2010)
> worker_spi (for bgworkers, added Dec 2012)
> test_shm_mq (test program for shared memory queues, added Jan 2014)
> test_decoding (test program for logical decoding, added March 2014)

> What would you say if we were to move them to src/test/? I could also
> see putting them in a brand new top-level directory, say testing/ or
> testprg/.

It's revealing that two of the first three responses each doubted the fit of
one of those moves. I think that shows the lines aren't so bright after all,
and this specific proposal is not strong enough. The line between a test
module and a sample-code module is blurry.

> Now, I know there is some resistance to the idea of moving source code
> around. If this proposal is objected to, would people object the idea
> of putting the commit timestamp test module in src/test/commit_ts
> instead of the patch author's proposal, contrib/test_committs?

I'd rather defend moving source code or defend continuing to dump in contrib
than defend a src/test/modules defined as "test-oriented modules added after
November 2014."

Incidentally, +1 on "test_commit_ts" in preference to "test_committs".

On 11/24/14 8:49 AM, Alvaro Herrera wrote:
> What would you say if we were to move them to src/test/?

Yes please.

> Now, I know there is some resistance to the idea of moving source code
> around.

I think clarifying "contrib" is more important than that.

On 11/24/14 9:35 AM, Andres Freund wrote:
> I actually think that test_decoding is somewhat useful in other cases as
> well, so it might be prudent to leave it there.

For what?

> src/test/ is good, but I think there should be another subdirectory
> inside. testcases/?

What tests are not test cases?

On 11/24/14 10:46 AM, Tom Lane wrote:
> I think that test_parser is arguably useful as a skeleton/example for
> user-written TS parsers, so I'd lean towards leaving it where it is,
> but the others could move to src/test/ IMO.

I think a useful dividing line would be, is it normally useful to
install? A skeleton is still useful if it is in a different place in
the source tree (arguably more useful). It's not useful if it's
installed as a *.so.

> Usually that's when there is (a) a lot of history and (b) concern about
> back-patching fixes. Neither of those arguments seem real strong for
> these modules, with the possible exception of test_parser.

Have we ever really tried to use the various git options that are meant
to help with that (in a recent git version)?

(If not, now we'd have a chance to try.)

On 2014-11-25 16:07:52 -0500, Peter Eisentraut wrote:
> On 11/24/14 9:35 AM, Andres Freund wrote:
> > I actually think that test_decoding is somewhat useful in other cases as
> > well, so it might be prudent to leave it there.
>
> For what?
>
> > src/test/ is good, but I think there should be another subdirectory
> > inside. testcases/?
>
> What tests are not test cases?

There's infrastructure for tests in there already. It seems like a bad
idea to me to have individual tests on the same level as
src/test/regress and src/test/isolation.

regress/

Greetings,

Andres Freund

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

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 11/24/14 10:46 AM, Tom Lane wrote:
>> I think that test_parser is arguably useful as a skeleton/example for
>> user-written TS parsers, so I'd lean towards leaving it where it is,
>> but the others could move to src/test/ IMO.

> I think a useful dividing line would be, is it normally useful to
> install? A skeleton is still useful if it is in a different place in
> the source tree (arguably more useful). It's not useful if it's
> installed as a *.so.

I agree that where it is in the source tree isn't all that exciting
(for any purpose other than back-patching). What is exciting is what
the context and build infrastructure look like. The fact that test_parser
is packaged as a .so and can be built with PGXS makes it very easy to copy
as a skeleton for a user-written parser --- you don't need to invent your
own Makefile, in particular. Now, maybe we'd retain those properties if
it were under src/test/, but that was not immediately clear to me.

Agreed that it shouldn't be installed as part of a standard binary
distribution, though. We could potentially fix that while keeping it
in contrib, but maybe relocating it would be clearer.

What do we do about docs, though? These things do need some user-facing
docs, or people won't even know they exist to be copied.

regards, tom lane

Here's a patch. This creates a new subdir src/test/modules and places
the five initially proposed modules in there. They continue to have
their makefile with the same ifdef USE_PGXS pattern; they are no longer
installed by default.

Because many of them had either "test" in their names or some other
now-useless particle, I renamed them:

worker_spi -> bgworker
test_decoding -> logical_decoding
dummy_seclabel -> seclabel
test_shm_mq -> shm_mq
test_parser -> tsparser

The renaming is not complete: the extensions continue to have the old
names, for instance. If the consensus is to rename them completely I
can finish that, or we can decide to keep the original names, but they
all seem inappropriate to me.

I haven't done anything about documentation. I thought a new chapter
after "Additional Supplied Modules", perhaps entitled "Additional Sample
Modules" would be appropriate.

I tweaked make targets check, installcheck, installcheck-world,
check-world: they all run the additional tests now. For buildfarm, the
client code will need to be updated to have a new stage for
src/test/modules running "make check".

I haven't touched MSVC yet.

Opinions on this approach please?

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

On Wed, Nov 26, 2014 at 9:27 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Here's a patch. This creates a new subdir src/test/modules and places
> the five initially proposed modules in there. They continue to have
> their makefile with the same ifdef USE_PGXS pattern; they are no longer
> installed by default.
>
> Because many of them had either "test" in their names or some other
> now-useless particle, I renamed them:
>
> worker_spi -> bgworker
> test_decoding -> logical_decoding
> dummy_seclabel -> seclabel
> test_shm_mq -> shm_mq
> test_parser -> tsparser
>
> The renaming is not complete: the extensions continue to have the old
> names, for instance. If the consensus is to rename them completely I
> can finish that, or we can decide to keep the original names, but they
> all seem inappropriate to me.
>
> I haven't done anything about documentation. I thought a new chapter
> after "Additional Supplied Modules", perhaps entitled "Additional Sample
> Modules" would be appropriate.
>
> I tweaked make targets check, installcheck, installcheck-world,
> check-world: they all run the additional tests now. For buildfarm, the
> client code will need to be updated to have a new stage for
> src/test/modules running "make check".
>
> I haven't touched MSVC yet.
>
> Opinions on this approach please?

I like the move. I dislike the renaming.

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

On Wed, Nov 26, 2014 at 12:27 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
>
> Here's a patch. This creates a new subdir src/test/modules and places
> the five initially proposed modules in there. They continue to have
> their makefile with the same ifdef USE_PGXS pattern; they are no longer
> installed by default.
>
> Because many of them had either "test" in their names or some other
> now-useless particle, I renamed them:
>
> worker_spi -> bgworker
> test_decoding -> logical_decoding
> dummy_seclabel -> seclabel
> test_shm_mq -> shm_mq
> test_parser -> tsparser
>
> The renaming is not complete: the extensions continue to have the old
> names, for instance. If the consensus is to rename them completely I
> can finish that, or we can decide to keep the original names, but they
> all seem inappropriate to me.
>
> I haven't done anything about documentation. I thought a new chapter
> after "Additional Supplied Modules", perhaps entitled "Additional Sample
> Modules" would be appropriate.
>
> I tweaked make targets check, installcheck, installcheck-world,
> check-world: they all run the additional tests now. For buildfarm, the
> client code will need to be updated to have a new stage for
> src/test/modules running "make check".
>
> I haven't touched MSVC yet.
>
> Opinions on this approach please?
>

The patch is missing...

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

This is pretty bulky, but really the vast majority of the changes here
are just "git mv".

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> This is pretty bulky, but really the vast majority of the changes here
> are just "git mv".

For ease of review, is there a way to get git to show just the diffs that
*aren't* git mv? (That is, show changes in a file's content without
respect to its having moved?)

regards, tom lane

On 2014-11-26 10:08:57 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > This is pretty bulky, but really the vast majority of the changes here
> > are just "git mv".
>
> For ease of review, is there a way to get git to show just the diffs that
> *aren't* git mv? (That is, show changes in a file's content without
> respect to its having moved?)

Yes, that's possible. git diff/show/whatever -M

Greetings,

Andres Freund

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > This is pretty bulky, but really the vast majority of the changes here
> > are just "git mv".
>
> For ease of review, is there a way to get git to show just the diffs that
> *aren't* git mv? (That is, show changes in a file's content without
> respect to its having moved?)

I think git diff -D -M -B does that. Here's such a diff, which I
obtained from "git show" (otherwise you'd need to "git add" all the new
files, I think, which would be pretty annoying)

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

Robert Haas wrote:
> On Wed, Nov 26, 2014 at 9:27 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:

> > Because many of them had either "test" in their names or some other
> > now-useless particle, I renamed them:
> >
> > worker_spi -> bgworker
> > test_decoding -> logical_decoding
> > dummy_seclabel -> seclabel
> > test_shm_mq -> shm_mq
> > test_parser -> tsparser

> I like the move. I dislike the renaming.

Do you dislike the new names, or the fact that they are changing at all?

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

On 11/26/14 9:27 AM, Alvaro Herrera wrote:
> I haven't done anything about documentation. I thought a new chapter
> after "Additional Supplied Modules", perhaps entitled "Additional Sample
> Modules" would be appropriate.

I would remove the SGML files and put simple README files into each
directory.

Peter Eisentraut wrote:
> On 11/26/14 9:27 AM, Alvaro Herrera wrote:
> > I haven't done anything about documentation. I thought a new chapter
> > after "Additional Supplied Modules", perhaps entitled "Additional Sample
> > Modules" would be appropriate.
>
> I would remove the SGML files and put simple README files into each
> directory.

They are so small that it makes sense to do it like that. Here's a
patch for this.

I have also changed things so that:

1. test modules are not installed by "make install", not checked by
"make installcheck", not checked by "make check".

2. test modules are checked by "make check-world" (this is consistent
with handling of contrib).

3. test modules are checked by "make installcheck-world" (this is
consistent with handling of contrib)

4. test modules are installed by "make install-world". This is
consistent with contrib, and it's necessary so that "make
installcheck-world" passes.

I moved the contents from SGML files into READMEs, and removed the
references from other SGML files, turning them into
<filename>contrib/</> instead (these are release-9.4 and so on, which is
why I reference the old locations. I assume release-9.5 will mention
the moves).

There's some untested new code in vcregress.pl, but nothing else
about msvc has been done.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I have also changed things so that:

> 1. test modules are not installed by "make install", not checked by
> "make installcheck", not checked by "make check".

> 2. test modules are checked by "make check-world" (this is consistent
> with handling of contrib).

> 3. test modules are checked by "make installcheck-world" (this is
> consistent with handling of contrib)

> 4. test modules are installed by "make install-world". This is
> consistent with contrib, and it's necessary so that "make
> installcheck-world" passes.

I'm not too happy with that approach, because packagers are going to
tend to think they should package any files installed by install-world.
The entire point of this change, IMO, is that the test modules should
*not* get installed, certainly not by normal install targets. Being
consistent with the existing contrib packaging is exactly not what we
want.

Maybe we should only allow check-world to run these tests, and not
installcheck-world? That's kind of annoying, but what you are
doing now seems to defeat the purpose altogether.

regards, tom lane

On 11/24/2014 05:49 AM, Alvaro Herrera wrote:
> test_parser (a toy text search parser, added in 2007)
> dummy_seclabel (for SECURITY LABEL regression testing, added Sept 2010)
> worker_spi (for bgworkers, added Dec 2012)
> test_shm_mq (test program for shared memory queues, added Jan 2014)
> test_decoding (test program for logical decoding, added March 2014)

So test_decoding is fairly useful for users demonstrating that decoding
works, especially if they're also testing an external decoding module
and are unsure of where their replication problem is located, or what's
wrong with their HBA settings. For that reason it's important that
test_decoding be available via OS packages, which would give me some
reluctance to move it out of /contrib.

dummy_seclabel might serve the same purpose for users who are having
issues with SEPostgres etc. I don't know enough about it ...
Stephen/Kaigai?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> So test_decoding is fairly useful for users demonstrating that decoding
> works, especially if they're also testing an external decoding module
> and are unsure of where their replication problem is located, or what's
> wrong with their HBA settings. For that reason it's important that
> test_decoding be available via OS packages, which would give me some
> reluctance to move it out of /contrib.

If we follow that reasoning we'll end up removing nothing from contrib.
There is no reason that end users should need to be performing such
testing; anyone who does have reason to do it will have a source tree
at hand.

> dummy_seclabel might serve the same purpose for users who are having
> issues with SEPostgres etc. I don't know enough about it ...

And as for dummy_seclabel, the same applies in spades, considering
that the number of users of SEPostgres can probably be counted without
running out of fingers.

regards, tom lane

Tom Lane wrote:

> I'm not too happy with that approach, because packagers are going to
> tend to think they should package any files installed by install-world.
> The entire point of this change, IMO, is that the test modules should
> *not* get installed, certainly not by normal install targets. Being
> consistent with the existing contrib packaging is exactly not what we
> want.
>
> Maybe we should only allow check-world to run these tests, and not
> installcheck-world? That's kind of annoying, but what you are
> doing now seems to defeat the purpose altogether.

Hadn't thought of the packaging angle of this. I don't think packagers
really are as dumb as you suggest, but anyway implementing this idea
turned out to be simpler than I expected; here's a preliminary patch.

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

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Josh Berkus
> Sent: Friday, November 28, 2014 5:48 AM
> To: Alvaro Herrera; Pg Hackers
> Subject: Re: [HACKERS] no test programs in contrib
>
> On 11/24/2014 05:49 AM, Alvaro Herrera wrote:
> > test_parser (a toy text search parser, added in 2007) dummy_seclabel
> > (for SECURITY LABEL regression testing, added Sept 2010) worker_spi
> > (for bgworkers, added Dec 2012) test_shm_mq (test program for shared
> > memory queues, added Jan 2014) test_decoding (test program for logical
> > decoding, added March 2014)
>
> So test_decoding is fairly useful for users demonstrating that decoding
> works, especially if they're also testing an external decoding module and
> are unsure of where their replication problem is located, or what's wrong
> with their HBA settings. For that reason it's important that test_decoding
> be available via OS packages, which would give me some reluctance to move
> it out of /contrib.
>
> dummy_seclabel might serve the same purpose for users who are having issues
> with SEPostgres etc. I don't know enough about it ...
> Stephen/Kaigai?
>
Its original purpose is to run regression test on the platform without
selinux. So, it does not intend to use the dummy_seclabel for something
useful except for regression test.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On 11/27/14 3:10 PM, Tom Lane wrote:
> I'm not too happy with that approach, because packagers are going to
> tend to think they should package any files installed by install-world.
> The entire point of this change, IMO, is that the test modules should
> *not* get installed, certainly not by normal install targets. Being
> consistent with the existing contrib packaging is exactly not what we
> want.

I share this objection.

> Maybe we should only allow check-world to run these tests, and not
> installcheck-world? That's kind of annoying, but what you are
> doing now seems to defeat the purpose altogether.

Maybe this will have to do for now.

But the existing main regression tests are able to run against an
existing installation while using the modules autoinc.so and refint.so
without installing them. I think the problem is that we are able to
load a .so file from just about anywhere, but we can't load a full
extension in the same way. There have been discussions about that, in
the context of being able to test an externally developed extension
before/without installing it. This is pretty much the same case.

On 2014-11-27 15:51:51 -0500, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > So test_decoding is fairly useful for users demonstrating that decoding
> > works, especially if they're also testing an external decoding module
> > and are unsure of where their replication problem is located, or what's
> > wrong with their HBA settings. For that reason it's important that
> > test_decoding be available via OS packages, which would give me some
> > reluctance to move it out of /contrib.
>
> If we follow that reasoning we'll end up removing nothing from contrib.
> There is no reason that end users should need to be performing such
> testing; anyone who does have reason to do it will have a source tree
> at hand.

Actually I don't think that's true for test_decoding - there's quite a
bit of actual things that you can do with it. At the very least it's
useful to roughly measure the impact logical replication would have on a
database, but it's also helpful to look at the changes. And even if the
format isn't super nice, thanks to Robert's insistence it's actually
safely parseable if necessary.

> > dummy_seclabel might serve the same purpose for users who are having
> > issues with SEPostgres etc. I don't know enough about it ...
>
> And as for dummy_seclabel, the same applies in spades, considering
> that the number of users of SEPostgres can probably be counted without
> running out of fingers.

I agree that dummy_seclabel really doesn't have any applications besides
regression tests.

Greetings,

Andres Freund

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

On 11/27/2014 12:51 PM, Tom Lane wrote:
>> So test_decoding is fairly useful for users demonstrating that decoding
>> > works, especially if they're also testing an external decoding module
>> > and are unsure of where their replication problem is located, or what's
>> > wrong with their HBA settings. For that reason it's important that
>> > test_decoding be available via OS packages, which would give me some
>> > reluctance to move it out of /contrib.
> If we follow that reasoning we'll end up removing nothing from contrib.
> There is no reason that end users should need to be performing such
> testing; anyone who does have reason to do it will have a source tree
> at hand.

1) Decoding extension "Slony-II", installed from PGXN, won't connect
2) Install test_decoding
3) Check if decoding is working and you can connect

That's the scenario I'm looking at. It's useful for "is there something
wrong with my decoding setup or is the decoding plugin broken?"

And I can imagine quite a few users who don't have source installs
needing to check that. That doesn't mean test_decoding needs to stay in
contrib, just that it needs to be somewhere which goes into some common
package.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Peter Eisentraut wrote:
> On 11/27/14 3:10 PM, Tom Lane wrote:
> > I'm not too happy with that approach, because packagers are going to
> > tend to think they should package any files installed by install-world.
> > The entire point of this change, IMO, is that the test modules should
> > *not* get installed, certainly not by normal install targets. Being
> > consistent with the existing contrib packaging is exactly not what we
> > want.
>
> I share this objection.

Okay, the attached version does it that way.

I also attach some changes for the MSVC build stuff. I tested it and it
builds fine AFAICT, but it doesn't install because Install.pm wants to
install contrib modules from contrib/ (which seems reasonable) but my
hack adds the src/test/modules/ as contrib modules also, so Install.pm
goes bonkers. I'm not even sure *what* we're supposed to build -- there
is no distinction in these programs as there is in the makefiles about
what to install. So if some Windows developer can look into this, I'd
appreciate it.

> But the existing main regression tests are able to run against an
> existing installation while using the modules autoinc.so and refint.so
> without installing them. I think the problem is that we are able to
> load a .so file from just about anywhere, but we can't load a full
> extension in the same way. There have been discussions about that, in
> the context of being able to test an externally developed extension
> before/without installing it. This is pretty much the same case.

I'm leaving that problem for someone else to solve.

Andres Freund wrote:
> On 2014-11-27 15:51:51 -0500, Tom Lane wrote:
> >
> > If we follow that reasoning we'll end up removing nothing from contrib.
> > There is no reason that end users should need to be performing such
> > testing; anyone who does have reason to do it will have a source tree
> > at hand.
>
> Actually I don't think that's true for test_decoding - there's quite a
> bit of actual things that you can do with it. At the very least it's
> useful to roughly measure the impact logical replication would have on a
> database, but it's also helpful to look at the changes. And even if the
> format isn't super nice, thanks to Robert's insistence it's actually
> safely parseable if necessary.

Argh. Okay, the attached doesn't move test_decoding either.

I think it's fine anyway -- I'm sure we will come up with a few
additional test modules, such as the one for the commit_ts patch.

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

On Sat, Nov 29, 2014 at 5:54 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
>
> I also attach some changes for the MSVC build stuff. I tested it and it
> builds fine AFAICT, but it doesn't install because Install.pm wants to
> install contrib modules from contrib/ (which seems reasonable) but my
> hack adds the src/test/modules/ as contrib modules also, so Install.pm
> goes bonkers. I'm not even sure *what* we're supposed to build -- there
> is no distinction in these programs as there is in the makefiles about
> what to install. So if some Windows developer can look into this, I'd
> appreciate it.
>

It would be good to be consistent on Windows with what is now done on other
platforms: those modules should not be installed by default, but it would
be good to make install.pm a bit smarter with for example an option "full",
aka install server + client + test modules. Building them is worth it in
any case as they can be used with modulescheck.
My 2c.
--
Michael

Michael Paquier wrote:

> It would be good to be consistent on Windows with what is now done on other
> platforms: those modules should not be installed by default, but it would
> be good to make install.pm a bit smarter with for example an option "full",
> aka install server + client + test modules. Building them is worth it in
> any case as they can be used with modulescheck.

Sure, I agree with that.

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

On Sat, Nov 29, 2014 at 5:54 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Peter Eisentraut wrote:
>> On 11/27/14 3:10 PM, Tom Lane wrote:
>> > I'm not too happy with that approach, because packagers are going to
>> > tend to think they should package any files installed by install-world.
>> > The entire point of this change, IMO, is that the test modules should
>> > *not* get installed, certainly not by normal install targets. Being
>> > consistent with the existing contrib packaging is exactly not what we
>> > want.
>>
>> I share this objection.
>
> Okay, the attached version does it that way.
>
> I also attach some changes for the MSVC build stuff. I tested it and it
> builds fine AFAICT, but it doesn't install because Install.pm wants to
> install contrib modules from contrib/ (which seems reasonable) but my
> hack adds the src/test/modules/ as contrib modules also, so Install.pm
> goes bonkers. I'm not even sure *what* we're supposed to build -- there
> is no distinction in these programs as there is in the makefiles about
> what to install. So if some Windows developer can look into this, I'd
> appreciate it.
>
>> But the existing main regression tests are able to run against an
>> existing installation while using the modules autoinc.so and refint.so
>> without installing them. I think the problem is that we are able to
>> load a .so file from just about anywhere, but we can't load a full
>> extension in the same way. There have been discussions about that, in
>> the context of being able to test an externally developed extension
>> before/without installing it. This is pretty much the same case.
>
> I'm leaving that problem for someone else to solve.
>
> Andres Freund wrote:
>> On 2014-11-27 15:51:51 -0500, Tom Lane wrote:
>> >
>> > If we follow that reasoning we'll end up removing nothing from contrib.
>> > There is no reason that end users should need to be performing such
>> > testing; anyone who does have reason to do it will have a source tree
>> > at hand.
>>
>> Actually I don't think that's true for test_decoding - there's quite a
>> bit of actual things that you can do with it. At the very least it's
>> useful to roughly measure the impact logical replication would have on a
>> database, but it's also helpful to look at the changes. And even if the
>> format isn't super nice, thanks to Robert's insistence it's actually
>> safely parseable if necessary.
>
> Argh. Okay, the attached doesn't move test_decoding either.
>
> I think it's fine anyway -- I'm sure we will come up with a few
> additional test modules, such as the one for the commit_ts patch.
When src/test/modules is compiled directly after running ./configure,
make complains about utils/errcodes.h missing:
In file included from worker_spi.c:23:
In file included from ../../../../src/include/postgres.h:48:
../../../../src/include/utils/elog.h:69:10: fatal error:
'utils/errcodes.h' file not found
#include "utils/errcodes.h"

Shouldn't src/test/modules/Makefile includes .PHONY with
submake-errcodes like for example in the patch attached?
Regards,
--
Michael