Horton

Thread

Commits

  1. Make pg_regress.c unset PGDATABASE during make installcheck.

  1. ECPG installcheck tests fail if PGDATABASE is set

    Andres Freund <andres@anarazel.de> — 2018-03-18T20:55:48Z

    Hi,

I got a bit confused running installcheck-world and seeing ecpg
failures like:
 [NO_PID]: ECPGconnect: opening database <DEFAULT> on <DEFAULT> port <DEFAULT>  for user regress_ecpg_user2
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ECPGconnect: could not open database: FATAL:  database "regress_ecpg_user2" does not exist
-
-[NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: ecpg_finish: connection main closed
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: raising sqlcode -402 on line 43: could not connect to database "<DEFAULT>" on line 43
-[NO_PID]: sqlca: code: -402, state: 08001
-[NO_PID]: raising sqlcode -220 on line 44: connection "main" does not exist on line 44
-[NO_PID]: sqlca: code: -220, state: 08003
 [NO_PID]: ECPGconnect: opening database ecpg2_regression on <DEFAULT> port <DEFAULT>  for user regress_ecpg_user1

A bit of pondering pointed me towards my environment's
PGDATABASE=postgres being to blame. Unsetting that makes the test
succeed.

Do we consider that an unsupported configuration?  It seems like a
fairly reasonable thing to want to support imo.

Greetings,

Andres Freund

  2. Re: ECPG installcheck tests fail if PGDATABASE is set

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-18T23:06:18Z

    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

  3. Re: ECPG installcheck tests fail if PGDATABASE is set

    Andres Freund <andres@anarazel.de> — 2018-03-18T23:09:58Z

    
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.

  4. Re: ECPG installcheck tests fail if PGDATABASE is set

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-18T23:30:33Z

    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

  5. Re: ECPG installcheck tests fail if PGDATABASE is set

    Andres Freund <andres@anarazel.de> — 2018-03-18T23:41:15Z

    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

  6. Re: ECPG installcheck tests fail if PGDATABASE is set

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-19T00:28:18Z

    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

  7. Re: ECPG installcheck tests fail if PGDATABASE is set

    Peter Geoghegan <pg@bowt.ie> — 2019-09-27T21:32:34Z

    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

  8. Re: ECPG installcheck tests fail if PGDATABASE is set

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-27T21:39:16Z

    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

  9. Re: ECPG installcheck tests fail if PGDATABASE is set

    Peter Geoghegan <pg@bowt.ie> — 2019-09-27T21:43:00Z

    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

  10. Re: ECPG installcheck tests fail if PGDATABASE is set

    Andres Freund <andres@anarazel.de> — 2019-09-27T21:52:17Z

    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

  11. Re: ECPG installcheck tests fail if PGDATABASE is set

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-27T22:03:08Z

    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

  12. Re: ECPG installcheck tests fail if PGDATABASE is set

    Peter Geoghegan <pg@bowt.ie> — 2019-09-28T03:18:25Z

    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