Horton

Re: Assert in pageinspect with NULL pages

Julien Rouhaud <rjuju123@gmail.com>

View thread View on postgr.es
From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>, Daria Lepikhova <d.lepikhova@postgrespro.ru>, pgsql-hackers@postgresql.org
Date: 2022-03-25T03:03:47Z
Lists: pgsql-hackers 
On Fri, Mar 25, 2022 at 11:44:26AM +0900, Michael Paquier wrote:
> I have reviewed what you have sent, bumping on a couple of issues:

Thanks!

I'm happy with all the changes, except:

+		if (P_ISLEAF(opaque) && opaque->btpo_level != 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("block is not a valid leaf page")));

All other messages specify which kind of page it's about, so I think it would
be better to specify "btree" leaf page here, especially since some other AMs
also have leaf pages.

> - The tests of btree and BRIN failed with 32-bit builds, because
> MAXALIGN returns shorter special area sizes in those cases.  This can
> be fixed by abusing of \set VERBOSITY to mask the error details.  We
> already do that in some of the tests to make them portable.

Yeah, that's the other stability problem I was worried about.  I should have
tried to compile with -m32.

> > I'm a bit worried about the btree tests stability.  I avoid emitting the level
> > found to help with that, but it still depends on what other AM will put in
> > their special page.
> 
> Well, the limit of the pageinspect model comes from the fact that it
> is possible to pass down any bytea and all those code paths would
> happily process the blobs as long as they are 8kB.  Pages can be
> crafted as well to bypass some of the checks.  This is superuser-only,
> so people have to be careful, but preventing out-of-bound reads is a
> different class of problem, as long as these come from valid pages.

Agreed.  Also pageinspect can be handy when debugging corruption, so I think it
shouldn't try too hard to discard buggy pages.

Commits

  1. pageinspect: Fix handling of all-zero pages

  2. pageinspect: Add more sanity checks to prevent out-of-bound reads

  3. pageinspect: Fix handling of page sizes and AM types

  4. Teach pageinspect about nbtree deduplication.