Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 17 May 2023 14:23:40 +0900
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
Message-ID: <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="hsE3MiS6P9cNkHf6"
Content-Disposition: inline
In-Reply-To: <8a991814-97c0-81d6-1e24-60c26308f005@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZGRk3LVBpt9/dFlt%40paquier.xyz>
Precedence: bulk


--hsE3MiS6P9cNkHf6
Content-Type: multipart/mixed; boundary="76yTH/M++M6YJ4fa"
Content-Disposition: inline


--76yTH/M++M6YJ4fa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 =3D CreateTupleDescCopy(RelationGetDescr(indexRel));
+       tupdesc->natts =3D 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

--76yTH/M++M6YJ4fa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-01-fix-gist_page_items-for-index-with-INCLUDE.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 0d39cd7dfc1afab291aa4a5ee979a8f9cb624905 Mon Sep 17 00:00:00 2001
=46rom: 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;
=20
+/* Flags for pg_get_indexdef_columns_extended() */
+#define RULE_INDEXDEF_PRETTY		0x01
+#define RULE_INDEXDEF_KEYS_ONLY		0x02	/* ignore included attributes */
=20
 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);
=20
 extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rule=
utils.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);
 }
=20
+/* Internal version, extensible with flags to control its behavior */
+char *
+pg_get_indexdef_columns_extended(Oid indexrelid, bits16 flags)
+{
+	bool		pretty =3D ((flags & RULE_INDEXDEF_PRETTY) !=3D 0);
+	bool		keys_only =3D ((flags & RULE_INDEXDEF_KEYS_ONLY) !=3D 0);
+	int			prettyFlags;
+
+	prettyFlags =3D 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/ex=
pected/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', :bloc=
k_size), 'hex'));
 =20
 (1 row)
=20
+-- 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 =3D 1;
+ itemoffset |   ctid    | itemlen | dead |      keys      =20
+------------+-----------+---------+------+-----------------
+          1 | (1,65535) |      40 | f    | (p)=3D((135,135))
+(1 row)
+
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_=
gist_idx_inc')
+   WHERE itemoffset =3D 1;
+ itemoffset | ctid  | itemlen | dead |                     keys           =
         =20
+------------+-------+---------+------+------------------------------------=
----------
+          1 | (0,1) |      56 | f    | (p) INCLUDE (t, i)=3D((1,1)) INCLUD=
E (1, null)
+(1 row)
+
 DROP TABLE test_gist;
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfunc=
s.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"
=20
 PG_FUNCTION_INFO_V1(gist_page_opaque_info);
@@ -198,9 +200,13 @@ gist_page_items(PG_FUNCTION_ARGS)
 	Oid			indexRelid =3D PG_GETARG_OID(1);
 	ReturnSetInfo *rsinfo =3D (ReturnSetInfo *) fcinfo->resultinfo;
 	Relation	indexRel;
+	TupleDesc	tupdesc;
 	Page		page;
+	uint16		flagbits;
+	bits16		printflags =3D 0;
 	OffsetNumber offset;
 	OffsetNumber maxoff =3D InvalidOffsetNumber;
+	char	   *index_columns;
=20
 	if (!superuser())
 		ereport(ERROR,
@@ -226,6 +232,27 @@ gist_page_items(PG_FUNCTION_ARGS)
 		PG_RETURN_NULL();
 	}
=20
+	flagbits =3D 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 |=3D RULE_INDEXDEF_PRETTY;
+	if (flagbits & F_LEAF)
+	{
+		tupdesc =3D RelationGetDescr(indexRel);
+	}
+	else
+	{
+		tupdesc =3D CreateTupleDescCopy(RelationGetDescr(indexRel));
+		tupdesc->natts =3D IndexRelationGetNumberOfKeyAttributes(indexRel);
+		printflags |=3D RULE_INDEXDEF_KEYS_ONLY;
+	}
+
+	index_columns =3D 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;
=20
 		id =3D PageGetItemId(page, offset);
=20
@@ -251,7 +279,7 @@ gist_page_items(PG_FUNCTION_ARGS)
=20
 		itup =3D (IndexTuple) PageGetItem(page, id);
=20
-		index_deform_tuple(itup, RelationGetDescr(indexRel),
+		index_deform_tuple(itup, tupdesc,
 						   itup_values, itup_isnull);
=20
 		memset(nulls, 0, sizeof(nulls));
@@ -261,9 +289,55 @@ gist_page_items(PG_FUNCTION_ARGS)
 		values[2] =3D Int32GetDatum((int) IndexTupleSize(itup));
 		values[3] =3D BoolGetDatum(ItemIdIsDead(id));
=20
-		key_desc =3D BuildIndexValueDescription(indexRel, itup_values, itup_isnu=
ll);
-		if (key_desc)
-			values[4] =3D CStringGetTextDatum(key_desc);
+		if (index_columns)
+		{
+			initStringInfo(&buf);
+			appendStringInfo(&buf, "(%s)=3D(", index_columns);
+
+			/*
+			 * This is similar to BuildIndexValueDescription(), though it
+			 * covers included attributes.
+			 */
+			for (i =3D 0; i < tupdesc->natts; i++)
+			{
+				char	   *val;
+
+				if (itup_isnull[i])
+					val =3D "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 =3D indexRel->rd_opcintype[i];
+					else
+						attroid =3D tupdesc->attrs[i].atttypid;
+
+					getTypeOutputInfo(attroid, &foutoid, &typisvarlena);
+					val =3D OidOutputFunctionCall(foutoid, itup_values[i]);
+				}
+
+				if (i =3D=3D IndexRelationGetNumberOfKeyAttributes(indexRel))
+					appendStringInfoString(&buf, ") INCLUDE (");
+				else if (i > 0)
+					appendStringInfoString(&buf, ", ");
+
+				appendStringInfoString(&buf, val);
+			}
+
+			appendStringInfoChar(&buf, ')');
+
+			values[4] =3D CStringGetTextDatum(buf.data);
+			nulls[4] =3D false;
+		}
 		else
 		{
 			values[4] =3D (Datum) 0;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gis=
t.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_gis=
t_idx'::regclass);
 SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex'));
=20
+-- 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 =3D 1;
+SELECT * FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_=
gist_idx_inc')
+   WHERE itemoffset =3D 1;
+
 DROP TABLE test_gist;
--=20
2.40.1


--76yTH/M++M6YJ4fa--

--hsE3MiS6P9cNkHf6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmRkZNwACgkQnvQgOdby
QH1+vA//Ynoaf+AGc/Kzz49Dbh9LXdgX5XchxxnyH9jDuQKGR9vBS8O5+aP6sZEJ
Af4ozo7Dwzjcs7WXiBIXfLClqqu48dXX8UzQ3H8ZJ9One0SiX7ZSAIROYh4obAxG
lpGB+tJkLoeuoFcnXCPZ3gKkzgo8flsWI2NiiiDaD6Bv4xQyR4TXictTptniyDt/
hqU3iqaVy5LFQIuPKxNWxC1YybPV1yowLgPDe7nztx9sAAmI1Vp2jnzk4bgO8d/t
+wm4hhJ78DoXrhL35Ii4VCffav/Nq57vySHZJBfNn5H3oggpiBWCw0dQ64BpjbGd
wcpZdS/v1HA5bxvCKlsga+0Y9z2RpxK9G0xLU4tdKR9vyDjduNe7ChIy7McKdBgM
JuWngQlcoT/2eeiY3YqfTQHO18Ypb8mZ+vpOmYmsrdZVPfXbQBo4hjZUeQycIDbY
n1UPq85yFalsKqVuKaFFmTcBtcWsUwp4+Zh6Y3sgsms81MTnmbAWk0SIJi2p3Dgw
YzlflXJg9GYA33YssIJscqAK1bjhL6FputOYoPwHmwB570en5/ZxBjRzD9WjInxe
C7AexzjoyGc0CVBH0T4oNwPXHMBVwrvMEU8SnXzoztQtyRQgswOto3fF8JsrMIyb
KfqwCYGiKeideehXkvi6A8Aw3qlQokdDUz9b+5HStsvUnAcW+zs=
=pSUB
-----END PGP SIGNATURE-----

--hsE3MiS6P9cNkHf6--