Re: Inadequate executor locking of indexes

On 2019/04/04 11:13, David Rowley wrote:
> On Thu, 4 Apr 2019 at 15:01, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Sorry, I didn't understand why it wouldn't be OK to pass NoLock to
>> index_open, for example, here:
>>
>> @@ -5191,7 +5191,14 @@ get_actual_variable_range(PlannerInfo *root,
>> VariableStatData *vardata,
>>                          * necessarily on the index.
>>                          */
>>                         heapRel = table_open(rte->relid, NoLock);
>> -                       indexRel = index_open(index->indexoid, AccessShareLock);
>> +
>> +                       /*
>> +                        * We use the same lock level as the relation as it may have
>> +                        * already been locked with that level.  Using the same lock level
>> +                        * can save a trip to the shared lock manager.
>> +                        */
>> +                       Assert(rte->rellockmode != NoLock);
>> +                       indexRel = index_open(index->indexoid, rte->rellockmode);
>>
>> Especially seeing that the table itself is opened without lock.  If there
>> are any Assert failures, wouldn't that need to be fixed in the upstream
>> code (such as get_relation_info)?
> 
> I put that Assert there to ensure that everything that calls it has
> properly set RangeTblEntry's rellockmode. If there's some valid reason
> for that to be NoLock then it's the wrong thing to do, but I can't
> think of a valid reason.

Which means the index must have been locked with that mode somewhere
upstream, so there's no need to lock it again.  Why not save the local
hash table lookup too if we can?

BTW, get_actual_variable_range is static to selfuncs.c and other public
functions that are entry point to get_actual_variable_range's
functionality appear to require having built a RelOptInfo first, which
means (even for third party code) having gone through get_relation_info
and opened the indexes.  That may be too much wishful thinking though.

>> Also, I noticed that there is infer_arbiter_indexes() too, which opens the
>> target table's indexes with RowExclusiveLock.  I thought for a second
>> that's a index-locking site in the planner that you may have missed, but
>> turns out it might very well be the first time those indexes are locked in
>> a given insert query's processing, because query_planner doesn't need to
>> plan access to the result relation, so get_relation_info is not called.
> 
> I skimmed over that and thought that the rellockmode would always be
> RowExclusiveLock anyway, so didn't change it. However, it would make
> sense to use rellockmode for consistency. We're already looking up the
> RangeTblEntry to get the relid, so getting the rellockmode is about
> free.

Yeah, maybe it'd be a good idea, if only for consistency, to fetch the
rellockmode of the resultRelation RTE and use it for index_open.

Thanks,
Amit