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: Mon, 15 May 2023 11:00:00 +0300
Message-ID: <8a991814-97c0-81d6-1e24-60c26308f005@gmail.com> (raw)
In-Reply-To: <ZGF/qgICp6JvNDhV@paquier.xyz>
References: <17884-cb8c326522977acb@postgresql.org>
<92873f8d-a9b1-0ee8-5753-ee4a1664fb0e@gmail.com>
<ZGF/qgICp6JvNDhV@paquier.xyz>
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) downloaddiff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.outindex 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.cindex 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.sqlindex 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.cindex 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.hindex 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);
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: <8a991814-97c0-81d6-1e24-60c26308f005@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