contrib/dblink GRANTs and regression test fix

Joe Conway <mail@joeconway.com> writes:

The attached adds GRANTs to PUBLIC for dblink functions, removes the
non-standard regression test, and adds standard installcheck regression test
support.

The test creates a second database (regression_slave) and drops it again, in
order to avoid the cheesy-ness of connecting back to the same database ;-)

Applied. The method for dropping the second database seems pretty
cheesy itself, though; and wouldn't it make it harder to investigate
the cause of a test failure, if one occurs? I think I'd vote for
getting rid of the trailing wait() and DROP and instead doing

\set ECHO none
DROP DATABASE regression_slave;
\set ECHO all
CREATE DATABASE regression_slave;

at the top.

regards, tom lane

Tom Lane wrote:

Applied. The method for dropping the second database seems pretty
cheesy itself, though; and wouldn't it make it harder to investigate
the cause of a test failure, if one occurs? I think I'd vote for
getting rid of the trailing wait() and DROP and instead doing

\set ECHO none
DROP DATABASE regression_slave;
\set ECHO all
CREATE DATABASE regression_slave;

Yeah, I knew wait() was a bit of a kludge itself. But I couldn't get the drop
at the end to work (due to disconnect time I guess). I tried your suggestion
above, but "\set ECHO none" doesn't suppress the error message when
regression_slave does not already exist:

\set ECHO none
ERROR: DROP DATABASE: database "regression_slave" does not exist

So if expected/dblink.out includes this, it will fail on second and subsequent
runs of `make installcheck`. I guess it's easy enough to see why, but it
doesn't seem ideal either.

What do you think about a plpgsql function to check for the existence of
database "regression_slave", and drop it conditionally -- run at the beginning
of the script in place of DROP DATABASE?

Joe

p.s. BTW, I think this is a good example why an IF construct supported by the
main grammer would be nice. Something like:
IF (expression) {statements};
or something similar would allow conditional dropping of objects if they
exist, but no ERROR if they didn't. Possible 7.4 TODO?

Tom Lane wrote:

Applied. The method for dropping the second database seems pretty
cheesy itself, though; and wouldn't it make it harder to investigate
the cause of a test failure, if one occurs? I think I'd vote for
getting rid of the trailing wait() and DROP and instead doing

\set ECHO none
DROP DATABASE regression_slave;
\set ECHO all
CREATE DATABASE regression_slave;

I didn't give you any time for a reply, but just in case you think this
acceptable, here's a patch that does what I mentioned in the last post. Namely
it removes the wait() function and DROP DATABASE at the end of the script, and
adds a conditional_drop() function to the beginning of the script.

If there are no objections, please apply.

Thanks,

Joe

Joe Conway <mail@joeconway.com> writes:

What do you think about a plpgsql function to check for the existence
of database "regression_slave", and drop it conditionally -- run at
the beginning of the script in place of DROP DATABASE?

Okay by me.

p.s. BTW, I think this is a good example why an IF construct supported by the
main grammer would be nice. Something like:
IF (expression) {statements};
or something similar would allow conditional dropping of objects if they
exist, but no ERROR if they didn't. Possible 7.4 TODO?

Before we could move such a thing into the main grammar I'd want to
think hard about the concurrency implications. For the regression tests
we need not sweat about two backends dropping "regression_slave" at
about the same time ... but if it's going to be a mainstream supported
feature then I don't think we can ignore those issues.

regards, tom lane

Joe Conway <mail@joeconway.com> writes:

I didn't give you any time for a reply, but just in case you think this
acceptable, here's a patch that does what I mentioned in the last
post.

Applied. I made it "CREATE OR REPLACE" the function, strictly for
paranoia's sake (probably unnecessary, but...)

regards, tom lane