Re: Inadequate executor locking of indexes

On Thu, 14 Mar 2019 at 21:52, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> If the correct lock is taken in both cases by the current code, then maybe
> there's no need to change anything?  What does idxlockmode improve about
> the existing situation?  One thing I can imagine it improves is that we
> don't need the potentially expensive ExecRelationIsTargetRelation() check
> anymore, because idxlockmode gives that information for free.

I'm aiming to fix the problem described by Tom when he started this
thread. It's pretty easy to get an assert failure in master.

create table t1(a int);
create index on t1(a);
prepare q1 as delete from t1 where a = 1;
execute q1;
execute q1;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

this fails because modifytable does not open the indexes for DELETE
and the index scan node thinks it can get away with NoLock on the
index since it thinks modifytable already locked it. We hit the
CheckRelationLockedByMe() Assert in relation_open().

The idea with the patch is to a) fix this; and b) try and reduce the
chances of bugs in this area in the future by having something more
central decide the lock level we need rather than having various bits
of code that currently disagree with each other about what needs to be
done; and c) attempt to fix the lock upgrade hazard as best as we can.

Maybe you're right about being able to use rellockmode for indexes,
but I assume that we lowered the lock level for indexes for some
reason, and this would reverse that.

> If we're to fix that upgrade hazard, maybe we'll need a separate phase to
> determine the strongest lock to take on each table referenced in different
> parts of the query (across CTEs) that runs before
> pg_analyze_and_rewrite()?  We surely can't use table OIDs as keys though.

I don't think that's workable as we've yet to perform
expand_inherited_tables() and some other subquery might reference some
partition directly.

I think this borderline impossible to fix completely.  To do it just
for the planner we'll need to plan each subquery as far as determining
all relations that will be used. Probably at least as far as
expand_inherited_tables(), but not as far as calling
get_relation_info() on any relation. We'd then do the same for all the
other subqueries then do finalize_lockmodes(), only then could we call
get_relation_info(), otherwise, we might call it with too weak a lock
and later we might need to obtain a stronger lock.  But even then it's
possible we get lock upgrades in the parser too, for example, select *
from t1 a, t1 b for update of b;

Maybe if we can't fix that hazard completely, then it's not worth
adding any code for at all, but we still need something for a) and
fixing b) seems like a good idea too. If we don't care enough for c)
then we can use the patch without finalize_lockmodes().

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