On Sun, Jul 7, 2013 at 10:35 AM, Robins Tharakan <tharakan@gmail.com> wrote:
> On 26 June 2013 01:55, Robins Tharakan <tharakan@gmail.com> wrote:
>>
>> Code coverage improved from 36% to 68%.
>
> Updated patch:
> - Renamed ROLEs as per Robert's feedback (prepend regress_xxx)
> - Added test to serial_schedule (missed out earlier).
Databases - like roles - are global objects, so those should be
similarly prefixed with "regress". This is very dangerous:
+DROP DATABASE db_db2; -- doesn't exist
+DROP DATABASE IF EXISTS db_db2; -- doesn't exist with IF EXISTS;
+DROP DATABASE template1; -- can't drop a template database
+DROP DATABASE regression; -- can't drop a database in use
I think we should drop the first three of these. The first one risks
dropping actual user data in the "make installcheck" case, as does the
second one. We can reduce the risk of death and destruction by
choosing a better database name, but I don't think it's really worth
it. If DROP DATABASE gets broken, we'll notice that soon enough.
I don't think the third one is a good test, either. There's no
requirement that the user keep template1 around, although nearly
everyone probably does.
Please see if you can't also do something to minimize the number of
create/drop role cycles this patch performs - like maybe to just one.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company