pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
11+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-05-19 e7bff46e50 2dd7782217 e72580232c pageinspect: Fix gist_page_items() with included columns
* BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
@ 2023-04-04 13:00 PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: PG Bug reporting form @ 2023-04-04 13:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
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.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
@ 2023-05-12 10:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Lakhin @ 2023-05-12 10:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
v1-01-fix-gist_page_items-for-index-with-INCLUDE.patch (text/x-patch, 16.3K) download
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index 460bef3037..cd10c34960 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -107,4 +107,180 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
(1 row)
+-- Test gist_page_items with pages of an index containing non-key columns
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t);
+SELECT gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc');
+ gist_page_items
+------------------------------------------
+ (1,"(1,65535)",40,f,"(p)=((156,156))")
+ (2,"(2,65535)",40,f,"(p)=((312,312))")
+ (3,"(3,65535)",40,f,"(p)=((468,468))")
+ (4,"(4,65535)",40,f,"(p)=((624,624))")
+ (5,"(5,65535)",40,f,"(p)=((718,718))")
+ (6,"(6,65535)",40,f,"(p)=((812,812))")
+ (7,"(7,65535)",40,f,"(p)=((906,906))")
+ (8,"(8,65535)",40,f,"(p)=((1000,1000))")
+(8 rows)
+
+SELECT gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc');
+ gist_page_items
+------------------------------------------------------------------
+ (1,"(0,1)",48,f,"(p) INCLUDE (t)=((1,1)) INCLUDE (1)")
+ (2,"(0,2)",48,f,"(p) INCLUDE (t)=((2,2)) INCLUDE (2)")
+ (3,"(0,3)",48,f,"(p) INCLUDE (t)=((3,3)) INCLUDE (3)")
+ (4,"(0,4)",48,f,"(p) INCLUDE (t)=((4,4)) INCLUDE (4)")
+ (5,"(0,5)",48,f,"(p) INCLUDE (t)=((5,5)) INCLUDE (5)")
+ (6,"(0,6)",48,f,"(p) INCLUDE (t)=((6,6)) INCLUDE (6)")
+ (7,"(0,7)",48,f,"(p) INCLUDE (t)=((7,7)) INCLUDE (7)")
+ (8,"(0,8)",48,f,"(p) INCLUDE (t)=((8,8)) INCLUDE (8)")
+ (9,"(0,9)",48,f,"(p) INCLUDE (t)=((9,9)) INCLUDE (9)")
+ (10,"(0,10)",48,f,"(p) INCLUDE (t)=((10,10)) INCLUDE (10)")
+ (11,"(0,11)",48,f,"(p) INCLUDE (t)=((11,11)) INCLUDE (11)")
+ (12,"(0,12)",48,f,"(p) INCLUDE (t)=((12,12)) INCLUDE (12)")
+ (13,"(0,13)",48,f,"(p) INCLUDE (t)=((13,13)) INCLUDE (13)")
+ (14,"(0,14)",48,f,"(p) INCLUDE (t)=((14,14)) INCLUDE (14)")
+ (15,"(0,15)",48,f,"(p) INCLUDE (t)=((15,15)) INCLUDE (15)")
+ (16,"(0,16)",48,f,"(p) INCLUDE (t)=((16,16)) INCLUDE (16)")
+ (17,"(0,17)",48,f,"(p) INCLUDE (t)=((17,17)) INCLUDE (17)")
+ (18,"(0,18)",48,f,"(p) INCLUDE (t)=((18,18)) INCLUDE (18)")
+ (19,"(0,19)",48,f,"(p) INCLUDE (t)=((19,19)) INCLUDE (19)")
+ (20,"(0,20)",48,f,"(p) INCLUDE (t)=((20,20)) INCLUDE (20)")
+ (21,"(0,21)",48,f,"(p) INCLUDE (t)=((21,21)) INCLUDE (21)")
+ (22,"(0,22)",48,f,"(p) INCLUDE (t)=((22,22)) INCLUDE (22)")
+ (23,"(0,23)",48,f,"(p) INCLUDE (t)=((23,23)) INCLUDE (23)")
+ (24,"(0,24)",48,f,"(p) INCLUDE (t)=((24,24)) INCLUDE (24)")
+ (25,"(0,25)",48,f,"(p) INCLUDE (t)=((25,25)) INCLUDE (25)")
+ (26,"(0,26)",48,f,"(p) INCLUDE (t)=((26,26)) INCLUDE (26)")
+ (27,"(0,27)",48,f,"(p) INCLUDE (t)=((27,27)) INCLUDE (27)")
+ (28,"(0,28)",48,f,"(p) INCLUDE (t)=((28,28)) INCLUDE (28)")
+ (29,"(0,29)",48,f,"(p) INCLUDE (t)=((29,29)) INCLUDE (29)")
+ (30,"(0,30)",48,f,"(p) INCLUDE (t)=((30,30)) INCLUDE (30)")
+ (31,"(0,31)",48,f,"(p) INCLUDE (t)=((31,31)) INCLUDE (31)")
+ (32,"(0,32)",48,f,"(p) INCLUDE (t)=((32,32)) INCLUDE (32)")
+ (33,"(0,33)",48,f,"(p) INCLUDE (t)=((33,33)) INCLUDE (33)")
+ (34,"(0,34)",48,f,"(p) INCLUDE (t)=((34,34)) INCLUDE (34)")
+ (35,"(0,35)",48,f,"(p) INCLUDE (t)=((35,35)) INCLUDE (35)")
+ (36,"(0,36)",48,f,"(p) INCLUDE (t)=((36,36)) INCLUDE (36)")
+ (37,"(0,37)",48,f,"(p) INCLUDE (t)=((37,37)) INCLUDE (37)")
+ (38,"(0,38)",48,f,"(p) INCLUDE (t)=((38,38)) INCLUDE (38)")
+ (39,"(0,39)",48,f,"(p) INCLUDE (t)=((39,39)) INCLUDE (39)")
+ (40,"(0,40)",48,f,"(p) INCLUDE (t)=((40,40)) INCLUDE (40)")
+ (41,"(0,41)",48,f,"(p) INCLUDE (t)=((41,41)) INCLUDE (41)")
+ (42,"(0,42)",48,f,"(p) INCLUDE (t)=((42,42)) INCLUDE (42)")
+ (43,"(0,43)",48,f,"(p) INCLUDE (t)=((43,43)) INCLUDE (43)")
+ (44,"(0,44)",48,f,"(p) INCLUDE (t)=((44,44)) INCLUDE (44)")
+ (45,"(0,45)",48,f,"(p) INCLUDE (t)=((45,45)) INCLUDE (45)")
+ (46,"(0,46)",48,f,"(p) INCLUDE (t)=((46,46)) INCLUDE (46)")
+ (47,"(0,47)",48,f,"(p) INCLUDE (t)=((47,47)) INCLUDE (47)")
+ (48,"(0,48)",48,f,"(p) INCLUDE (t)=((48,48)) INCLUDE (48)")
+ (49,"(0,49)",48,f,"(p) INCLUDE (t)=((49,49)) INCLUDE (49)")
+ (50,"(0,50)",48,f,"(p) INCLUDE (t)=((50,50)) INCLUDE (50)")
+ (51,"(0,51)",48,f,"(p) INCLUDE (t)=((51,51)) INCLUDE (51)")
+ (52,"(0,52)",48,f,"(p) INCLUDE (t)=((52,52)) INCLUDE (52)")
+ (53,"(0,53)",48,f,"(p) INCLUDE (t)=((53,53)) INCLUDE (53)")
+ (54,"(0,54)",48,f,"(p) INCLUDE (t)=((54,54)) INCLUDE (54)")
+ (55,"(0,55)",48,f,"(p) INCLUDE (t)=((55,55)) INCLUDE (55)")
+ (56,"(0,56)",48,f,"(p) INCLUDE (t)=((56,56)) INCLUDE (56)")
+ (57,"(0,57)",48,f,"(p) INCLUDE (t)=((57,57)) INCLUDE (57)")
+ (58,"(0,58)",48,f,"(p) INCLUDE (t)=((58,58)) INCLUDE (58)")
+ (59,"(0,59)",48,f,"(p) INCLUDE (t)=((59,59)) INCLUDE (59)")
+ (60,"(0,60)",48,f,"(p) INCLUDE (t)=((60,60)) INCLUDE (60)")
+ (61,"(0,61)",48,f,"(p) INCLUDE (t)=((61,61)) INCLUDE (61)")
+ (62,"(0,62)",48,f,"(p) INCLUDE (t)=((62,62)) INCLUDE (62)")
+ (63,"(0,63)",48,f,"(p) INCLUDE (t)=((63,63)) INCLUDE (63)")
+ (64,"(0,64)",48,f,"(p) INCLUDE (t)=((64,64)) INCLUDE (64)")
+ (65,"(0,65)",48,f,"(p) INCLUDE (t)=((65,65)) INCLUDE (65)")
+ (66,"(0,66)",48,f,"(p) INCLUDE (t)=((66,66)) INCLUDE (66)")
+ (67,"(0,67)",48,f,"(p) INCLUDE (t)=((67,67)) INCLUDE (67)")
+ (68,"(0,68)",48,f,"(p) INCLUDE (t)=((68,68)) INCLUDE (68)")
+ (69,"(0,69)",48,f,"(p) INCLUDE (t)=((69,69)) INCLUDE (69)")
+ (70,"(0,70)",48,f,"(p) INCLUDE (t)=((70,70)) INCLUDE (70)")
+ (71,"(0,71)",48,f,"(p) INCLUDE (t)=((71,71)) INCLUDE (71)")
+ (72,"(0,72)",48,f,"(p) INCLUDE (t)=((72,72)) INCLUDE (72)")
+ (73,"(0,73)",48,f,"(p) INCLUDE (t)=((73,73)) INCLUDE (73)")
+ (74,"(0,74)",48,f,"(p) INCLUDE (t)=((74,74)) INCLUDE (74)")
+ (75,"(0,75)",48,f,"(p) INCLUDE (t)=((75,75)) INCLUDE (75)")
+ (76,"(0,76)",48,f,"(p) INCLUDE (t)=((76,76)) INCLUDE (76)")
+ (77,"(0,77)",48,f,"(p) INCLUDE (t)=((77,77)) INCLUDE (77)")
+ (78,"(0,78)",48,f,"(p) INCLUDE (t)=((78,78)) INCLUDE (78)")
+ (79,"(0,79)",48,f,"(p) INCLUDE (t)=((79,79)) INCLUDE (79)")
+ (80,"(0,80)",48,f,"(p) INCLUDE (t)=((80,80)) INCLUDE (80)")
+ (81,"(0,81)",48,f,"(p) INCLUDE (t)=((81,81)) INCLUDE (81)")
+ (82,"(0,82)",48,f,"(p) INCLUDE (t)=((82,82)) INCLUDE (82)")
+ (83,"(0,83)",48,f,"(p) INCLUDE (t)=((83,83)) INCLUDE (83)")
+ (84,"(0,84)",48,f,"(p) INCLUDE (t)=((84,84)) INCLUDE (84)")
+ (85,"(0,85)",48,f,"(p) INCLUDE (t)=((85,85)) INCLUDE (85)")
+ (86,"(0,86)",48,f,"(p) INCLUDE (t)=((86,86)) INCLUDE (86)")
+ (87,"(0,87)",48,f,"(p) INCLUDE (t)=((87,87)) INCLUDE (87)")
+ (88,"(0,88)",48,f,"(p) INCLUDE (t)=((88,88)) INCLUDE (88)")
+ (89,"(0,89)",48,f,"(p) INCLUDE (t)=((89,89)) INCLUDE (89)")
+ (90,"(0,90)",48,f,"(p) INCLUDE (t)=((90,90)) INCLUDE (90)")
+ (91,"(0,91)",48,f,"(p) INCLUDE (t)=((91,91)) INCLUDE (91)")
+ (92,"(0,92)",48,f,"(p) INCLUDE (t)=((92,92)) INCLUDE (92)")
+ (93,"(0,93)",48,f,"(p) INCLUDE (t)=((93,93)) INCLUDE (93)")
+ (94,"(0,94)",48,f,"(p) INCLUDE (t)=((94,94)) INCLUDE (94)")
+ (95,"(0,95)",48,f,"(p) INCLUDE (t)=((95,95)) INCLUDE (95)")
+ (96,"(0,96)",48,f,"(p) INCLUDE (t)=((96,96)) INCLUDE (96)")
+ (97,"(0,97)",48,f,"(p) INCLUDE (t)=((97,97)) INCLUDE (97)")
+ (98,"(0,98)",48,f,"(p) INCLUDE (t)=((98,98)) INCLUDE (98)")
+ (99,"(0,99)",48,f,"(p) INCLUDE (t)=((99,99)) INCLUDE (99)")
+ (100,"(0,100)",48,f,"(p) INCLUDE (t)=((100,100)) INCLUDE (100)")
+ (101,"(0,101)",48,f,"(p) INCLUDE (t)=((101,101)) INCLUDE (101)")
+ (102,"(0,102)",48,f,"(p) INCLUDE (t)=((102,102)) INCLUDE (102)")
+ (103,"(0,103)",48,f,"(p) INCLUDE (t)=((103,103)) INCLUDE (103)")
+ (104,"(0,104)",48,f,"(p) INCLUDE (t)=((104,104)) INCLUDE (104)")
+ (105,"(0,105)",48,f,"(p) INCLUDE (t)=((105,105)) INCLUDE (105)")
+ (106,"(0,106)",48,f,"(p) INCLUDE (t)=((106,106)) INCLUDE (106)")
+ (107,"(0,107)",48,f,"(p) INCLUDE (t)=((107,107)) INCLUDE (107)")
+ (108,"(0,108)",48,f,"(p) INCLUDE (t)=((108,108)) INCLUDE (108)")
+ (109,"(0,109)",48,f,"(p) INCLUDE (t)=((109,109)) INCLUDE (109)")
+ (110,"(0,110)",48,f,"(p) INCLUDE (t)=((110,110)) INCLUDE (110)")
+ (111,"(0,111)",48,f,"(p) INCLUDE (t)=((111,111)) INCLUDE (111)")
+ (112,"(0,112)",48,f,"(p) INCLUDE (t)=((112,112)) INCLUDE (112)")
+ (113,"(0,113)",48,f,"(p) INCLUDE (t)=((113,113)) INCLUDE (113)")
+ (114,"(0,114)",48,f,"(p) INCLUDE (t)=((114,114)) INCLUDE (114)")
+ (115,"(0,115)",48,f,"(p) INCLUDE (t)=((115,115)) INCLUDE (115)")
+ (116,"(0,116)",48,f,"(p) INCLUDE (t)=((116,116)) INCLUDE (116)")
+ (117,"(0,117)",48,f,"(p) INCLUDE (t)=((117,117)) INCLUDE (117)")
+ (118,"(0,118)",48,f,"(p) INCLUDE (t)=((118,118)) INCLUDE (118)")
+ (119,"(0,119)",48,f,"(p) INCLUDE (t)=((119,119)) INCLUDE (119)")
+ (120,"(0,120)",48,f,"(p) INCLUDE (t)=((120,120)) INCLUDE (120)")
+ (121,"(0,121)",48,f,"(p) INCLUDE (t)=((121,121)) INCLUDE (121)")
+ (122,"(0,122)",48,f,"(p) INCLUDE (t)=((122,122)) INCLUDE (122)")
+ (123,"(0,123)",48,f,"(p) INCLUDE (t)=((123,123)) INCLUDE (123)")
+ (124,"(0,124)",48,f,"(p) INCLUDE (t)=((124,124)) INCLUDE (124)")
+ (125,"(0,125)",48,f,"(p) INCLUDE (t)=((125,125)) INCLUDE (125)")
+ (126,"(0,126)",48,f,"(p) INCLUDE (t)=((126,126)) INCLUDE (126)")
+ (127,"(0,127)",48,f,"(p) INCLUDE (t)=((127,127)) INCLUDE (127)")
+ (128,"(0,128)",48,f,"(p) INCLUDE (t)=((128,128)) INCLUDE (128)")
+ (129,"(0,129)",48,f,"(p) INCLUDE (t)=((129,129)) INCLUDE (129)")
+ (130,"(0,130)",48,f,"(p) INCLUDE (t)=((130,130)) INCLUDE (130)")
+ (131,"(0,131)",48,f,"(p) INCLUDE (t)=((131,131)) INCLUDE (131)")
+ (132,"(0,132)",48,f,"(p) INCLUDE (t)=((132,132)) INCLUDE (132)")
+ (133,"(0,133)",48,f,"(p) INCLUDE (t)=((133,133)) INCLUDE (133)")
+ (134,"(0,134)",48,f,"(p) INCLUDE (t)=((134,134)) INCLUDE (134)")
+ (135,"(0,135)",48,f,"(p) INCLUDE (t)=((135,135)) INCLUDE (135)")
+ (136,"(0,136)",48,f,"(p) INCLUDE (t)=((136,136)) INCLUDE (136)")
+ (137,"(0,137)",48,f,"(p) INCLUDE (t)=((137,137)) INCLUDE (137)")
+ (138,"(0,138)",48,f,"(p) INCLUDE (t)=((138,138)) INCLUDE (138)")
+ (139,"(0,139)",48,f,"(p) INCLUDE (t)=((139,139)) INCLUDE (139)")
+ (140,"(0,140)",48,f,"(p) INCLUDE (t)=((140,140)) INCLUDE (140)")
+ (141,"(0,141)",48,f,"(p) INCLUDE (t)=((141,141)) INCLUDE (141)")
+ (142,"(0,142)",48,f,"(p) INCLUDE (t)=((142,142)) INCLUDE (142)")
+ (143,"(0,143)",48,f,"(p) INCLUDE (t)=((143,143)) INCLUDE (143)")
+ (144,"(0,144)",48,f,"(p) INCLUDE (t)=((144,144)) INCLUDE (144)")
+ (145,"(0,145)",48,f,"(p) INCLUDE (t)=((145,145)) INCLUDE (145)")
+ (146,"(0,146)",48,f,"(p) INCLUDE (t)=((146,146)) INCLUDE (146)")
+ (147,"(0,147)",48,f,"(p) INCLUDE (t)=((147,147)) INCLUDE (147)")
+ (148,"(0,148)",48,f,"(p) INCLUDE (t)=((148,148)) INCLUDE (148)")
+ (149,"(0,149)",48,f,"(p) INCLUDE (t)=((149,149)) INCLUDE (149)")
+ (150,"(0,150)",48,f,"(p) INCLUDE (t)=((150,150)) INCLUDE (150)")
+ (151,"(0,151)",48,f,"(p) INCLUDE (t)=((151,151)) INCLUDE (151)")
+ (152,"(0,152)",48,f,"(p) INCLUDE (t)=((152,152)) INCLUDE (152)")
+ (153,"(0,153)",48,f,"(p) INCLUDE (t)=((153,153)) INCLUDE (153)")
+ (154,"(0,154)",48,f,"(p) INCLUDE (t)=((154,154)) INCLUDE (154)")
+ (155,"(0,155)",48,f,"(p) INCLUDE (t)=((155,155)) INCLUDE (155)")
+ (156,"(0,156)",48,f,"(p) INCLUDE (t)=((156,156)) INCLUDE (156)")
+(156 rows)
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 3dca7f1318..422290311c 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -24,6 +24,8 @@
#include "utils/rel.h"
#include "utils/pg_lsn.h"
#include "utils/varlena.h"
+#include "utils/ruleutils.h"
+#include "utils/lsyscache.h"
PG_FUNCTION_INFO_V1(gist_page_opaque_info);
PG_FUNCTION_INFO_V1(gist_page_items);
@@ -198,9 +200,12 @@ gist_page_items(PG_FUNCTION_ARGS)
Oid indexRelid = PG_GETARG_OID(1);
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Relation indexRel;
+ TupleDesc tupdesc;
Page page;
+ uint16 flagbits;
OffsetNumber offset;
OffsetNumber maxoff = InvalidOffsetNumber;
+ char *index_columns;
if (!superuser())
ereport(ERROR,
@@ -226,6 +231,19 @@ gist_page_items(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ flagbits = GistPageGetOpaque(page)->flags;
+ if (flagbits & F_LEAF)
+ {
+ tupdesc = RelationGetDescr(indexRel);
+ index_columns = pg_get_indexdef_columns_with_included(indexRelid, true);
+ }
+ else
+ {
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
+ index_columns = pg_get_indexdef_columns(indexRelid, true);
+ }
+
/* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */
if (GistPageIsDeleted(page))
elog(NOTICE, "page is deleted");
@@ -242,7 +260,8 @@ gist_page_items(PG_FUNCTION_ARGS)
IndexTuple itup;
Datum itup_values[INDEX_MAX_KEYS];
bool itup_isnull[INDEX_MAX_KEYS];
- char *key_desc;
+ StringInfoData buf;
+ int i;
id = PageGetItemId(page, offset);
@@ -251,7 +270,7 @@ gist_page_items(PG_FUNCTION_ARGS)
itup = (IndexTuple) PageGetItem(page, id);
- index_deform_tuple(itup, RelationGetDescr(indexRel),
+ index_deform_tuple(itup, tupdesc,
itup_values, itup_isnull);
memset(nulls, 0, sizeof(nulls));
@@ -261,9 +280,56 @@ gist_page_items(PG_FUNCTION_ARGS)
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
- key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull);
- if (key_desc)
- values[4] = CStringGetTextDatum(key_desc);
+ if (index_columns)
+ {
+ initStringInfo(&buf);
+ appendStringInfo(&buf, "(%s)=(", index_columns);
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ char *val;
+
+ if (itup_isnull[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+
+ if (i < IndexRelationGetNumberOfKeyAttributes(indexRel))
+ {
+ /*
+ * Use rd_opcintype for key columns as in
+ * BuildIndexValueDescription().
+ */
+ getTypeOutputInfo(indexRel->rd_opcintype[i],
+ &foutoid, &typisvarlena);
+ }
+ else
+ {
+ getTypeOutputInfo(tupdesc->attrs[i].atttypid,
+ &foutoid, &typisvarlena);
+ }
+ val = OidOutputFunctionCall(foutoid, itup_values[i]);
+ }
+
+ if (i == IndexRelationGetNumberOfKeyAttributes(indexRel))
+ {
+ appendStringInfoString(&buf, ") INCLUDE (");
+ }
+ else if (i > 0)
+ {
+ appendStringInfoString(&buf, ", ");
+ }
+
+ appendStringInfoString(&buf, val);
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ values[4] = CStringGetTextDatum(buf.data);
+ nulls[4] = false;
+ }
else
{
values[4] = (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 4787b784a4..16cc14bcac 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -52,4 +52,9 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex'));
SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass);
SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
+-- Test gist_page_items with pages of an index containing non-key columns
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t);
+SELECT gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc');
+SELECT gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc');
+
DROP TABLE test_gist;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 60f9d08d5d..71f650868a 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1215,6 +1215,20 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty)
prettyFlags, false);
}
+/* Internal version that reports also the non-key-column definitions */
+char *
+pg_get_indexdef_columns_with_included(Oid indexrelid, bool pretty)
+{
+ int prettyFlags;
+
+ prettyFlags = GET_PRETTY_FLAGS(pretty);
+
+ return pg_get_indexdef_worker(indexrelid, 0, NULL,
+ true, false,
+ false, false,
+ prettyFlags, false);
+}
+
/*
* Internal workhorse to decompile an index definition.
*
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 1a42d9f39b..d221724cc1 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -23,6 +23,8 @@ struct PlannedStmt;
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+extern char *pg_get_indexdef_columns_with_included(Oid indexrelid,
+ bool pretty);
extern char *pg_get_querydef(Query *query, bool pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
@ 2023-05-15 00:41 ` Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2023-05-15 00:41 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
@ 2023-05-15 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Lakhin @ 2023-05-15 08:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
v2-01-fix-gist_page_items-for-index-with-INCLUDE.patch (text/x-patch, 6.7K) download
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index 460bef3037..b7767cdb42 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -107,4 +107,21 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
(1 row)
+-- Test gist_page_items with pages of an index containing non-key columns
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-----------+---------+------+-----------------
+ 1 | (1,65535) | 40 | f | (p)=((135,135))
+(1 row)
+
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-------+---------+------+----------------------------------------------
+ 1 | (0,1) | 56 | f | (p) INCLUDE (t, i)=((1,1)) INCLUDE (1, null)
+(1 row)
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 3dca7f1318..422290311c 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -24,6 +24,8 @@
#include "utils/rel.h"
#include "utils/pg_lsn.h"
#include "utils/varlena.h"
+#include "utils/ruleutils.h"
+#include "utils/lsyscache.h"
PG_FUNCTION_INFO_V1(gist_page_opaque_info);
PG_FUNCTION_INFO_V1(gist_page_items);
@@ -198,9 +200,12 @@ gist_page_items(PG_FUNCTION_ARGS)
Oid indexRelid = PG_GETARG_OID(1);
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Relation indexRel;
+ TupleDesc tupdesc;
Page page;
+ uint16 flagbits;
OffsetNumber offset;
OffsetNumber maxoff = InvalidOffsetNumber;
+ char *index_columns;
if (!superuser())
ereport(ERROR,
@@ -226,6 +231,19 @@ gist_page_items(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ flagbits = GistPageGetOpaque(page)->flags;
+ if (flagbits & F_LEAF)
+ {
+ tupdesc = RelationGetDescr(indexRel);
+ index_columns = pg_get_indexdef_columns_with_included(indexRelid, true);
+ }
+ else
+ {
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
+ index_columns = pg_get_indexdef_columns(indexRelid, true);
+ }
+
/* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */
if (GistPageIsDeleted(page))
elog(NOTICE, "page is deleted");
@@ -242,7 +260,8 @@ gist_page_items(PG_FUNCTION_ARGS)
IndexTuple itup;
Datum itup_values[INDEX_MAX_KEYS];
bool itup_isnull[INDEX_MAX_KEYS];
- char *key_desc;
+ StringInfoData buf;
+ int i;
id = PageGetItemId(page, offset);
@@ -251,7 +270,7 @@ gist_page_items(PG_FUNCTION_ARGS)
itup = (IndexTuple) PageGetItem(page, id);
- index_deform_tuple(itup, RelationGetDescr(indexRel),
+ index_deform_tuple(itup, tupdesc,
itup_values, itup_isnull);
memset(nulls, 0, sizeof(nulls));
@@ -261,9 +280,56 @@ gist_page_items(PG_FUNCTION_ARGS)
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
- key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull);
- if (key_desc)
- values[4] = CStringGetTextDatum(key_desc);
+ if (index_columns)
+ {
+ initStringInfo(&buf);
+ appendStringInfo(&buf, "(%s)=(", index_columns);
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ char *val;
+
+ if (itup_isnull[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+
+ if (i < IndexRelationGetNumberOfKeyAttributes(indexRel))
+ {
+ /*
+ * Use rd_opcintype for key columns as in
+ * BuildIndexValueDescription().
+ */
+ getTypeOutputInfo(indexRel->rd_opcintype[i],
+ &foutoid, &typisvarlena);
+ }
+ else
+ {
+ getTypeOutputInfo(tupdesc->attrs[i].atttypid,
+ &foutoid, &typisvarlena);
+ }
+ val = OidOutputFunctionCall(foutoid, itup_values[i]);
+ }
+
+ if (i == IndexRelationGetNumberOfKeyAttributes(indexRel))
+ {
+ appendStringInfoString(&buf, ") INCLUDE (");
+ }
+ else if (i > 0)
+ {
+ appendStringInfoString(&buf, ", ");
+ }
+
+ appendStringInfoString(&buf, val);
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ values[4] = CStringGetTextDatum(buf.data);
+ nulls[4] = false;
+ }
else
{
values[4] = (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 4787b784a4..398af01c66 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -52,4 +52,12 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex'));
SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass);
SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
+-- Test gist_page_items with pages of an index containing non-key columns
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+
DROP TABLE test_gist;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 60f9d08d5d..71f650868a 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1215,6 +1215,20 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty)
prettyFlags, false);
}
+/* Internal version that reports also the non-key-column definitions */
+char *
+pg_get_indexdef_columns_with_included(Oid indexrelid, bool pretty)
+{
+ int prettyFlags;
+
+ prettyFlags = GET_PRETTY_FLAGS(pretty);
+
+ return pg_get_indexdef_worker(indexrelid, 0, NULL,
+ true, false,
+ false, false,
+ prettyFlags, false);
+}
+
/*
* Internal workhorse to decompile an index definition.
*
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 1a42d9f39b..d221724cc1 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -23,6 +23,8 @@ struct PlannedStmt;
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+extern char *pg_get_indexdef_columns_with_included(Oid indexrelid,
+ bool pretty);
extern char *pg_get_querydef(Query *query, bool pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
@ 2023-05-17 05:23 ` Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2023-05-17 05:23 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
v3-01-fix-gist_page_items-for-index-with-INCLUDE.patch (text/x-diff, 8.3K) download
From 0d39cd7dfc1afab291aa4a5ee979a8f9cb624905 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 17 May 2023 14:13:35 +0900
Subject: [PATCH v3] pageinspect: Fix gist_page_items with included columns
Blah.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/17884-cb8c326522977acb@postgresql.org
Backpatch-through: 14
---
src/include/utils/ruleutils.h | 5 ++
src/backend/utils/adt/ruleutils.c | 16 +++++
contrib/pageinspect/expected/gist.out | 19 ++++++
contrib/pageinspect/gistfuncs.c | 86 +++++++++++++++++++++++++--
contrib/pageinspect/sql/gist.sql | 10 ++++
5 files changed, 130 insertions(+), 6 deletions(-)
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 1a42d9f39b..b006d9d475 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -20,9 +20,14 @@
struct Plan; /* avoid including plannodes.h here */
struct PlannedStmt;
+/* Flags for pg_get_indexdef_columns_extended() */
+#define RULE_INDEXDEF_PRETTY 0x01
+#define RULE_INDEXDEF_KEYS_ONLY 0x02 /* ignore included attributes */
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+extern char *pg_get_indexdef_columns_extended(Oid indexrelid,
+ bits16 flags);
extern char *pg_get_querydef(Query *query, bool pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index e93d66a7ec..6d673493cb 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1215,6 +1215,22 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty)
prettyFlags, false);
}
+/* Internal version, extensible with flags to control its behavior */
+char *
+pg_get_indexdef_columns_extended(Oid indexrelid, bits16 flags)
+{
+ bool pretty = ((flags & RULE_INDEXDEF_PRETTY) != 0);
+ bool keys_only = ((flags & RULE_INDEXDEF_KEYS_ONLY) != 0);
+ int prettyFlags;
+
+ prettyFlags = GET_PRETTY_FLAGS(pretty);
+
+ return pg_get_indexdef_worker(indexrelid, 0, NULL,
+ true, keys_only,
+ false, false,
+ prettyFlags, false);
+}
+
/*
* Internal workhorse to decompile an index definition.
*
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index 460bef3037..f55000349f 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -107,4 +107,23 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
(1 row)
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-----------+---------+------+-----------------
+ 1 | (1,65535) | 40 | f | (p)=((135,135))
+(1 row)
+
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-------+---------+------+----------------------------------------------
+ 1 | (0,1) | 56 | f | (p) INCLUDE (t, i)=((1,1)) INCLUDE (1, null)
+(1 row)
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 3dca7f1318..fe7b06c331 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -21,8 +21,10 @@
#include "storage/itemptr.h"
#include "utils/array.h"
#include "utils/builtins.h"
-#include "utils/rel.h"
#include "utils/pg_lsn.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/varlena.h"
PG_FUNCTION_INFO_V1(gist_page_opaque_info);
@@ -198,9 +200,13 @@ gist_page_items(PG_FUNCTION_ARGS)
Oid indexRelid = PG_GETARG_OID(1);
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Relation indexRel;
+ TupleDesc tupdesc;
Page page;
+ uint16 flagbits;
+ bits16 printflags = 0;
OffsetNumber offset;
OffsetNumber maxoff = InvalidOffsetNumber;
+ char *index_columns;
if (!superuser())
ereport(ERROR,
@@ -226,6 +232,27 @@ gist_page_items(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ flagbits = GistPageGetOpaque(page)->flags;
+
+ /*
+ * Included attributes are added when dealing with leaf pages, discarded
+ * for non-leaf pages as these include only data for key attributes.
+ */
+ printflags |= RULE_INDEXDEF_PRETTY;
+ if (flagbits & F_LEAF)
+ {
+ tupdesc = RelationGetDescr(indexRel);
+ }
+ else
+ {
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
+ printflags |= RULE_INDEXDEF_KEYS_ONLY;
+ }
+
+ index_columns = pg_get_indexdef_columns_extended(indexRelid,
+ printflags);
+
/* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */
if (GistPageIsDeleted(page))
elog(NOTICE, "page is deleted");
@@ -242,7 +269,8 @@ gist_page_items(PG_FUNCTION_ARGS)
IndexTuple itup;
Datum itup_values[INDEX_MAX_KEYS];
bool itup_isnull[INDEX_MAX_KEYS];
- char *key_desc;
+ StringInfoData buf;
+ int i;
id = PageGetItemId(page, offset);
@@ -251,7 +279,7 @@ gist_page_items(PG_FUNCTION_ARGS)
itup = (IndexTuple) PageGetItem(page, id);
- index_deform_tuple(itup, RelationGetDescr(indexRel),
+ index_deform_tuple(itup, tupdesc,
itup_values, itup_isnull);
memset(nulls, 0, sizeof(nulls));
@@ -261,9 +289,55 @@ gist_page_items(PG_FUNCTION_ARGS)
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
- key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull);
- if (key_desc)
- values[4] = CStringGetTextDatum(key_desc);
+ if (index_columns)
+ {
+ initStringInfo(&buf);
+ appendStringInfo(&buf, "(%s)=(", index_columns);
+
+ /*
+ * This is similar to BuildIndexValueDescription(), though it
+ * covers included attributes.
+ */
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ char *val;
+
+ if (itup_isnull[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+ Oid attroid;
+
+ /*
+ * The provided data is not necessarily of the type stored
+ * in the index; rather it is of the GiST opclass's input
+ * type. So look at rd_opcintype and not the index
+ * tupdesc for key attributes.
+ */
+ if (i < IndexRelationGetNumberOfKeyAttributes(indexRel))
+ attroid = indexRel->rd_opcintype[i];
+ else
+ attroid = tupdesc->attrs[i].atttypid;
+
+ getTypeOutputInfo(attroid, &foutoid, &typisvarlena);
+ val = OidOutputFunctionCall(foutoid, itup_values[i]);
+ }
+
+ if (i == IndexRelationGetNumberOfKeyAttributes(indexRel))
+ appendStringInfoString(&buf, ") INCLUDE (");
+ else if (i > 0)
+ appendStringInfoString(&buf, ", ");
+
+ appendStringInfoString(&buf, val);
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ values[4] = CStringGetTextDatum(buf.data);
+ nulls[4] = false;
+ }
else
{
values[4] = (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 4787b784a4..fb91aa48cd 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -52,4 +52,14 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex'));
SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass);
SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+
DROP TABLE test_gist;
--
2.40.1
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
@ 2023-05-17 14:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Lakhin @ 2023-05-17 14:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
v4-01-fix-gist_page_items-for-index-with-INCLUDE-and-diff-storage.patch (text/x-patch, 9.3K) download
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index 460bef3037..5ab09fbf2e 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -31,24 +31,24 @@ SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 2));
COMMIT;
SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 0), 'test_gist_idx');
- itemoffset | ctid | itemlen | dead | keys
-------------+-----------+---------+------+-------------------
- 1 | (1,65535) | 40 | f | (p)=((185,185))
- 2 | (2,65535) | 40 | f | (p)=((370,370))
- 3 | (3,65535) | 40 | f | (p)=((555,555))
- 4 | (4,65535) | 40 | f | (p)=((740,740))
- 5 | (5,65535) | 40 | f | (p)=((870,870))
- 6 | (6,65535) | 40 | f | (p)=((1000,1000))
+ itemoffset | ctid | itemlen | dead | keys
+------------+-----------+---------+------+-----------------------------
+ 1 | (1,65535) | 40 | f | (p)=((185,185),(1,1))
+ 2 | (2,65535) | 40 | f | (p)=((370,370),(186,186))
+ 3 | (3,65535) | 40 | f | (p)=((555,555),(371,371))
+ 4 | (4,65535) | 40 | f | (p)=((740,740),(556,556))
+ 5 | (5,65535) | 40 | f | (p)=((870,870),(741,741))
+ 6 | (6,65535) | 40 | f | (p)=((1000,1000),(871,871))
(6 rows)
SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 1), 'test_gist_idx') LIMIT 5;
- itemoffset | ctid | itemlen | dead | keys
-------------+-------+---------+------+-------------
- 1 | (0,1) | 40 | f | (p)=((1,1))
- 2 | (0,2) | 40 | f | (p)=((2,2))
- 3 | (0,3) | 40 | f | (p)=((3,3))
- 4 | (0,4) | 40 | f | (p)=((4,4))
- 5 | (0,5) | 40 | f | (p)=((5,5))
+ itemoffset | ctid | itemlen | dead | keys
+------------+-------+---------+------+-------------------
+ 1 | (0,1) | 40 | f | (p)=((1,1),(1,1))
+ 2 | (0,2) | 40 | f | (p)=((2,2),(2,2))
+ 3 | (0,3) | 40 | f | (p)=((3,3),(3,3))
+ 4 | (0,4) | 40 | f | (p)=((4,4),(4,4))
+ 5 | (0,5) | 40 | f | (p)=((5,5),(5,5))
(5 rows)
-- gist_page_items_bytea prints the raw key data as a bytea. The output of that is
@@ -107,4 +107,23 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
(1 row)
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-----------+---------+------+-----------------------
+ 1 | (1,65535) | 40 | f | (p)=((135,135),(1,1))
+(1 row)
+
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+ itemoffset | ctid | itemlen | dead | keys
+------------+-------+---------+------+----------------------------------------------------
+ 1 | (0,1) | 56 | f | (p) INCLUDE (t, i)=((1,1),(1,1)) INCLUDE (1, null)
+(1 row)
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 3dca7f1318..a51cdb32a0 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -21,8 +21,10 @@
#include "storage/itemptr.h"
#include "utils/array.h"
#include "utils/builtins.h"
-#include "utils/rel.h"
#include "utils/pg_lsn.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/varlena.h"
PG_FUNCTION_INFO_V1(gist_page_opaque_info);
@@ -198,9 +200,13 @@ gist_page_items(PG_FUNCTION_ARGS)
Oid indexRelid = PG_GETARG_OID(1);
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Relation indexRel;
+ TupleDesc tupdesc;
Page page;
+ uint16 flagbits;
+ bits16 printflags = 0;
OffsetNumber offset;
OffsetNumber maxoff = InvalidOffsetNumber;
+ char *index_columns;
if (!superuser())
ereport(ERROR,
@@ -226,6 +232,27 @@ gist_page_items(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ flagbits = GistPageGetOpaque(page)->flags;
+
+ /*
+ * Included attributes are added when dealing with leaf pages, discarded
+ * for non-leaf pages as these include only data for key attributes.
+ */
+ printflags |= RULE_INDEXDEF_PRETTY;
+ if (flagbits & F_LEAF)
+ {
+ tupdesc = RelationGetDescr(indexRel);
+ }
+ else
+ {
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
+ printflags |= RULE_INDEXDEF_KEYS_ONLY;
+ }
+
+ index_columns = pg_get_indexdef_columns_extended(indexRelid,
+ printflags);
+
/* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */
if (GistPageIsDeleted(page))
elog(NOTICE, "page is deleted");
@@ -242,7 +269,8 @@ gist_page_items(PG_FUNCTION_ARGS)
IndexTuple itup;
Datum itup_values[INDEX_MAX_KEYS];
bool itup_isnull[INDEX_MAX_KEYS];
- char *key_desc;
+ StringInfoData buf;
+ int i;
id = PageGetItemId(page, offset);
@@ -251,7 +279,7 @@ gist_page_items(PG_FUNCTION_ARGS)
itup = (IndexTuple) PageGetItem(page, id);
- index_deform_tuple(itup, RelationGetDescr(indexRel),
+ index_deform_tuple(itup, tupdesc,
itup_values, itup_isnull);
memset(nulls, 0, sizeof(nulls));
@@ -261,9 +289,41 @@ gist_page_items(PG_FUNCTION_ARGS)
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
- key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull);
- if (key_desc)
- values[4] = CStringGetTextDatum(key_desc);
+ if (index_columns)
+ {
+ initStringInfo(&buf);
+ appendStringInfo(&buf, "(%s)=(", index_columns);
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ char *val;
+
+ if (itup_isnull[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+ Oid typoid;
+
+ typoid = tupdesc->attrs[i].atttypid;
+ getTypeOutputInfo(typoid, &foutoid, &typisvarlena);
+ val = OidOutputFunctionCall(foutoid, itup_values[i]);
+ }
+
+ if (i == IndexRelationGetNumberOfKeyAttributes(indexRel))
+ appendStringInfoString(&buf, ") INCLUDE (");
+ else if (i > 0)
+ appendStringInfoString(&buf, ", ");
+
+ appendStringInfoString(&buf, val);
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ values[4] = CStringGetTextDatum(buf.data);
+ nulls[4] = false;
+ }
else
{
values[4] = (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 4787b784a4..fb91aa48cd 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -52,4 +52,14 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex'));
SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass);
SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i int;
+CREATE INDEX test_gist_idx_inc ON test_gist USING gist (p) INCLUDE (t, i);
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc')
+ WHERE itemoffset = 1;
+
DROP TABLE test_gist;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index e93d66a7ec..6d673493cb 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1215,6 +1215,22 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty)
prettyFlags, false);
}
+/* Internal version, extensible with flags to control its behavior */
+char *
+pg_get_indexdef_columns_extended(Oid indexrelid, bits16 flags)
+{
+ bool pretty = ((flags & RULE_INDEXDEF_PRETTY) != 0);
+ bool keys_only = ((flags & RULE_INDEXDEF_KEYS_ONLY) != 0);
+ int prettyFlags;
+
+ prettyFlags = GET_PRETTY_FLAGS(pretty);
+
+ return pg_get_indexdef_worker(indexrelid, 0, NULL,
+ true, keys_only,
+ false, false,
+ prettyFlags, false);
+}
+
/*
* Internal workhorse to decompile an index definition.
*
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 1a42d9f39b..b006d9d475 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -20,9 +20,14 @@
struct Plan; /* avoid including plannodes.h here */
struct PlannedStmt;
+/* Flags for pg_get_indexdef_columns_extended() */
+#define RULE_INDEXDEF_PRETTY 0x01
+#define RULE_INDEXDEF_KEYS_ONLY 0x02 /* ignore included attributes */
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+extern char *pg_get_indexdef_columns_extended(Oid indexrelid,
+ bits16 flags);
extern char *pg_get_querydef(Query *query, bool pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
@ 2023-05-18 01:50 ` Michael Paquier <michael@paquier.xyz>
2023-05-18 04:01 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2023-05-18 01:50 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
v5-01-fix-gist_page_items-for-index-with-INCLUDE-and-diff-storage.patch (text/x-diff, 13.6K) download
From 0680c5c6fc2134b6ba86daa4f5e0e27843acb42b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 18 May 2023 10:48:50 +0900
Subject: [PATCH v5] pageinspect: Fix gist_page_items with included columns
Blah.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/17884-cb8c326522977acb@postgresql.org
Backpatch-through: 14
---
src/include/utils/ruleutils.h | 5 ++
src/backend/utils/adt/ruleutils.c | 16 +++++
contrib/pageinspect/expected/gist.out | 89 ++++++++++++++++++++++-----
contrib/pageinspect/gistfuncs.c | 81 ++++++++++++++++++++++--
contrib/pageinspect/sql/gist.sql | 30 +++++++++
5 files changed, 200 insertions(+), 21 deletions(-)
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 1a42d9f39b..b006d9d475 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -20,9 +20,14 @@
struct Plan; /* avoid including plannodes.h here */
struct PlannedStmt;
+/* Flags for pg_get_indexdef_columns_extended() */
+#define RULE_INDEXDEF_PRETTY 0x01
+#define RULE_INDEXDEF_KEYS_ONLY 0x02 /* ignore included attributes */
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+extern char *pg_get_indexdef_columns_extended(Oid indexrelid,
+ bits16 flags);
extern char *pg_get_querydef(Query *query, bool pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index e93d66a7ec..6d673493cb 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1215,6 +1215,22 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty)
prettyFlags, false);
}
+/* Internal version, extensible with flags to control its behavior */
+char *
+pg_get_indexdef_columns_extended(Oid indexrelid, bits16 flags)
+{
+ bool pretty = ((flags & RULE_INDEXDEF_PRETTY) != 0);
+ bool keys_only = ((flags & RULE_INDEXDEF_KEYS_ONLY) != 0);
+ int prettyFlags;
+
+ prettyFlags = GET_PRETTY_FLAGS(pretty);
+
+ return pg_get_indexdef_worker(indexrelid, 0, NULL,
+ true, keys_only,
+ false, false,
+ prettyFlags, false);
+}
+
/*
* Internal workhorse to decompile an index definition.
*
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index 460bef3037..c240e51415 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -31,24 +31,24 @@ SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 2));
COMMIT;
SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 0), 'test_gist_idx');
- itemoffset | ctid | itemlen | dead | keys
-------------+-----------+---------+------+-------------------
- 1 | (1,65535) | 40 | f | (p)=((185,185))
- 2 | (2,65535) | 40 | f | (p)=((370,370))
- 3 | (3,65535) | 40 | f | (p)=((555,555))
- 4 | (4,65535) | 40 | f | (p)=((740,740))
- 5 | (5,65535) | 40 | f | (p)=((870,870))
- 6 | (6,65535) | 40 | f | (p)=((1000,1000))
+ itemoffset | ctid | itemlen | dead | keys
+------------+-----------+---------+------+-------------------------------
+ 1 | (1,65535) | 40 | f | (p)=(((185,185),(1,1)))
+ 2 | (2,65535) | 40 | f | (p)=(((370,370),(186,186)))
+ 3 | (3,65535) | 40 | f | (p)=(((555,555),(371,371)))
+ 4 | (4,65535) | 40 | f | (p)=(((740,740),(556,556)))
+ 5 | (5,65535) | 40 | f | (p)=(((870,870),(741,741)))
+ 6 | (6,65535) | 40 | f | (p)=(((1000,1000),(871,871)))
(6 rows)
SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 1), 'test_gist_idx') LIMIT 5;
- itemoffset | ctid | itemlen | dead | keys
-------------+-------+---------+------+-------------
- 1 | (0,1) | 40 | f | (p)=((1,1))
- 2 | (0,2) | 40 | f | (p)=((2,2))
- 3 | (0,3) | 40 | f | (p)=((3,3))
- 4 | (0,4) | 40 | f | (p)=((4,4))
- 5 | (0,5) | 40 | f | (p)=((5,5))
+ itemoffset | ctid | itemlen | dead | keys
+------------+-------+---------+------+---------------------
+ 1 | (0,1) | 40 | f | (p)=(((1,1),(1,1)))
+ 2 | (0,2) | 40 | f | (p)=(((2,2),(2,2)))
+ 3 | (0,3) | 40 | f | (p)=(((3,3),(3,3)))
+ 4 | (0,4) | 40 | f | (p)=(((4,4),(4,4)))
+ 5 | (0,5) | 40 | f | (p)=(((5,5),(5,5)))
(5 rows)
-- gist_page_items_bytea prints the raw key data as a bytea. The output of that is
@@ -107,4 +107,63 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
(1 row)
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i1 int DEFAULT NULL;
+ALTER TABLE test_gist ADD COLUMN i2 int DEFAULT 1;
+CREATE INDEX test_gist_idx_inc_1 ON test_gist
+ USING gist (p, point(i1, i1)) INCLUDE (p, i1);
+CREATE INDEX test_gist_idx_inc_12 ON test_gist
+ USING gist (p, point(i1, i2)) INCLUDE (t, i1, i2);
+CREATE INDEX test_gist_idx_inc_2 ON test_gist
+ USING gist (p, point(i2, i2)) INCLUDE (p, t, i2);
+SELECT keys AS keys_nonleaf_1
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_1', 0), 'test_gist_idx_inc_1')
+ WHERE itemoffset = 1;
+ keys_nonleaf_1
+----------------------------------------------------------------------------------
+ (p, point(i1::double precision, i1::double precision))=(((119,119),(1,1)), null)
+(1 row)
+
+SELECT keys AS keys_leaf_1
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_1', 1), 'test_gist_idx_inc_1')
+ WHERE itemoffset = 1;
+ keys_leaf_1
+--------------------------------------------------------------------------------------------------------------------
+ (p, point(i1::double precision, i1::double precision)) INCLUDE (p, i1)=(((1,1),(1,1)), null) INCLUDE ((1,1), null)
+(1 row)
+
+SELECT keys AS keys_nonleaf_12
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_12', 0), 'test_gist_idx_inc_12')
+ WHERE itemoffset = 1;
+ keys_nonleaf_12
+----------------------------------------------------------------------------------
+ (p, point(i1::double precision, i2::double precision))=(((135,135),(1,1)), null)
+(1 row)
+
+SELECT keys AS keys_leaf_12
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_12', 1), 'test_gist_idx_inc_12')
+ WHERE itemoffset = 1;
+ keys_leaf_12
+-----------------------------------------------------------------------------------------------------------------------
+ (p, point(i1::double precision, i2::double precision)) INCLUDE (t, i1, i2)=(((1,1),(1,1)), null) INCLUDE (1, null, 1)
+(1 row)
+
+SELECT keys AS keys_nonleaf_2
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_2', 0), 'test_gist_idx_inc_2')
+ WHERE itemoffset = 1;
+ keys_nonleaf_2
+-----------------------------------------------------------------------------------------
+ (p, point(i2::double precision, i2::double precision))=(((81,81),(1,1)), ((1,1),(1,1)))
+(1 row)
+
+SELECT keys AS keys_leaf_2
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_2', 1), 'test_gist_idx_inc_2')
+ WHERE itemoffset = 1;
+ keys_leaf_2
+--------------------------------------------------------------------------------------------------------------------------------
+ (p, point(i2::double precision, i2::double precision)) INCLUDE (p, t, i2)=(((1,1),(1,1)), ((1,1),(1,1))) INCLUDE ((1,1), 1, 1)
+(1 row)
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 3dca7f1318..c11d2915dc 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -21,8 +21,10 @@
#include "storage/itemptr.h"
#include "utils/array.h"
#include "utils/builtins.h"
-#include "utils/rel.h"
#include "utils/pg_lsn.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/varlena.h"
PG_FUNCTION_INFO_V1(gist_page_opaque_info);
@@ -198,9 +200,13 @@ gist_page_items(PG_FUNCTION_ARGS)
Oid indexRelid = PG_GETARG_OID(1);
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Relation indexRel;
+ TupleDesc tupdesc;
Page page;
+ uint16 flagbits;
+ bits16 printflags = 0;
OffsetNumber offset;
OffsetNumber maxoff = InvalidOffsetNumber;
+ char *index_columns;
if (!superuser())
ereport(ERROR,
@@ -226,6 +232,27 @@ gist_page_items(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ flagbits = GistPageGetOpaque(page)->flags;
+
+ /*
+ * Included attributes are added when dealing with leaf pages, discarded
+ * for non-leaf pages as these include only data for key attributes.
+ */
+ printflags |= RULE_INDEXDEF_PRETTY;
+ if (flagbits & F_LEAF)
+ {
+ tupdesc = RelationGetDescr(indexRel);
+ }
+ else
+ {
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
+ printflags |= RULE_INDEXDEF_KEYS_ONLY;
+ }
+
+ index_columns = pg_get_indexdef_columns_extended(indexRelid,
+ printflags);
+
/* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */
if (GistPageIsDeleted(page))
elog(NOTICE, "page is deleted");
@@ -242,7 +269,8 @@ gist_page_items(PG_FUNCTION_ARGS)
IndexTuple itup;
Datum itup_values[INDEX_MAX_KEYS];
bool itup_isnull[INDEX_MAX_KEYS];
- char *key_desc;
+ StringInfoData buf;
+ int i;
id = PageGetItemId(page, offset);
@@ -251,7 +279,7 @@ gist_page_items(PG_FUNCTION_ARGS)
itup = (IndexTuple) PageGetItem(page, id);
- index_deform_tuple(itup, RelationGetDescr(indexRel),
+ index_deform_tuple(itup, tupdesc,
itup_values, itup_isnull);
memset(nulls, 0, sizeof(nulls));
@@ -261,9 +289,50 @@ gist_page_items(PG_FUNCTION_ARGS)
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
- key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull);
- if (key_desc)
- values[4] = CStringGetTextDatum(key_desc);
+ if (index_columns)
+ {
+ initStringInfo(&buf);
+ appendStringInfo(&buf, "(%s)=(", index_columns);
+
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ char *val;
+
+ if (itup_isnull[i])
+ val = "null";
+ else
+ {
+ Oid foutoid;
+ bool typisvarlena;
+ Oid typoid;
+
+ typoid = tupdesc->attrs[i].atttypid;
+ getTypeOutputInfo(typoid, &foutoid, &typisvarlena);
+ val = OidOutputFunctionCall(foutoid, itup_values[i]);
+ }
+
+ if (i == IndexRelationGetNumberOfKeyAttributes(indexRel))
+ appendStringInfoString(&buf, ") INCLUDE (");
+ else if (i > 0)
+ appendStringInfoString(&buf, ", ");
+
+ /*
+ * NULL values are printed as-is, and included attributes
+ * don't require more parenthesis for clarity in the output.
+ */
+ if (itup_isnull[i])
+ appendStringInfoString(&buf, val);
+ else if (i >= IndexRelationGetNumberOfKeyAttributes(indexRel))
+ appendStringInfo(&buf, "%s", val);
+ else
+ appendStringInfo(&buf, "(%s)", val);
+ }
+
+ appendStringInfoChar(&buf, ')');
+
+ values[4] = CStringGetTextDatum(buf.data);
+ nulls[4] = false;
+ }
else
{
values[4] = (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 4787b784a4..5d9384907e 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -52,4 +52,34 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex'));
SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass);
SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
+-- Test gist_page_items with included columns.
+-- Non-leaf pages have only the key attributes, and leaf pages have
+-- the included attributes.
+ALTER TABLE test_gist ADD COLUMN i1 int DEFAULT NULL;
+ALTER TABLE test_gist ADD COLUMN i2 int DEFAULT 1;
+CREATE INDEX test_gist_idx_inc_1 ON test_gist
+ USING gist (p, point(i1, i1)) INCLUDE (p, i1);
+CREATE INDEX test_gist_idx_inc_12 ON test_gist
+ USING gist (p, point(i1, i2)) INCLUDE (t, i1, i2);
+CREATE INDEX test_gist_idx_inc_2 ON test_gist
+ USING gist (p, point(i2, i2)) INCLUDE (p, t, i2);
+SELECT keys AS keys_nonleaf_1
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_1', 0), 'test_gist_idx_inc_1')
+ WHERE itemoffset = 1;
+SELECT keys AS keys_leaf_1
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_1', 1), 'test_gist_idx_inc_1')
+ WHERE itemoffset = 1;
+SELECT keys AS keys_nonleaf_12
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_12', 0), 'test_gist_idx_inc_12')
+ WHERE itemoffset = 1;
+SELECT keys AS keys_leaf_12
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_12', 1), 'test_gist_idx_inc_12')
+ WHERE itemoffset = 1;
+SELECT keys AS keys_nonleaf_2
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_2', 0), 'test_gist_idx_inc_2')
+ WHERE itemoffset = 1;
+SELECT keys AS keys_leaf_2
+ FROM gist_page_items(get_raw_page('test_gist_idx_inc_2', 1), 'test_gist_idx_inc_2')
+ WHERE itemoffset = 1;
+
DROP TABLE test_gist;
--
2.40.1
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
@ 2023-05-18 04:01 ` Michael Paquier <michael@paquier.xyz>
2023-05-18 06:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2023-05-18 04:01 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-18 04:01 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
@ 2023-05-18 06:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-18 07:16 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Lakhin @ 2023-05-18 06:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-18 04:01 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-18 06:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
@ 2023-05-18 07:16 ` Michael Paquier <michael@paquier.xyz>
2023-05-19 05:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2023-05-18 07:16 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-18 04:01 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
2023-05-18 06:00 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Alexander Lakhin <exclusion@gmail.com>
2023-05-18 07:16 ` Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns Michael Paquier <michael@paquier.xyz>
@ 2023-05-19 05:41 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 11+ messages in thread
From: Michael Paquier @ 2023-05-19 05:41 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-05-19 05:41 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-04-04 13:00 BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns PG Bug reporting form <noreply@postgresql.org>
2023-05-12 10:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-15 00:41 ` Michael Paquier <michael@paquier.xyz>
2023-05-15 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-17 05:23 ` Michael Paquier <michael@paquier.xyz>
2023-05-17 14:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-18 01:50 ` Michael Paquier <michael@paquier.xyz>
2023-05-18 04:01 ` Michael Paquier <michael@paquier.xyz>
2023-05-18 06:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-05-18 07:16 ` Michael Paquier <michael@paquier.xyz>
2023-05-19 05:41 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox