ECPG installcheck tests fail if PGDATABASE is set

Andres Freund <andres@anarazel.de> writes:

I got a bit confused running installcheck-world and seeing ecpg
failures like:
...
A bit of pondering pointed me towards my environment's
PGDATABASE=postgres being to blame. Unsetting that makes the test
succeed.

Hm ... pg_regress unsets PGDATABASE, along with the other related
environment variables, when it has a temp installation but not
when it doesn't. So what I don't understand is why your environment
doesn't also break every other regression test besides ecpg.
Perhaps they're all connecting to "postgres", but it's hard to
believe there wouldn't be conflicts if so.

regards, tom lane

On March 18, 2018 4:06:18 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

I got a bit confused running installcheck-world and seeing ecpg
failures like:
...
A bit of pondering pointed me towards my environment's
PGDATABASE=postgres being to blame. Unsetting that makes the test
succeed.

Hm ... pg_regress unsets PGDATABASE, along with the other related
environment variables, when it has a temp installation but not
when it doesn't. So what I don't understand is why your environment
doesn't also break every other regression test besides ecpg.
Perhaps they're all connecting to "postgres", but it's hard to
believe there wouldn't be conflicts if so.

All the others specify a database. The issue with the ecpg test is that it doesn't for two test cases. The failure is caused by libpq guessing the db name from the user name (hence the error about a database with role in it), which doesn't happen if libpq sees the environment variable.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Andres Freund <andres@anarazel.de> writes:

On March 18, 2018 4:06:18 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hm ... pg_regress unsets PGDATABASE, along with the other related
environment variables, when it has a temp installation but not
when it doesn't. So what I don't understand is why your environment
doesn't also break every other regression test besides ecpg.

All the others specify a database. The issue with the ecpg test is that
it doesn't for two test cases.

Ah. Well, it doesn't seem unreasonable to want to test that case,
so I don't think "remove the test case" is the right answer.

Is it sane for pg_regress to unset PGDATABASE unconditionally? Not
sure, but if we're generally always specifying a value, maybe that's
OK.

regards, tom lane

Hi,

On 2018-03-18 19:30:33 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On March 18, 2018 4:06:18 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hm ... pg_regress unsets PGDATABASE, along with the other related
environment variables, when it has a temp installation but not
when it doesn't. So what I don't understand is why your environment
doesn't also break every other regression test besides ecpg.

All the others specify a database. The issue with the ecpg test is that
it doesn't for two test cases.

Ah. Well, it doesn't seem unreasonable to want to test that case,
so I don't think "remove the test case" is the right answer.

Right.

Is it sane for pg_regress to unset PGDATABASE unconditionally? Not
sure, but if we're generally always specifying a value, maybe that's
OK.

I'm not sure either. I wonder whether we should just make ecpg's
pg_regress invocation do so? That seems to be the way of least
resistance ;)

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-03-18 19:30:33 -0400, Tom Lane wrote:

Is it sane for pg_regress to unset PGDATABASE unconditionally? Not
sure, but if we're generally always specifying a value, maybe that's
OK.

I'm not sure either. I wonder whether we should just make ecpg's
pg_regress invocation do so? That seems to be the way of least
resistance ;)

Don't think I like ecpg's tests behaving differently in this respect
than the rest of them do; that seems like a recipe for unrecognized
security issues.

If nobody can think of a positive reason for pg_regress not to
unset PGDATABASE unconditionally, let's try that and see how it
goes.

regards, tom lane

On Sun, Mar 18, 2018 at 5:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Don't think I like ecpg's tests behaving differently in this respect
than the rest of them do; that seems like a recipe for unrecognized
security issues.

If nobody can think of a positive reason for pg_regress not to
unset PGDATABASE unconditionally, let's try that and see how it
goes.

It would be nice to get this fixed. Several people have been confused
by it at this point.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

On Sun, Mar 18, 2018 at 5:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Don't think I like ecpg's tests behaving differently in this respect
than the rest of them do; that seems like a recipe for unrecognized
security issues.

If nobody can think of a positive reason for pg_regress not to
unset PGDATABASE unconditionally, let's try that and see how it
goes.

It would be nice to get this fixed. Several people have been confused
by it at this point.

I think I just forgot about this thread. Shall we change it in HEAD
and see what happens? Maybe backpatch, but not till after 12.0 is out.

regards, tom lane

On Fri, Sep 27, 2019 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think I just forgot about this thread. Shall we change it in HEAD
and see what happens? Maybe backpatch, but not till after 12.0 is out.

Please do.

--
Peter Geoghegan

On 2019-09-27 14:43:00 -0700, Peter Geoghegan wrote:

On Fri, Sep 27, 2019 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think I just forgot about this thread. Shall we change it in HEAD
and see what happens? Maybe backpatch, but not till after 12.0 is out.

Please do.

+1

Peter Geoghegan <pg@bowt.ie> writes:

On Fri, Sep 27, 2019 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think I just forgot about this thread. Shall we change it in HEAD
and see what happens? Maybe backpatch, but not till after 12.0 is out.

Please do.

After looking closer at the code in pg_regress.c, I'm wondering a bit
about PGSERVICE. A setting for that might certainly bring in a value
for the database name, but I don't think we can just summarily unset it.
I don't plan to do anything about that right now, but conceivably it'd
bite people someday.

Another thing that looks a bit fishy here is that the set of variables
that pg_regress.c unsets is very much smaller than the set that libpq
reacts to --- we have added a ton of the latter without touching this
list (much less the three or four other places that duplicate it).
I wonder how problematic that is.

regards, tom lane

On Fri, Sep 27, 2019 at 3:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Please do.

After looking closer at the code in pg_regress.c, I'm wondering a bit
about PGSERVICE. A setting for that might certainly bring in a value
for the database name, but I don't think we can just summarily unset it.
I don't plan to do anything about that right now, but conceivably it'd
bite people someday.

At least there will be some breadcrumbs to follow in the archive.

Another thing that looks a bit fishy here is that the set of variables
that pg_regress.c unsets is very much smaller than the set that libpq
reacts to --- we have added a ton of the latter without touching this
list (much less the three or four other places that duplicate it).
I wonder how problematic that is.

Only time will tell, I suspect.

--
Peter Geoghegan