From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Julien Rouhaud <rjuju123(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com> |
Subject: | Re: Inadequate executor locking of indexes |
Date: | 2019-04-04 02:01:41 |
Message-ID: | c01cd66e-44f5-71c4-1e28-9cf25605b8f5@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi David,
Thanks for updating the patch.
On 2019/04/04 9:14, David Rowley wrote:
> I also looked over other index_open() calls in the planner and found a
> bunch of places in selfuncs.c that we open an index to grab some
> information then close it again releasing the lock. At this stage
> get_relation_info() should have already grabbed what it needs and
> stored it into an IndexOptInfo, so we might have no need to access the
> index again. However, if any code was added that happened to assume
> the index was already locked then we'd get the same Assert failure
> that we're fixing here. I've ended up changing these calls so that
> they also use rellockmode, which may make the lock just a trip to the
> local lock table for relations that have rellockmode >
> AccessShareLock. I also changed the index_close to use NoLock so we
> hold the lock.
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)?
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 scanned around other usages of index_open() and saw that
> gin_clean_pending_list() uses an AccessShareLock. That seems strange
> since it modifies the index.
Yeah, other maintenance tasks modifying an index, such as
brin_summarize_range(), take ShareUpdateExclusiveLock.
Thanks,
Amit
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2019-04-04 02:01:48 | Re: [PATCH v20] GSSAPI encryption support |
Previous Message | Michael Paquier | 2019-04-04 01:26:50 | Re: Simplify redability of some tests for toast_tuple_target in strings.sql |