Re: Add tests for LOCK TABLE

On Sun, Jul 7, 2013 at 12:17 PM, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:
> Updated the patch:
> - Updated ROLEs as per Robert's feedback

You managed to use a naming convention different from the one that you
used before. Before, you had regress_rol_op1; here, you've got
regress_lock_rol1. Consistency may be the hobgoblin of little minds,
but grep's mind is very little.

> - Added test to serial_schedule.

When you add the test to serial_schedule, you're supposed to add it to
the same place that it occupies in the parallel schedule, more or
less, not just add it to the bottom of the file. The idea is that the
two files should roughly correspond. We should probably automate
that, but for now, this is how it is.

I have committed this patch with substantial simplifications and a few
other tweaks that I thought would improve test coverage. I feel that
this patch wasn't prepared as carefully as it could have been. For
example, consider this expected output:

+-- Should work. Ensure that LOCK works for inherited tables;
+CREATE ROLE regress_lock_rol3;
+CREATE TABLE lock_tbl3 (a BIGINT);
+CREATE TABLE lock_tbl4 (b BIGINT) INHERITS (lock_tbl3);
+BEGIN TRANSACTION;
+LOCK TABLE lock_tbl4 * IN access EXCLUSIVE MODE;
+SET ROLE regress_lock_rol3;
+SET search_path = lock_schema1;
+LOCK TABLE lock_tbl3 NOWAIT;
+ERROR: relation "lock_tbl3" does not exist
+ROLLBACK;
+RESET ROLE;

The comment asserts that this "should work", but only part of it
works. Also, the failure is evidently intending to test whether
regress_lock_rol3 can lock lock_tbl3 despite lack of privileges on
that object, but the error message is complaining about something
different - namely, that regress_lock_rol3 doesn't even have
permissions on the schema. So the thing you meant to test isn't
really being tested.

The comment block at the top of the file was obviously cut and pasted
from somewhere else without being modified to reflect reality.

I altogether removed the last block - with lock_tbl{7,8,9,10} -
because the comment asserts that it "should fail" yet no statement in
that chunk actually fails, and locking on tables with inheritance is
test higher up. I did however modify the inheritance test to use a
multi-level inheritance hierarchy, since that does seem worth
verifying.

I also removed the test that simply checked whether a user without
permissions could lock the table; there's already a similar check in
privileges.sql.

All in all, I'm starting to get a bit skeptical of your test coverage
numbers. How are you deriving those, exactly? I agree that the tests
in that patch as committed are worthwhile, but they don't seem
sufficient to raise the coverage from 57% to 84% of... whatever those
numbers are percentages of. Now maybe in simplifying this down I
simplified away something essential, but I can't see what.

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