Обсуждение: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns

The following bug has been logged on the website:

Bug reference:      17884
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE TABLE tbl(p point, t1 text, t2 text);
CREATE INDEX tbl_gist_idx ON tbl USING gist (p) INCLUDE (t1, t2);
INSERT INTO tbl SELECT point(x, x), repeat('x', 500), repeat('x', 500) FROM
generate_series(1, 100) AS x;

CREATE EXTENSION pageinspect;
SELECT * FROM gist_page_items(get_raw_page('tbl_gist_idx', 0),
'tbl_gist_idx');

causes a server crash with the following stack trace:
Core was generated by `postgres: law regression [local] SELECT
                        '.
Program terminated with signal SIGSEGV, Segmentation fault.

warning: Section `.reg-xstate/681022' in core file too small.
#0  0x000055c05775638c in index_deform_tuple_internal
(tupleDescriptor=0x7f3b2cb45558, values=0x7ffe813b5cf0, 
    isnull=0x7ffe813b5ca0, tp=0x55c058cb5630 "", bp=0x55c058cb5630 "",
hasnulls=0) at indextuple.c:520
520                                     off = att_align_pointer(off,
thisatt->attalign, -1,
(gdb) bt
#0  0x000055c05775638c in index_deform_tuple_internal
(tupleDescriptor=0x7f3b2cb45558, values=0x7ffe813b5cf0, 
    isnull=0x7ffe813b5ca0, tp=0x55c058cb5630 "", bp=0x55c058cb5630 "",
hasnulls=0) at indextuple.c:520
#1  0x000055c057756186 in index_deform_tuple (tup=0x55c058cb5628,
tupleDescriptor=0x7f3b2cb45558, 
    values=0x7ffe813b5cf0, isnull=0x7ffe813b5ca0) at indextuple.c:467
#2  0x00007f3b38c79abc in gist_page_items (fcinfo=0x55c058c77420) at
gistfuncs.c:258
#3  0x000055c057a5e447 in ExecMakeTableFunctionResult
(setexpr=0x55c058c81a28, econtext=0x55c058c818e0, 
    argContext=0x55c058c77300, expectedDesc=0x55c058c82ce8,
randomAccess=false) at execSRF.c:234
#4  0x000055c057a7be29 in FunctionNext (node=0x55c058c816c8) at
nodeFunctionscan.c:95
#5  0x000055c057a5ff3d in ExecScanFetch (node=0x55c058c816c8,
accessMtd=0x55c057a7bd74 <FunctionNext>, 
    recheckMtd=0x55c057a7c170 <FunctionRecheck>) at execScan.c:133
#6  0x000055c057a5ffb6 in ExecScan (node=0x55c058c816c8,
accessMtd=0x55c057a7bd74 <FunctionNext>, 
    recheckMtd=0x55c057a7c170 <FunctionRecheck>) at execScan.c:182
#7  0x000055c057a7c1c9 in ExecFunctionScan (pstate=0x55c058c816c8) at
nodeFunctionscan.c:270
#8  0x000055c057a5b9a4 in ExecProcNodeFirst (node=0x55c058c816c8) at
execProcnode.c:464
#9  0x000055c057a4ebd1 in ExecProcNode (node=0x55c058c816c8) at
../../../src/include/executor/executor.h:259
#10 0x000055c057a51ae4 in ExecutePlan (estate=0x55c058c81490,
planstate=0x55c058c816c8, use_parallel_mode=false, 
    operation=CMD_SELECT, sendTuples=true, numberTuples=0,
direction=ForwardScanDirection, dest=0x55c058cacc28, 
    execute_once=true) at execMain.c:1636
#11 0x000055c057a4f308 in standard_ExecutorRun (queryDesc=0x55c058bbe9b0,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:363
#12 0x000055c057a4f0f3 in ExecutorRun (queryDesc=0x55c058bbe9b0,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:307
#13 0x000055c057cc959e in PortalRunSelect (portal=0x55c058c08e80,
forward=true, count=0, dest=0x55c058cacc28)
    at pquery.c:924
#14 0x000055c057cc91bc in PortalRun (portal=0x55c058c08e80,
count=9223372036854775807, isTopLevel=true, run_once=true, 
    dest=0x55c058cacc28, altdest=0x55c058cacc28, qc=0x7ffe813b6430) at
pquery.c:768
#15 0x000055c057cc2050 in exec_simple_query (
    query_string=0x55c058b9c680 "SELECT * FROM
gist_page_items(get_raw_page('tbl_gist_idx', 0), 'tbl_gist_idx');")
    at postgres.c:1250
#16 0x000055c057cc6f74 in PostgresMain (dbname=0x55c058bc8d08 "regression",
username=0x55c058bc8ce8 "law")
    at postgres.c:4593
#17 0x000055c057bebe7c in BackendRun (port=0x55c058bc0420) at
postmaster.c:4511
#18 0x000055c057beb703 in BackendStartup (port=0x55c058bc0420) at
postmaster.c:4239
#19 0x000055c057be7678 in ServerLoop () at postmaster.c:1806
#20 0x000055c057be6dd8 in PostmasterMain (argc=3, argv=0x55c058b96630) at
postmaster.c:1478
#21 0x000055c057ada5f4 in main (argc=3, argv=0x55c058b96630) at main.c:202

Here gist_page_items() tries to decode a tuple using a descriptor of the
index (defining 3 columns), but in fact non-leaf page items contain no
non-key data as
SELECT * FROM gist_page_items_bytea(get_raw_page('tbl_gist_idx', 0));
shows:
 itemoffset |    ctid    | itemlen | dead |
    key_data                                      

------------+------------+---------+------+------------------------------------------------------------------------------------
          1 | (1,65535)  |      40 | f    | \x00000100ffff28...00f03f
          2 | (2,65535)  |      40 | f    | \x00000200ffff28...001440
...
         25 | (25,65535) |      40 | f    | \x00001900ffff28...405840
(25 rows)

Compare with a leaf page (notably different itemlen):
SELECT * FROM gist_page_items_bytea(get_raw_page('tbl_gist_idx', 1));
          1 | (0,1) |    1048 | f    | \x0000000001001844...787878
          2 | (0,2) |    1048 | f    | \x0000000002001844....787878
...

Other index types are not affected because:
btree outputs all data bytes, not keys only;
brin, gin, hash don't support included columns.

Observed on REL_14_STABLE (starting from 756ab2912) .. master.

04.04.2023 16:00, PG Bug reporting form sent:
> The following bug has been logged on the website:
>
> Bug reference:      17884
>
> ...
>
> Here gist_page_items() tries to decode a tuple using a descriptor of the
> index (defining 3 columns), but in fact non-leaf page items contain no
> non-key data ...

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).

Best regards,
Alexander

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 :)

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?

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.

+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.

+ (1,"(0,1)",48,f,"(p) INCLUDE (t)=((1,1)) INCLUDE (1)")

Agreed that this format kind of makes sense, showing both the
attribute included and the value include.
--
Michael

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

On Mon, May 15, 2023 at 11:00:00AM +0300, Alexander Lakhin wrote:
> Yes, the existing implementation might crash with non-leaf pages and just
> doesn't show non-key columns for items on leaves.

Ah, OK.  After more caffeine..  So that's two problems, not one.

>> 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.
>
> Yeah, the test output could be shorter and more representative at the same
> time. Please look at the improved test.

Sounds fine as it is, thanks for the update.  The patch is now much
shorter in size.

> 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.)

Sure, we could do that for HEAD, but we would still have to maintain
an extra routine for the back-branches.  So I would tend to choose an
approach that's consistent across all the branches while being as much
as possible extensible.

Something similar could be said about the lack of extensibility of
BuildIndexValueDescription().  However, note also its top comment
telling about the fact that the function is used only for the
generation of error messages, which is something that pageinspect has
actually violated since we have the item functions for GiST :)

As you said upthread, there may be an argument in making an extra
version of BuildIndexValueDescription() to be aware of included
attributes but I cannot get much excited about adding an extra code
path in genam.c without something else than just begin able to look at
the GiST items on a page.  And BuildIndexValueDescription() has a few
ACL checks while pageinspect requires superuser rights, so that makes
the module a bit faster, so agreed with your points.

+       tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+       tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);

Ah, I see.  So you need that to be able to deform correctly the tuple
coming from a non-leaf page that only has key attributes.  Tricky at
first sight, indeed..

I have done a few adjustments to that patch, refining some comments,
fixing some ordering with the headers and applying an indentation.  Is
that OK for you?
--
Michael

17.05.2023 08:23, Michael Paquier wrote:
> On Mon, May 15, 2023 at 11:00:00AM +0300, Alexander Lakhin wrote:
>> Yes, the existing implementation might crash with non-leaf pages and just
>> doesn't show non-key columns for items on leaves.
> Ah, OK.  After more caffeine..  So that's two problems, not one.

Perhaps I had the lack of caffeine too, because it seems that I missed third
one. I'm sorry about that.

>> Yeah, the test output could be shorter and more representative at the same
>> time. Please look at the improved test.
> Sounds fine as it is, thanks for the update.  The patch is now much
> shorter in size.

Yes, but I'm afraid it is not a final version yet.

>> 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.)
> Sure, we could do that for HEAD, but we would still have to maintain
> an extra routine for the back-branches.  So I would tend to choose an
> approach that's consistent across all the branches while being as much
> as possible extensible.

I am fine with this approach, Thank you!

> +       tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
> +       tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
>
> Ah, I see.  So you need that to be able to deform correctly the tuple
> coming from a non-leaf page that only has key attributes.  Tricky at
> first sight, indeed..

Yes, the same thing was done in f2e403803, so I think we can safely repeat it here.

> I have done a few adjustments to that patch, refining some comments,
> fixing some ordering with the headers and applying an indentation.  Is
> that OK for you?

While thinking about naming (what exactly "attroid" means there), I've
reread the comment above BuildIndexValueDescription():
  * The passed-in values/nulls arrays are the "raw" input to the index AM,
  * e.g. results of FormIndexDatum --- this is not necessarily what is stored
  * in the index, but it's what the user perceives to be stored.

and recognized that I've made a mistake when relied on the existing coding.
A simple example:
create extension pageinspect;
create extension pg_trgm;

create table test_trgm(t text);
create index trgm_idx on test_trgm using gist (t gist_trgm_ops);
insert into test_trgm select to_char(g, 'textFM000')  from generate_series(1, 1000) g;
select gist_page_items(get_raw_page('trgm_idx', 0), 'trgm_idx'::regclass);
select gist_page_items(get_raw_page('trgm_idx', 1), 'trgm_idx'::regclass);

(I chose gist_trgm_ops because that opclass defined as follows:
CREATE OPERATOR CLASS gist_trgm_ops FOR TYPE text USING gist ... STORAGE gtrgm)
shows garbage values:
                gist_page_items
---------------------------------------------
  (1,"(1,65535)",32,f,"(t)=(\x02\x7F\x7F_ߟ)")
  (2,"(3,65535)",32,f,"(t)=(\x02')")
  (3,"(2,65535)",32,f,"(t)=(\x02߽)")
  (4,"(5,65535)",32,f,"(t)=(\x02\x7F)")
  (5,"(4,65535)",32,f,"(t)=(\x02w?\x7F)")
  (6,"(6,65535)",32,f,"(t)=(\x02/)")
(6 rows)

                       gist_page_items
-----------------------------------------------------------
  (1,"(0,198)",40,f,"(t)=(\x01  t te19898 extt19texxt1)")
  (2,"(0,200)",40,f,"(t)=(\x01  t te00 200extt20texxt2)")
  (3,"(0,202)",40,f,"(t)=(\x01  t te02 202extt20texxt2)")
  (4,"(0,203)",40,f,"(t)=(\x01  t te03 203extt20texxt2)")
  (5,"(0,56)",40,f,"(t)=(\x01  t te05656 extt05texxt0)")
...

It means that pageinspect tries to print stored gtrgm values as text, and I
believe that's the third problem here.
Moreover, for the point_ops class opcintype is point, but opckeytype is box,
thus indexes test_gist_idx and test_gist_idx_inc used in the test in fact
contain boxes, not points. So the fixed gist_page_items() produces different
output even for existing test cases. (Please look at the patch attached.)
I'm somewhat confused with the output:
(p)=((185,185),(1,1))
(Maybe add more parentheses around val to get "(p)=(((185,185),(1,1)))", not
sure about that.)
But I think the current gist_page_items() output is not correct and should be
fixed anyway.

Best regards,
Alexander

On Wed, May 17, 2023 at 05:00:00PM +0300, Alexander Lakhin wrote:
> While thinking about naming (what exactly "attroid" means there), I've
> reread the comment above BuildIndexValueDescription():
>  * The passed-in values/nulls arrays are the "raw" input to the index AM,
>  * e.g. results of FormIndexDatum --- this is not necessarily what is stored
>  * in the index, but it's what the user perceives to be stored.

Yeah, I was wondering about that a bit as well yesterday, see below.
But in short I'd agree that showing all the data from a single item as
stored is more helpful than just showing what would be perceived by
the user, through originally what's an API to generate error messages
on constraint failures.

> and recognized that I've made a mistake when relied on the existing coding.
> A simple example:
> create extension pageinspect;
> create extension pg_trgm;
>
> create table test_trgm(t text);
> create index trgm_idx on test_trgm using gist (t gist_trgm_ops);
> insert into test_trgm select to_char(g, 'textFM000')  from generate_series(1, 1000) g;
> select gist_page_items(get_raw_page('trgm_idx', 0), 'trgm_idx'::regclass);
> select gist_page_items(get_raw_page('trgm_idx', 1), 'trgm_idx'::regclass);
>
> (I chose gist_trgm_ops because that opclass defined as follows:
> CREATE OPERATOR CLASS gist_trgm_ops FOR TYPE text USING gist ... STORAGE gtrgm)
> shows garbage values:
>                gist_page_items
> ---------------------------------------------
>  (1,"(1,65535)",32,f,"(t)=(\x02\x7F\x7F_ߟ)")
>  (2,"(3,65535)",32,f,"(t)=(\x02')")
>  (3,"(2,65535)",32,f,"(t)=(\x02߽)")
>  (4,"(5,65535)",32,f,"(t)=(\x02\x7F)")
>  (5,"(4,65535)",32,f,"(t)=(\x02w?\x7F)")
>  (6,"(6,65535)",32,f,"(t)=(\x02/)")
> (6 rows)

Fun.

> It means that pageinspect tries to print stored gtrgm values as text, and I
> believe that's the third problem here.

And it is not possible to display them as its output function says.
People could just use the bytea flavor in this case to show such items

> Moreover, for the point_ops class opcintype is point, but opckeytype is box,
> thus indexes test_gist_idx and test_gist_idx_inc used in the test in fact
> contain boxes, not points. So the fixed gist_page_items() produces different
> output even for existing test cases. (Please look at the patch attached.)
> I'm somewhat confused with the output:

Hmm.  I was actually looking at something pretty close to that
yesterday, but discarded the thought when I sent v3.  Included
attributes don't support expressions, but key attributes do, and the
generated output was a bit confusing when not applying parenthesis for
each attribute printed.  For example, take this case:
CREATE EXTENSION pageinspect;
CREATE TABLE test_gist AS SELECT point(i,i) p, i i FROM
  generate_series(1, 1000) i;
CREATE INDEX test_gist_idx_inc ON test_gist USING
  gist (p, point(i,i), (point(i, i)));
SELECT keys FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
  WHERE itemoffset = 1;

On HEAD or with v3, I get that, which is parseable:
                                                            keys
     

-----------------------------------------------------------------------------------------------------------------------------
 (p, point(i::double precision, i::double precision), point(i::double precision, i::double precision))=((1,1), (1,1),
(1,1))
(1 row)

v4 would give that once you include the full contents of the page items:
                                                                     keys
                       

-----------------------------------------------------------------------------------------------------------------------------------------------
 (p, point(i::double precision, i::double precision), point(i::double precision, i::double precision))=((1,1),(1,1),
(1,1),(1,1),(1,1),(1,1)) 
(1 row)

So, as you say, we should really add more parenthesis in this case.
Even with that, we cannot add too many of them, or the output could be
confusing.  For example, appending always parenthesis for included
attributes could be seen as an expression, but they can never be that.
So my take would be something a little bit more tricky, as of:
- Show NULL as it is.
- If dealing with a key attribute, always add parenthesis to it to
clarify that we are interested in the opckeytype.
- If dealing with an included attribute, don't add parenthesis at
all.

I have expanded the tests to show how this applies for more data
types, like point or integers.  Any thoughts about the v5 attached?
--
Michael

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?

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.

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.
--
Michael

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

On Thu, May 18, 2023 at 09:00:00AM +0300, Alexander Lakhin wrote:
> 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.

Ok, cool.  Let's go with v5 for now, then.

The format output could be tweaked forever, so if there are arguments
for more improvements, we could always discuss that later as adapted
and the tests will be here to provide coverage.  I am planning to
apply and backpatch that down to v14 tomorrow.
--
Michael

On Thu, May 18, 2023 at 04:16:24PM +0900, Michael Paquier wrote:
> The format output could be tweaked forever, so if there are arguments
> for more improvements, we could always discuss that later as adapted
> and the tests will be here to provide coverage.  I am planning to
> apply and backpatch that down to v14 tomorrow.

Well, I have woken up on this one and using parentheses was still
confusing when I attached composite types into the INCLUDE clause, so
I have looked again at record_out() and hstore, and noted that the
part where we apply the double quotes to the values is actually ~15
lines.  With that, it is easily possible to apply it to all the
values, giving a representation close to how rows display for a nice
result.

So, at the end, I have used that, reduced a bit the scope of the
tests, and applied the result down to 14.  Note the trick with
regexp_replace() to avoid some issues I noticed with alignment when
using -m32 for the non-leaf page when using INCLUDE.  The
documentation needed also a refresh to show the new output generated.
--
Michael