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

18.05.2023 07:01, Michael Paquier wrote:
> On Thu, May 18, 2023 at 10:50:28AM +0900, Michael Paquier wrote:
>> I have expanded the tests to show how this applies for more data
>> types, like point or integers.  Any thoughts about the v5 attached?

I like this version and think it's ready to launch.

Thank you!

I would suggest just two changes:
more parenthesis
->
more parentheses
(isn't this the plural form?)

Non-leaf pages have only the key attributes, and leaf pages have
the included attributes.
->
Non-leaf pages contain only the key attributes, and leaf pages contain
the included attributes.
(To avoid misreading as if there are some attributes of the pages.)

> Two extra things that I had in mind, as long as I don't forget about
> them..  We could have a logic closer to record_out(), and grab a copy
> of it for this specific function (hstore does that, as one example),
> but I cannot really get into this approach, it just does not seem
> worth the complications compared to the use cases.

Yeah, hstore_from_record() doesn't look simpler. For the moment, I see no
reasons to use that approach.

> Another thing would be to use separate bracket types to the groups of
> values while still using parenthesis for the whole set of key and
> included values, like:
> (a1, a2) INCLUDE (a3, a4) = ([val1], [val2]) INCLUDE ([val3], [val4])
>
> But I am not sure that we need this much complication, either.
> Compared to the point of showing all the values from the GiST tuples,
> tweaking the style is not interesting as pageinspect is for advanced
> users.  More opinions or ideas are welcome, of course.

Yes, I had thought about adding "{}", too, but decided that this would
arguably improve reading, but inarguably raise formalization questions.
So I would leave "()" as done in v5.

Best regards,
Alexander