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