Fix brin_summarize_new_values() to check index type and ownership.

brin_summarize_new_values() did not check that the passed OID was for
an index at all, much less that it was a BRIN index, and would fail in
obscure ways if it wasn't (possibly damaging data first?).  It also
lacked any permissions test; by analogy to VACUUM, we should only allow
the table's owner to summarize.

Noted by Jeff Janes, fix by Michael Paquier and me

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 99337b0f0c047ab33277f4e0fca342095a5e2a39..2622a7efb13b22d4bad186997574215096de0aee 100644 (file)
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -787,14 +787,50 @@ Datum
 brin_summarize_new_values(PG_FUNCTION_ARGS)
 {
    Oid         indexoid = PG_GETARG_OID(0);
+   Oid         heapoid;
    Relation    indexRel;
    Relation    heapRel;
    double      numSummarized = 0;
 
-   heapRel = heap_open(IndexGetRelation(indexoid, false),
-                       ShareUpdateExclusiveLock);
+   /*
+    * 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.
+    */
+   heapoid = IndexGetRelation(indexoid, true);
+   if (OidIsValid(heapoid))
+       heapRel = heap_open(heapoid, ShareUpdateExclusiveLock);
+   else
+       heapRel = NULL;
+
    indexRel = index_open(indexoid, ShareUpdateExclusiveLock);
 
+   /* Must be a BRIN index */
+   if (indexRel->rd_rel->relkind != RELKIND_INDEX ||
+       indexRel->rd_rel->relam != BRIN_AM_OID)
+       ereport(ERROR,
+               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                errmsg("\"%s\" is not a BRIN index",
+                       RelationGetRelationName(indexRel))));
+
+   /* User must own the index (comparable to privileges needed for VACUUM) */
+   if (!pg_class_ownercheck(indexoid, GetUserId()))
+       aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
+                      RelationGetRelationName(indexRel));
+
+   /*
+    * Since we did the IndexGetRelation call above without any lock, it's
+    * barely possible that a race against an index drop/recreation could have
+    * netted us the wrong table.  Recheck.
+    */
+   if (heapRel == NULL || heapoid != IndexGetRelation(indexoid, false))
+       ereport(ERROR,
+               (errcode(ERRCODE_UNDEFINED_TABLE),
+                errmsg("could not open parent table of index %s",
+                       RelationGetRelationName(indexRel))));
+
+   /* OK, do it */
    brinsummarize(indexRel, heapRel, &numSummarized, NULL);
 
    relation_close(indexRel, ShareUpdateExclusiveLock);
@@ -962,7 +998,7 @@ summarize_range(IndexInfo *indexInfo, BrinBuildState *state, Relation heapRel,
     */
    state->bs_currRangeStart = heapBlk;
    scanNumBlks = heapBlk + state->bs_pagesPerRange <= heapNumBlks ?
-                       state->bs_pagesPerRange : heapNumBlks - heapBlk;
+       state->bs_pagesPerRange : heapNumBlks - heapBlk;
    IndexBuildHeapRangeScan(heapRel, state->bs_irel, indexInfo, false, true,
                            heapBlk, scanNumBlks,
                            brinbuildCallback, (void *) state);

diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index 0937b63667c3dfd6cb5a641162559f1db1b04450..475525912fe7ba4b8d0093fe4a3c57ecc4932523 100644 (file)
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -407,3 +407,14 @@ FROM tenk1 ORDER BY unique2 LIMIT 5 OFFSET 5;
 VACUUM brintest;  -- force a summarization cycle in brinidx
 UPDATE brintest SET int8col = int8col * int4col;
 UPDATE brintest SET textcol = '' WHERE textcol IS NOT NULL;
+-- Tests for brin_summarize_new_values
+SELECT brin_summarize_new_values('brintest'); -- error, not an index
+ERROR:  "brintest" is not an index
+SELECT brin_summarize_new_values('tenk1_unique1'); -- error, not a BRIN index
+ERROR:  "tenk1_unique1" is not a BRIN index
+SELECT brin_summarize_new_values('brinidx'); -- ok, no change expected
+ brin_summarize_new_values 
+---------------------------
+                         0
+(1 row)
+

diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index db78d05ff3f55dc5544f8fc05619af2920926563..9e4836e17ebec32f7a8be29f692091a0751856e1 100644 (file)
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -416,3 +416,8 @@ VACUUM brintest;  -- force a summarization cycle in brinidx
 
 UPDATE brintest SET int8col = int8col * int4col;
 UPDATE brintest SET textcol = '' WHERE textcol IS NOT NULL;
+
+-- Tests for brin_summarize_new_values
+SELECT brin_summarize_new_values('brintest'); -- error, not an index
+SELECT brin_summarize_new_values('tenk1_unique1'); -- error, not a BRIN index
+SELECT brin_summarize_new_values('brinidx'); -- ok, no change expected