Re: Inadequate executor locking of indexes - Mailing list pgsql-hackers

From David Rowley
Subject Re: Inadequate executor locking of indexes
Date
Msg-id CAKJS1f-zzk9zWnG6Xurn83C1a1Hm2g7Q-XiZqYtwu=3NKdJwCw@mail.gmail.com
Whole thread Raw
In response to Re: Inadequate executor locking of indexes  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Responses Re: Inadequate executor locking of indexes
List pgsql-hackers
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.

> 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



pgsql-hackers by date:

Previous
From: Stephen Frost
Date:
Subject: Re: [PATCH v20] GSSAPI encryption support
Next
From: Andres Freund
Date:
Subject: Re: allow online change primary_conninfo