On Thu, 4 Apr 2019 at 15:01, Amit Langote <langote_amit...@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. > 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. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
