On 11/12/24 08:39, Michael Paquier wrote:
> On Mon, Nov 11, 2024 at 07:32:10PM +0100, Tomas Vondra wrote:
>> This adds an out argument to brin_page_items, but I failed to consider
>> the user may still run with an older version of the extension - either
>> after pg_upgrade (as in the report), or when the CREATE EXTENSION
>> command specifies VERSION.
>
> Perhaps it would be worth adding a test where the function is called
> in the context of a past extension version? (I know, I'm noisy when
> it comes to that.)
>
>> The only fix I can think of is explicitly looking at TupleDesc->natts,
>> as in the attached fix.
>
> How about some consistency instead as this is the same error as
> 691e8b2e1889? btreefuncs.c is deciding to issue an error if we are
> trying to call one of its functions in the context of a past version
> of the extension. pageinspect is a superuser module, it does not seem
> that bad to me to tell users that they should force an upgrade when
> using it.
Yeah, a test with an older version is a good idea. And pageinspect
already has such tests in oldextversions.sql, so it was simple to just
add it there.
And I think you're right it's probably better to just error out. Yes, we
could check the natts value and build the right tuple, but if btree
functions already error out, it'd be a bit futile to do it differently
(although I'm not sure why bt_meta() says it can't be done reliably).
regards
--
Tomas Vondra
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 22621d584fa..525c82600f1 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_COLS_V1_12 8
/*
* Extract all item values from a BRIN index page
@@ -153,6 +155,20 @@ brin_page_items(PG_FUNCTION_ARGS)
errmsg("\"%s\" is not a %s index",
RelationGetRelationName(indexRel), "BRIN")));
+ /*
+ * We need a kluge here to detect API versions prior to 1.12. Earlier
+ * versions don't have the "empty" flag as o
+ *
+ * In principle we could skip the 'empty' flag and build the tuple with
+ * the old tuple descriptor, but functions like bt_meta() already error
+ * out in this case, so follow that precedent.
+ */
+ if (rsinfo->setDesc->natts < BRIN_PAGE_ITEMS_COLS_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.")));
+
bdesc = brin_build_desc(indexRel);
/* minimally verify the page we got */
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
index f5c4b61bd79..c0abd7ab3ca 100644
--- a/contrib/pageinspect/expected/oldextversions.out
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -52,5 +52,20 @@ SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
8192 | 4
(1 row)
+DROP TABLE test1;
+-- brin_page_items got a new OUT parameter in 1.12
+CREATE TABLE test1 (a int, b text);
+INSERT INTO test1 VALUES (1, 'one');
+CREATE INDEX test1_a_idx ON test1 USING brin (a);
+SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_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('test1_a_idx', 2), 'test1_a_idx');
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value
+------------+--------+--------+----------+----------+-------------+-------+----------
+ 1 | 0 | 1 | f | f | f | f | {1 .. 1}
+(1 row)
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
index 9f953492c23..b5342d3397d 100644
--- a/contrib/pageinspect/sql/oldextversions.sql
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -22,5 +22,18 @@ ALTER EXTENSION pageinspect UPDATE TO '1.9';
\df page_header
SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+DROP TABLE test1;
+
+-- brin_page_items got a new OUT parameter in 1.12
+CREATE TABLE test1 (a int, b text);
+INSERT INTO test1 VALUES (1, 'one');
+CREATE INDEX test1_a_idx ON test1 USING brin (a);
+
+SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx');
+
+ALTER EXTENSION pageinspect UPDATE TO '1.12';
+
+SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx');
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
