Re: Regression tests failing if not launched on db "regression"

Hi all,

It happens that the following regression tests are failing if they are
run on a database not named "regression":
- updatable_views
- foreign_data
- sequence
Those tests are failing because some relations of information_schemas
contain information that are database-dependent. Please see the diffs
attached.

Note that this can be easily reproduced by running pg_regress with a
command of this type from src/test/regress:
./pg_regress --inputdir=. --temp-install=./tmp_check \
--top-builddir=../../.. --dlpath=. \
--schedule=./parallel_schedule \
--dbname=foo
IMHO, the regression test suite would gain in consistency and
portability if we do not refer to data that is database-dependent.

Opinions? A patch fixing that would be trivial to do, and I do not
mind writing it. Also, if we consider that as a bug, and I think it
is, the fix should be back-patched as well.
Regards,
--
Michael

On Thu, Dec 5, 2013 at 5:12 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Those tests are failing because some relations of information_schemas
> contain information that are database-dependent.
I forgot to mention that it is our QE team at VMware that reported me
a portion of this failure, and I just had to dig a little bit to find
the root cause. And for the curious people: yes, we run regressions on
customized database names.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> It happens that the following regression tests are failing if they are
> run on a database not named "regression":

This does not seem like a bug to me, although maybe we'd better update the
documentation to specify that you need to use a DB named regression.

regards, tom lane

On Thu, Dec 5, 2013 at 9:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> It happens that the following regression tests are failing if they are
>> run on a database not named "regression":
>
> This does not seem like a bug to me, although maybe we'd better update the
> documentation to specify that you need to use a DB named regression.

At the same thing, supporting it might not cost anything.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Dec 5, 2013 at 9:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>> It happens that the following regression tests are failing if they are
>>> run on a database not named "regression":

>> This does not seem like a bug to me, although maybe we'd better update the
>> documentation to specify that you need to use a DB named regression.

> At the same thing, supporting it might not cost anything.

Well, changing these specific tests today might not be terribly expensive,
but what is it that prevents more such tests from being committed next
week? Perhaps by somebody who feels current_database() should be included
in code coverage, for example?

More generally, we never have and never can promise that the regression
tests pass regardless of environment. If you turn off enable_seqscan,
for instance, you'll get a whole lot of not-terribly-exciting diffs.
I see no particular benefit to promising that the name of the regression
database isn't significant.

regards, tom lane

> On 2013/12/06, at 3:52, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Dec 5, 2013 at 9:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>> It happens that the following regression tests are failing if they are
>>>> run on a database not named "regression":
>
>>> This does not seem like a bug to me, although maybe we'd better update the
>>> documentation to specify that you need to use a DB named regression.
>
>> At the same thing, supporting it might not cost anything.
>
> Well, changing these specific tests today might not be terribly expensive,
> but what is it that prevents more such tests from being committed next
> week? Perhaps by somebody who feels current_database() should be included
> in code coverage, for example?
>
> More generally, we never have and never can promise that the regression
> tests pass regardless of environment. If you turn off enable_seqscan,
> for instance, you'll get a whole lot of not-terribly-exciting diffs.
> I see no particular benefit to promising that the name of the regression
> database isn't significant.
You got a point here. Classifying that as a "don't do that" in the documentation is fine for me as well, as I recall that --dbname has been added to pg_regress only for contrib regression support.

Regards,
--
Michael
(Sent from my mobile phone)

On Thu, 2013-12-05 at 17:12 +0900, Michael Paquier wrote:
> IMHO, the regression test suite would gain in consistency and
> portability if we do not refer to data that is database-dependent.

I once did some similar fixes (e3d9dceef62e072cf9a433ae6c74a1c5a10d94d3)
but then didn't pursue it any longer, because it would restrict what we
could actually test. I don't remember what I was trying to do there,
but why do you need to run the tests in a different database?

On Fri, Dec 6, 2013 at 12:02 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Thu, 2013-12-05 at 17:12 +0900, Michael Paquier wrote:
>> IMHO, the regression test suite would gain in consistency and
>> portability if we do not refer to data that is database-dependent.
>
> I once did some similar fixes (e3d9dceef62e072cf9a433ae6c74a1c5a10d94d3)
> but then didn't pursue it any longer, because it would restrict what we
> could actually test. I don't remember what I was trying to do there,
> but why do you need to run the tests in a different database?
I don't know, but by looking at this test code I could guess that
using a custom database name (that also changed depending on the
environment used) made errors easier to track. So, I just went ahead,
cleaned up our code to use "regression" and made things a bit smarter
for the environment information.

For the documentation patch, I propose the attached to avoid future
confusions. Comments? It might make sense to back-patch as well.

Also... An advice for the archives and other people here: never update
test output dynamically and use the existing ones. I wouldn't even
recommend adding new outputs to the existing tests except if you want
to make your maintenance a pain as you would need to track new tests
and update accordingly. Even better, submit patches if new outputs
make sense, or write new tests and break things independently as much
as you want.
--
Michael

Hi,

> For the documentation patch, I propose the attached to avoid future
> confusions. Comments? It might make sense to back-patch as well.

Compiles, didn't find any typos and I think it is comprehensible.

Looks fine for me.

Best regards,

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

On Thu, Jan 30, 2014 at 3:32 AM, Christian Kruse
<christian(at)2ndquadrant(dot)com> wrote:
>> For the documentation patch, I propose the attached to avoid future
>> confusions. Comments? It might make sense to back-patch as well.
>
> Compiles, didn't find any typos and I think it is comprehensible.

I took a look at this with a view to committing it but on examination
I'm not sure this is the best way to proceed. The proposed text
documents that the tests should be run in a database called
regression, but the larger documentation chapter of which this section
is a part never explains how to run them anywhere else, so it feels a
bit like telling a ten-year-old not to burn out the clutch.

The bit about not changing enable_* probably belongs, if anywhere, in
section 30.2, on test evaluation, rather than here.

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

On Fri, Jan 31, 2014 at 6:09 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jan 30, 2014 at 3:32 AM, Christian Kruse
> <christian(at)2ndquadrant(dot)com> wrote:
>>> For the documentation patch, I propose the attached to avoid future
>>> confusions. Comments? It might make sense to back-patch as well.
>>
>> Compiles, didn't find any typos and I think it is comprehensible.
>
> I took a look at this with a view to committing it but on examination
> I'm not sure this is the best way to proceed. The proposed text
> documents that the tests should be run in a database called
> regression, but the larger documentation chapter of which this section
> is a part never explains how to run them anywhere else, so it feels a
> bit like telling a ten-year-old not to burn out the clutch.
>
> The bit about not changing enable_* probably belongs, if anywhere, in
> section 30.2, on test evaluation, rather than here.
And what about the attached? I have moved all the content to 30.2, and
added two paragraphs: one about the planner flags, the other about the
database used.
Regards,
--
Michael

On Fri, Jan 31, 2014 at 2:28 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>> I took a look at this with a view to committing it but on examination
>> I'm not sure this is the best way to proceed. The proposed text
>> documents that the tests should be run in a database called
>> regression, but the larger documentation chapter of which this section
>> is a part never explains how to run them anywhere else, so it feels a
>> bit like telling a ten-year-old not to burn out the clutch.
>>
>> The bit about not changing enable_* probably belongs, if anywhere, in
>> section 30.2, on test evaluation, rather than here.
> And what about the attached? I have moved all the content to 30.2, and
> added two paragraphs: one about the planner flags, the other about the
> database used.
> Regards,

Well, it doesn't really address my first concern, which was that you
talk about running the tests in a database named regression, but
that's exactly what "make check" does and it's unclear how you would
do anything else without modifying the source code. It's not the
purpose of the documentation to tell you all the ways that you could
break things if you patch the tree. I also don't want to document
exactly which tests would fail if you did hack things like that; that
documentation is likely to become outdated.

I think the remaining points you raise are worth mentioning. I'm
attaching a patch with my proposed rewording of your changes. I made
the section about configuration parameters a bit more generic and
adjusted the phrasing to sound more natural in English, and I moved
your mention of the other issues around a bit. What do you think of
this version?

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

On Sat, Feb 1, 2014 at 12:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 31, 2014 at 2:28 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> I took a look at this with a view to committing it but on examination
>>> I'm not sure this is the best way to proceed. The proposed text
>>> documents that the tests should be run in a database called
>>> regression, but the larger documentation chapter of which this section
>>> is a part never explains how to run them anywhere else, so it feels a
>>> bit like telling a ten-year-old not to burn out the clutch.
>>>
>>> The bit about not changing enable_* probably belongs, if anywhere, in
>>> section 30.2, on test evaluation, rather than here.
>> And what about the attached? I have moved all the content to 30.2, and
>> added two paragraphs: one about the planner flags, the other about the
>> database used.
>> Regards,
>
> Well, it doesn't really address my first concern, which was that you
> talk about running the tests in a database named regression, but
> that's exactly what "make check" does and it's unclear how you would
> do anything else without modifying the source code. It's not the
> purpose of the documentation to tell you all the ways that you could
> break things if you patch the tree. I also don't want to document
> exactly which tests would fail if you did hack things like that; that
> documentation is likely to become outdated.
>
> I think the remaining points you raise are worth mentioning. I'm
> attaching a patch with my proposed rewording of your changes. I made
> the section about configuration parameters a bit more generic and
> adjusted the phrasing to sound more natural in English, and I moved
> your mention of the other issues around a bit. What do you think of
> this version?
The part about the planning parameter looks good, thanks. The places
used to mention the databases used also makes more sense. Thanks for
your input.
--
Michael

On Fri, Jan 31, 2014 at 5:48 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Feb 1, 2014 at 12:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jan 31, 2014 at 2:28 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> I took a look at this with a view to committing it but on examination
>>>> I'm not sure this is the best way to proceed. The proposed text
>>>> documents that the tests should be run in a database called
>>>> regression, but the larger documentation chapter of which this section
>>>> is a part never explains how to run them anywhere else, so it feels a
>>>> bit like telling a ten-year-old not to burn out the clutch.
>>>>
>>>> The bit about not changing enable_* probably belongs, if anywhere, in
>>>> section 30.2, on test evaluation, rather than here.
>>> And what about the attached? I have moved all the content to 30.2, and
>>> added two paragraphs: one about the planner flags, the other about the
>>> database used.
>>> Regards,
>>
>> Well, it doesn't really address my first concern, which was that you
>> talk about running the tests in a database named regression, but
>> that's exactly what "make check" does and it's unclear how you would
>> do anything else without modifying the source code. It's not the
>> purpose of the documentation to tell you all the ways that you could
>> break things if you patch the tree. I also don't want to document
>> exactly which tests would fail if you did hack things like that; that
>> documentation is likely to become outdated.
>>
>> I think the remaining points you raise are worth mentioning. I'm
>> attaching a patch with my proposed rewording of your changes. I made
>> the section about configuration parameters a bit more generic and
>> adjusted the phrasing to sound more natural in English, and I moved
>> your mention of the other issues around a bit. What do you think of
>> this version?
> The part about the planning parameter looks good, thanks. The places
> used to mention the databases used also makes more sense. Thanks for
> your input.

OK, committed and back-patched to 9.3.

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

On Tue, Feb 4, 2014 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 31, 2014 at 5:48 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> The part about the planning parameter looks good, thanks. The places
>> used to mention the databases used also makes more sense. Thanks for
>> your input.
>
> OK, committed and back-patched to 9.3.
Thanks.
--
Michael