Re: ALTER TABLE lock strength reduction patch is unsafe

On 24 February 2014 16:01, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> I took a quick peek at this, and noticed the following things:
> * I am pretty sure this patch doesn't compile anymore after the latest
> set of releases.

Refreshed to v18, will post shortly.

> * This definitely should include isolationtester tests actually
> performing concurrent ALTER TABLEs. All that's currently there is
> tests that the locklevel isn't too high, but not that it actually works.

There is no concurrent behaviour here, hence no code that would be
exercised by concurrent tests.

Lock levels are proven in regression tests, so no further tests needed.

> * So far no DDL uses ShareRowExclusiveLocks, why do so now? Not sure if
> there aren't relevant cases for with foreign key checks (which afair
> *do* acquire SRE locks).

That was discussed long ago. We can relax it more in the future, if
that is considered safe.

> * Why is AddConstraint "so complicated" when it's all used SRE locks?

To ensure each case was considered, rather than just assume all cases
are the same.

> * Are you sure AlterConstraint is generally safe without an AEL? It
> should be safe to change whether something is deferred, but not
> necessarily whether it's deferrable?

We change the lock levels for individual commands. This patch provides
some initial settings and infrastructure.

It is possible you are correct that changing the deferrability is not
safe without an AEL. That was enabled for the first time in this
release in a patch added by me after this patch was written. I will
think on that and change, if required.

> * Why does ChangeOwner need AEL?

Ownership affects privileges, which includes SELECTs, hence AEL.

This is not a critical aspect of the patch.

> * You changed several places to take out lower locks, which in itself is
> fine, but doesn't that lead to lock upgrade hazard if a later step
> acquires a stronger lock? Or do we take out a strong enough lock from
> the beginning.

We asess the lock needed at parse time, then use it consistently
later. There is no lock upgrade hazard since that has been
specifically considered and removed.

> * There's no explanation why the EOXact TupleDesc stuff is needed?

I will update comments.

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