Subject: Re: Assert in pageinspect with NULL pages

From: Justin Pryzby <[email-masked]>
Date: Thu, 17 Feb 2022 21:00:20 -0600

BRIN can also crash if passed a non-brin index.

I've been sitting on this one for awhile.  Feel free to include it in your
patchset.

commit 08010a6037fc4e24a9ba05e5386e766f4310d35e
Author: Justin Pryzby <[email-masked]>
Date:   Tue Jan 19 00:25:15 2021 -0600

    pageinspect: brin_page_items(): check that given relation is not only an index, but a brin one

diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index f1e64a39ef2..3de6dd943ef 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -16,6 +16,7 @@
 #include "access/brin_tuple.h"
 #include "access/htup_details.h"
 #include "catalog/index.h"
+#include "catalog/pg_am.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "lib/stringinfo.h"
@@ -59,7 +60,7 @@ brin_page_type(PG_FUNCTION_ARGS)
 	if (raw_page_size != BLCKSZ)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("input page too small"),
+				 errmsg("input page wrong size"),
 				 errdetail("Expected size %d, got %d",
 						   BLCKSZ, raw_page_size)));
 
@@ -97,7 +98,7 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
 	if (raw_page_size != BLCKSZ)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("input page too small"),
+				 errmsg("input page wrong size"),
 				 errdetail("Expected size %d, got %d",
 						   BLCKSZ, raw_page_size)));
 
@@ -169,7 +170,14 @@ brin_page_items(PG_FUNCTION_ARGS)
 	MemoryContextSwitchTo(oldcontext);
 
 	indexRel = index_open(indexRelid, AccessShareLock);
-	bdesc = brin_build_desc(indexRel);
+
+	/* 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))));
 
 	/* minimally verify the page we got */
 	page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, "regular");
@@ -178,6 +186,7 @@ brin_page_items(PG_FUNCTION_ARGS)
 	 * Initialize output functions for all indexed datatypes; simplifies
 	 * calling them later.
 	 */
+	bdesc = brin_build_desc(indexRel);
 	columns = palloc(sizeof(brin_column_state *) * RelationGetDescr(indexRel)->natts);
 	for (attno = 1; attno <= bdesc->bd_tupdesc->natts; attno++)
 	{

---

From: Michael Paquier <[email-masked]>
Date: Fri, 18 Feb 2022 12:07:21 +0900

> I've been sitting on this one for awhile.  Feel free to include it in your

Ugh.  Thanks!  I am keeping a note about this one.
--
Michael

---

From: Alexander Lakhin <[email-masked]>
Date: Mon, 21 Feb 2022 10:00:00 +0300

Hello Michael,

> Ugh.  Thanks!  I am keeping a note about this one.
Could you please confirm before committing the patchset that it fixes
the bug #16527 [1]? Or maybe I could check it?
(Original patch proposed by Daria doesn't cover that case, but if the
patch going to be improved, probably it's worth fixing that bug too.)

[1]
https://www.postgresql.org/message-id/flat/16527-ef7606186f0610a1%40postgresql.org

Best regards,
Alexander

---

From: Michael Paquier <[email-masked]>
Date: Mon, 21 Feb 2022 17:04:51 +0900

> patch going to be improved, probably it's worth fixing that bug too.)

I have not directly checked, but that looks like the same issue to
me.  By the way, I was waiting for an updated patch set, based on the
review provided upthread:
https://www.postgresql.org/message-id/Yg8MPrV49/[email-masked]

And it seems better to group everything in a single commit as the same
areas are touched.
--
Michael