From: Michael Paquier <michael@paquier.xyz>
To: Alexander Lakhin <exclusion@gmail.com>
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 14:23:40 +0900
Message-ID: <ZGRk3LVBpt9/dFlt@paquier.xyz> (raw)
In-Reply-To: <8a991814-97c0-81d6-1e24-60c26308f005@gmail.com>
References: <17884-cb8c326522977acb@postgresql.org>
<92873f8d-a9b1-0ee8-5753-ee4a1664fb0e@gmail.com>
<ZGF/qgICp6JvNDhV@paquier.xyz>
<8a991814-97c0-81d6-1e24-60c26308f005@gmail.com>
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.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);
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/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.outindex 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.cindex 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.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;
--
2.40.1
signature.asc (application/pgp-signature, 832B) download
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: michael@paquier.xyz, exclusion@gmail.com, 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: <ZGRk3LVBpt9/dFlt@paquier.xyz>
* 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