Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns

Hi Michael,

15.05.2023 03:41, Michael Paquier wrote:
> On Fri, May 12, 2023 at 01:00:01PM +0300, Alexander Lakhin wrote:
>> Please look at the patch attached, that makes gist_page_items() process items
>> on leaf and non-leaf pages differently, accounting for their contents.
>> I've decided to move away from BuildIndexValueDescription(), but borrow some
>> of it's code, for two reasons:
>> that function outputs only key columns;
>> it checks permissions for a table/index, but this is not needed for pageinspect
>> (firstly, because a user already has all the data do decode;
>> secondly, because gist_page_items() requires a superuser role anyway).
> I have not looked in details at the patch, and GiST is going to
> require some analysis, still I have a few comments after a lookup :)

Thanks for your comments!

> Anyway, is it right to assume that gist_page_items() is basically
> incorrect now with an included index, especially if one scans a range
> of pages, hitting leaves?

Yes, the existing implementation might crash with non-leaf pages and just
doesn't show non-key columns for items on leaves.

> The test output is a bit bloated. Could it be possible to make it
> shorter, like limiting the output to the first and last items, for
> example, as the goal is mainly to check the output format? This could
> check for the number of tuples, as well, but I am wondering if this
> would be stable across the buildfarm.
>
> + if (itup_isnull[i])
> + val = "null";
>
> The patch includes something for a NULL value, but does not seem to
> output anything related to it in the regression test.

Yeah, the test output could be shorter and more representative at the same
time. Please look at the improved test.

> +extern char *pg_get_indexdef_columns_with_included(Oid indexrelid,
> + bool pretty);
>
> Hmm. For back-branches, this approach would be fine, and even on HEAD
> I would not remove pg_get_indexdef_columns(). Still, for this new
> one, would it be better to have an _extended() version of this routine
> with some bits32 to control the addition of included columns and the
> pretty flag? For the API published, only these two would be
> necessary.

It sounds good to me, but for now I can see only a single caller of
pg_get_indexdef_columns(), so maybe it would be harmless to merely change the
existing function's parameter on HEAD? (To not have two distinct functions
with generic parameters for just two callers, which will use only two
fixed parameter values.)

Best regards,
Alexander