Re: Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

On Fri, Jul 31, 2015 at 8:30 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Fri, Jul 24, 2015 at 4:05 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
>> <fabriziomello@gmail.com> wrote:
>> > On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs <simon@2ndquadrant.com>
>> > wrote:
>> >> Looks functionally complete
>> >>
>> >> Need a test to show that ALTER TABLE works on views, as discussed on
>> >> this
>> >> thread. And confirmation that pg_dump is not broken by this.
>> >>
>> >> Message-ID:     20140321205828.GB3969106@tornado.leadboat.com
>> >>
>> >
>> > Added more test cases to cover ALTER TABLE on views.
>> >
>> > I'm thinking about the isolation tests, what about add another
>> > 'alter-table'
>> > spec for isolation tests enabling and disabling 'autovacuum' options?
>>
>> Yes, please.
>>
>
> Added. I really don't know if my isolation tests are completely correct, is
> my first time writing this kind of tests.

This patch size has increased from 16k to 157k because of the output
of the isolation tests you just added. This is definitely too large
and actually the test coverage is rather limited. Hence I think that
they should be changed as follows:
- Use only one table, the locks of tables a and b do not conflict, and
that's what we want to look at here.
- Check the locks of all the relation parameters, by that I mean as
well fillfactor and user_catalog_table which still take
AccessExclusiveLock on the relation
- Use custom permutations. I doubt that there is much sense to test
all the permutations in this context, and this will reduce the
expected output size drastically.

On further notice, I would recommend not to use the same string name
for the session and the query identifiers. And I think that inserting
only one tuple at initialization is just but fine.

Here is an example of such a spec:
setup
{CREATE TABLE a (id int PRIMARY KEY);INSERT INTO a SELECT generate_series(1,100);
}
teardown
{DROP TABLE a;
}
session "s1"
step "b1"       { BEGIN; }
# TODO add one query per parameter
step "at11"     { ALTER TABLE a SET (fillfactor=10); }
step "at12"     { ALTER TABLE a SET (autovacuum_vacuum_scale_factor=0.001); }
step "c1"       { COMMIT; }
session "s2"
step "b2"       { BEGIN; }
step "wx1"      { UPDATE a SET id = id + 10000; }
step "c2"       { COMMIT; }

And by testing permutations like for example that it is possible to
see which session is waiting for what. Input:
permutation "b1" "b2" "at11" "wx1" "c1" "c2"
Output where session 2 waits for lock taken after fillfactor update:
step b1: BEGIN;
step b2: BEGIN;
step at11: ALTER TABLE a SET (fillfactor=10);
step wx1: UPDATE a SET id = id + 10000; <waiting ...>
step c1: COMMIT;
step wx1: <... completed>
step c2: COMMIT;

Be careful as well to not include incorrect permutations in the
expected output file, the isolation tester is smart enough to ping you
about that.

>> +GetRelOptionsLockLevel(List *defList)
>> +{
>> +       LOCKMODE    lockmode = NoLock;
>> Shouldn't this default to AccessExclusiveLock instead of NoLock?
>>
>
> No, because it will break the logic bellow (get the highest locklevel
> according the defList), but I force return an AccessExclusiveLock if
> "defList == NIL".

Yep, OK with this change.

The rest of the patch looks good to me, so once the isolation test
coverage is done I think that it could be put in the hands of a
committer.
Regards
--
Michael