Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 19 Dec 2023 18:20:44 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18246: pgstathashindex() attempts to read invalid file for
 hash index attached to partitioned table
Message-ID: <ZYFgbOGYpsaen5LF@paquier.xyz>
References: <18246-f4d9ff7cb3af77e6@postgresql.org>
 <ZXm9Js8D_W_cFWzW@paquier.xyz>
 <876b6e52-f068-648a-3f0e-7eaabd57cf77@gmail.com>
 <11c0916f-82de-decf-c27a-c01a4f8dfcef@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="rj9GUDEdlM/X5J3Z"
Content-Disposition: inline
In-Reply-To: <11c0916f-82de-decf-c27a-c01a4f8dfcef@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZYFgbOGYpsaen5LF%40paquier.xyz>
Precedence: bulk


--rj9GUDEdlM/X5J3Z
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 15, 2023 at 04:00:01PM +0300, Alexander Lakhin wrote:
> 13.12.2023 21:00, Alexander Lakhin wrote:
>> 13.12.2023 17:18, Michael Paquier wrote:
>> > On Wed, Dec 13, 2023 at 09:00:01AM +0000, PG Bug reporting form wrote:
>> > > This anomaly can be observed since 8b08f7d48 from 2018-01-19, but IM=
O the
>> > > culprit is e759854a0 from 2017-02-03, which introduced the following
>> > > asymmetry in pgstatindex.c:
>> > > if (!IS_INDEX(rel) || !IS_BTREE(rel))
>> > > if (!IS_INDEX(rel) || !IS_GIN(rel))
>> > > But:
>> > > if (!IS_HASH(rel))
>> > Fun, let's fix that.=A0 Would you like to write a patch?
>>=20
>> Yes, please look at the attached,

Thanks.  The one for pgstattuple has been applied down to 12 as of
a8dd62ef4959.

> Looking around, I found the similar index_open() usage within pageinspect,
> function hash_bitmap_info():
> CREATE EXTENSION pageinspect;
> CREATE TABLE t (a int, b int[], c int) PARTITION BY RANGE (a);
> CREATE INDEX c_idx ON t USING hash(c);
> SELECT hash_bitmap_info('c_idx'::regclass, 1);
>=20
> Leads to:
> TRAP: failed Assert("false"), File: "bufmgr.c", Line: 3606, PID: 3984550

Right.

> I'm inclined to leave index_open() there for the internal consistency
> inside pageinspect, and to use the same condition as in
> bt_index_block_validate(), which called by functions bt_page_stats(...),
> bt_multi_page_stats(...), bt_page_items(relname text, blkno bigint).
>=20
> Functions for other index types, brin/gin/gist, don't read the index,
> instead they receive a page as input.

Not sure to agree on that.  It seems a bit strange to bump on a
different error when using a partitioned index or a relkind not
allowed by index_open().  And this is after looking at the pgstattuple
code.

At the end, I've switched to relation_open() for this one, added a
test and applied the second patch as of 208470136421.  I've checked
the rest and yes, it looks like there are no other holes like the
ones you have reported for the contrib modules when it comes to index
manipulations.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWBYGwACgkQnvQgOdby
QH3C6g//QnHDtGDWe1Q8Ohv4+ScaEj85y3ZtRiu89JaktRseNqx7IyfxEaioPJIp
UeY01P/oF8FZMyr+krLY8+lmjQOXrDW7k7vsega19PxEDxbVBVoJmxHwkOvI8her
PLzVHBregPnamIspADtWDkxDXnhwr0ZmGlT6vu4aXwpW34q6idM+3Un0DLlrKfTw
nVwZa1OquuU3oW30fwEDIW96YdNVCjSNVqO5qgyMDGVwCZIAdDS9LAOO2a4+BV6M
ZO/NPdexzlVY1MHc1tR3c7x6TAvbIZZyrsU1S58yG1uq4aWguS9ajyeFDfOwMCNT
m5FV9IvohQM9DqNAssXBEigbJTPkbFM/Y7MOYd5NRpC2fX+4UkSHtYtBDJUSswJ0
WF+O7JJsea9q/CDsU1cSPm4ez8odMcsufc8dyb3lOEpXng68ohdSFcJeAKo9XRZ7
gWk0gVGuze3/F+fE6wIhj6UJ9nujLtxbclqH8UAEriojMOK5lx/hgwdxCAJszlel
yTkNrqMeulKPdtZe+QN+9XUFXPT/4R/4o3L2qVQQbBrZt4xb/o0/7CB68z9NHsyD
tatZWEZHI7MwysL+pwMlXKTKViqd8o6rExGsvCiEFJVIwliSZMn5SQ3+4bGXXsIX
rwlNGugWtng5V+ai4s7YwkA6yt2PVKuvvwjyKa91GmfPVC6svk8=
=DkUl
-----END PGP SIGNATURE-----

--rj9GUDEdlM/X5J3Z--