From 957ba9ff14066782a42ebb974913b2fc616c99e1 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 17 Dec 2024 16:47:23 +0100
Subject: [PATCH] Detect version mismatch in brin_page_items
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Commit dae761a87ed modified brin_page_items() to return the new "empty"
flag for each BRIN range. But the new output parameter was added in the
middle, which may cause crashes when using the new binary with old
function definition.

The ideal solution would be to introduce API versioning similar to what
pg_stat_statements does, but it's too late for that as PG17 was already
released (so we can't introduce a new extension version). We could do
something similar in brin_page_items() by checking the number of output
columns (and ignoring the new flag), but it doesn't seem very nice.

Instead, simply error out and suggest updating the extension to the
latest version. pageinspect is a superuser-only extension, and there's
not much reason to run an older version. Moreover, there's a precedent
for this approach in 691e8b2e18.

Reported by Ä½uboslav Å pilÃ¡k, investigation and patch by me. Backpatch to
17, same as dae761a87ed.

Reported-by: Ä½uboslav Å pilÃ¡k
Reviewed-by: Michael Paquier, Hayato Kuroda, Peter Geoghegan
Backpatch-through: 17
Discussion: https://postgr.es/m/VI1PR02MB63331C3D90E2104FD12399D38A5D2@VI1PR02MB6333.eurprd02.prod.outlook.com
Discussion: https://postgr.es/m/flat/3385a58f-5484-49d0-b790-9a198a0bf236@vondra.me
---
 contrib/pageinspect/brinfuncs.c                 | 17 +++++++++++++++++
 contrib/pageinspect/expected/oldextversions.out | 14 ++++++++++++++
 contrib/pageinspect/sql/oldextversions.sql      |  9 +++++++++
 3 files changed, 40 insertions(+)

diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 22621d584f..6f719352dd 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -117,6 +117,8 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
 	return page;
 }
 
+/* Number of output arguments (columns) for brin_page_items() */
+#define BRIN_PAGE_ITEMS_V1_12	8
 
 /*
  * Extract all item values from a BRIN index page
@@ -145,6 +147,21 @@ brin_page_items(PG_FUNCTION_ARGS)
 
 	InitMaterializedSRF(fcinfo, 0);
 
+	/*
+	 * Version 1.12 added a new output column for the empty range flag. But as
+	 * it was added in the middle, it may cause crashes with function
+	 * definitions from older versions of the extension.
+	 *
+	 * There is no way to reliably avoid the problems created by the old
+	 * function definition at this point, so insist that the user update the
+	 * extension.
+	 */
+	if (rsinfo->setDesc->natts < BRIN_PAGE_ITEMS_V1_12)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+				 errmsg("function has wrong number of declared columns"),
+				 errhint("To resolve the problem, update the \"pageinspect\" extension to the latest version.")));
+
 	indexRel = index_open(indexRelid, AccessShareLock);
 
 	if (!IS_BRIN(indexRel))
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
index f5c4b61bd7..2910891ece 100644
--- a/contrib/pageinspect/expected/oldextversions.out
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -52,5 +52,19 @@ SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
      8192 |       4
 (1 row)
 
+-- brin_page_items() added a new "empty" flag in 1.12, make sure we detect
+-- an old function definition
+ALTER EXTENSION pageinspect UPDATE TO '1.11';
+CREATE INDEX test_1_a_brin_idx ON test1 USING BRIN (a);
+SELECT * FROM brin_page_items(get_raw_page('test_1_a_brin_idx', 2), 'test_1_a_brin_idx');
+ERROR:  function has wrong number of declared columns
+HINT:  To resolve the problem, update the "pageinspect" extension to the latest version.
+ALTER EXTENSION pageinspect UPDATE TO '1.12';
+SELECT * FROM brin_page_items(get_raw_page('test_1_a_brin_idx', 2), 'test_1_a_brin_idx');
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty |                  value                   
+------------+--------+--------+----------+----------+-------------+-------+------------------------------------------
+          1 |      0 |      1 | f        | f        | f           | f     | {72057594037927937 .. 72057594037927937}
+(1 row)
+
 DROP TABLE test1;
 DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
index 9f953492c2..44f564faec 100644
--- a/contrib/pageinspect/sql/oldextversions.sql
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -22,5 +22,14 @@ ALTER EXTENSION pageinspect UPDATE TO '1.9';
 \df page_header
 SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
 
+-- brin_page_items() added a new "empty" flag in 1.12, make sure we detect
+-- an old function definition
+ALTER EXTENSION pageinspect UPDATE TO '1.11';
+CREATE INDEX test_1_a_brin_idx ON test1 USING BRIN (a);
+SELECT * FROM brin_page_items(get_raw_page('test_1_a_brin_idx', 2), 'test_1_a_brin_idx');
+
+ALTER EXTENSION pageinspect UPDATE TO '1.12';
+SELECT * FROM brin_page_items(get_raw_page('test_1_a_brin_idx', 2), 'test_1_a_brin_idx');
+
 DROP TABLE test1;
 DROP EXTENSION pageinspect;
-- 
2.39.5

