On Thu, Nov 19, 2020 at 03:15:21PM -0300, Alvaro Herrera wrote:
> On 2020-Nov-19, Justin Pryzby wrote:
>
> > On Fri, Nov 13, 2020 at 12:11:21PM -0600, Justin Pryzby wrote:
>
> > > Your patch didn't actually say "try_relation_open", so didn't work.
> > > But it does works if I do that, and close the table.
>
> Thanks for fixing and testing.
>
> > That patch broke the case that a non-index is passed, which I addressed
> > here.
>
> Hmm, I think the reaction to that should be the same as before, so
> rather than return 0, the patch should raise the same error that
> index_open() would.
The resulting logic is not very clear and requires a lot of commentary..
BTW I saw that in tablecmds.c, RangeVarCallbackForAttachIndex() does this:
if (classform->relkind != RELKIND_PARTITIONED_INDEX &&
classform->relkind != RELKIND_INDEX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("\"%s\" is not an index",
rv->relname)));
Is it wrong to use ERRCODE_INVALID_OBJECT_DEFINITION ?
Most other places say ERRCODE_WRONG_OBJECT_TYPE
Likewise, transformPartitionCmd() in parse_utilcmd.c:
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("\"%s\" is not a partitioned table",
RelationGetRelationName(parentRel))));
--
Justin
>From 3c6deeb57ba007b142f35312a436defb9d1b3783 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
Date: Fri, 13 Nov 2020 13:39:31 -0300
Subject: [PATCH] Avoid errors in brin summarization..
..which can happen if an index is reindexed concurrently
---
src/backend/access/brin/brin.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 1f72562c60..a1089d5f9e 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -886,9 +886,9 @@ brin_summarize_range(PG_FUNCTION_ARGS)
/*
* We must lock table before index to avoid deadlocks. However, if the
- * passed indexoid isn't an index then IndexGetRelation() will fail.
- * Rather than emitting a not-very-helpful error message, postpone
- * complaining, expecting that the is-it-an-index test below will fail.
+ * passed indexoid isn't an index, then IndexGetRelation(true) would
+ * emit a not-very-helpful error message. Instead, postpone complaining
+ * until the is-it-an-index test, below.
*/
heapoid = IndexGetRelation(indexoid, true);
if (OidIsValid(heapoid))
@@ -896,11 +896,33 @@ brin_summarize_range(PG_FUNCTION_ARGS)
else
heapRel = NULL;
- indexRel = index_open(indexoid, ShareUpdateExclusiveLock);
+ indexRel = try_relation_open(indexoid, ShareUpdateExclusiveLock);
+
+ /* Silently skip autovacuum work-items if an index has disappeared. */
+ if (!indexRel)
+ {
+ if (heapRel)
+ table_close(heapRel, ShareUpdateExclusiveLock);
+
+ PG_RETURN_INT32(0);
+ }
+
+ /* If passed a non-index, fail noisily */
+ if (indexRel->rd_rel->relkind != RELKIND_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not an index",
+ RelationGetRelationName(indexRel))));
+
+ /* If the table wasn't opened for some other reason, silently skip it */
+ if (!heapRel)
+ {
+ relation_close(indexRel, ShareUpdateExclusiveLock);
+ PG_RETURN_INT32(0);
+ }
/* Must be a BRIN index */
- if (indexRel->rd_rel->relkind != RELKIND_INDEX ||
- indexRel->rd_rel->relam != BRIN_AM_OID)
+ if (indexRel->rd_rel->relam != BRIN_AM_OID)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a BRIN index",
--
2.17.0
