Re: ALTER TABLE lock strength reduction patch is unsafe Reply-To:

On 21 March 2014 03:45, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Sat, Mar 08, 2014 at 11:14:30AM +0000, Simon Riggs wrote:
>> On 7 March 2014 09:04, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > The right thing to do here is to not push to the extremes. If we mess
>> > too much with the ruleutil stuff it will just be buggy. A more
>> > considered analysis in a later release is required for a full and
>> > complete approach. As I indicated earlier, an 80/20 solution is better
>> > for this release.
>> >
>> > Slimming down the patch, I've removed changes to lock levels for
>> > almost all variants. The only lock levels now reduced are those for
>> > VALIDATE, plus setting of relation and attribute level options.
>
> Good call.

Thanks for the review. I'll respond to each point on a later email but
looks nothing much major, apart from the point raised on separate
thread.

>> + * Be careful to ensure this function is called for Tables and Indexes only.
>> + * It is not currently safe to be called for Views because security_barrier
>> + * is listed as an option and so would be allowed to be set at a level lower
>> + * than AccessExclusiveLock, which would not be correct.
>
> This statement is accepted and takes only ShareUpdateExclusiveLock:
>
> alter table information_schema.triggers set (security_barrier = true);

I find it hard to justify why we accept such a statement. Surely its a
bug when the named table turns out to be a view? Presumably ALTER
SEQUENCE and ALTER <other stuff> has checks for the correct object
type? OMG.

> I suggest adding a LOCKMODE field to relopt_gen and adding a
> reloptions_locklevel() function to determine the required level from an
> options list. That puts control of the lock level near the code that
> understands the implications for each option. You can then revert the
> addition of AlterViewInternal() and some of the utility.c changes.

Sure, that's how we code it, but I'm not sure we should introduce that
feature. The above weirdness is not itself justification.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services