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.