Message-ID: <457826a7-95a6-96ae-dabc-6e07d8020a7b@gmail.com>
Date: Thu, 18 May 2023 09:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an
 index with non-key columns
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
References: <17884-cb8c326522977acb@postgresql.org>
 <92873f8d-a9b1-0ee8-5753-ee4a1664fb0e@gmail.com>
 <ZGF/qgICp6JvNDhV@paquier.xyz>
 <8a991814-97c0-81d6-1e24-60c26308f005@gmail.com>
 <ZGRk3LVBpt9/dFlt@paquier.xyz>
 <e4c9897a-6864-4c47-f7ea-08385f2a7db5@gmail.com>
 <ZGWEZCN0PpfFona0@paquier.xyz> <ZGWjHcBJoOPIbzYY@paquier.xyz>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <ZGWjHcBJoOPIbzYY@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/457826a7-95a6-96ae-dabc-6e07d8020a7b%40gmail.com>
Precedence: bulk

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