From: Alexander Lakhin <exclusion@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@lists.postgresql.org, Andrey Borodin <amborodin86@gmail.com>
Subject: Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
Date: Wed, 17 May 2023 17:00:00 +0300
Message-ID: <e4c9897a-6864-4c47-f7ea-08385f2a7db5@gmail.com> (raw)
In-Reply-To: <ZGRk3LVBpt9/dFlt@paquier.xyz>
References: <17884-cb8c326522977acb@postgresql.org>
<92873f8d-a9b1-0ee8-5753-ee4a1664fb0e@gmail.com>
<ZGF/qgICp6JvNDhV@paquier.xyz>
<8a991814-97c0-81d6-1e24-60c26308f005@gmail.com>
<ZGRk3LVBpt9/dFlt@paquier.xyz>
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) downloaddiff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.outindex 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.cindex 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.sqlindex 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.cindex 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.hindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, michael@paquier.xyz, amborodin86@gmail.com
Subject: Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
In-Reply-To: <e4c9897a-6864-4c47-f7ea-08385f2a7db5@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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