Dear Tomas,
> Here's a fix for pageinspect bug in PG17, reported in [1]. The bug turns
> out to be introduced by my commit
I could not see the link, but I think it is [1], right?
>
> commit dae761a87edae444d11a411f711f1d679bed5941
> Author: Tomas Vondra <tomas.von...@postgresql.org>
> Date: Fri Dec 8 17:07:30 2023 +0100
>
> Add empty BRIN ranges during CREATE INDEX
>
> ...
>
> 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.
I confirmed that 428c0ca added new output entries (version is bumped 11->12),
and the same C function is used for both 11 and 12.
> The new "empty" field is added in the middle of the output tuple, which
> shifts the values, causing segfault when accessing a text field at the
> end of the array.
Agreed and I could reproduce by steps:
```
postgres=# CREATE EXTENSION pageinspect VERSION "1.11" ;
CREATE EXTENSION
postgres=# CREATE TABLE foo (id int);
CREATE TABLE
postgres=# CREATE INDEX ON foo USING brin ( id );
CREATE INDEX
postgres=# SELECT * FROM brin_page_items(get_raw_page('foo_id_idx', 2),
'foo_id_idx');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
```
>
> Of course, we add arguments to existing functions pretty often, and we
> know how to do that in backwards-compatible way - pg_stat_statements is
> a good example of how to do that nicely. Unfortunately, it's too late to
> do that for brin_page_items :-( There may already be upgraded systems or
> with installed pageinspect, etc.
>
> The only fix I can think of is explicitly looking at TupleDesc->natts,
> as in the attached fix.
I've tested your patch and it worked well. Also, I tried to upgrade from 11 and
12
and ran again, it could execute well.
Few points:
1.
Do you think we should follow the approach like pg_stat_statements for master?
Or, do you want to apply the patch both PG17 and HEAD? I think latter one is OK
because we should avoid code branches as much as possible.
2.
Later readers may surprise the part for checking `rsinfo->setDesc->natts`.
Can you use the boolean and add comments, something like
```
+ /* Support older API version */
+ bool output_empty_attr = (rsinfo->setDesc->natts >= 8);
```
3.
Do we have to add the test for the fix?
[1]:
https://www.postgresql.org/message-id/VI1PR02MB63331C3D90E2104FD12399D38A5D2%40VI1PR02MB6333.eurprd02.prod.outlook.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED
