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.
