Date: Wed, 30 Mar 2022 11:38:43 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Maxim Orlov <orlovmg@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Peter Geoghegan <pg@bowt.ie>,
	Robert Haas <robertmhaas@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Daria Lepikhova <d.lepikhova@postgrespro.ru>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Assert in pageinspect with NULL pages
Message-ID: <20220330033843.v325dpsl2dfjspl2@jrouhaud>
References: <186c0dc2-cf36-c45c-7933-676956821d11@gmail.com>
 <YjrXaecdozs6EzYe@paquier.xyz>
 <CAH2-WzkOHbSpW=Gs7eZ1CBKNATjMAczktTt=yGqhw9nbPd27sg@mail.gmail.com>
 <Yj1LolhYkHQCYiol@paquier.xyz>
 <CA+TgmoYy-qiM2Zf3MA9T_8sjtSTxiNm1fgBDvyrv96bf44VArA@mail.gmail.com>
 <CAH2-Wzk4czia7nPJaYQPfpecnQH6L6gRrO4ZC7dj3sOFa1D-JQ@mail.gmail.com>
 <CA+Tgmoa4o5U7wDzoQ90w5U7pWdXq+Lr9Bnhys4K5D30xw_0BwQ@mail.gmail.com>
 <CAH2-Wz=JCKZrXHyfP3pp+Kd8MrngK7ktTmdKaLXzXBEOT24T1w@mail.gmail.com>
 <YkEuC4vjThLm0mR6@paquier.xyz>
 <CACG=ezZLZwOW0ESJAdBmT9t9P4qM=FOsxv1YYK-oWGL53e62ew@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CACG=ezZLZwOW0ESJAdBmT9t9P4qM=FOsxv1YYK-oWGL53e62ew@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220330033843.v325dpsl2dfjspl2%40jrouhaud>
Precedence: bulk

Hi,

On Mon, Mar 28, 2022 at 08:29:29PM +0300, Maxim Orlov wrote:
> I've suddenly found that the test in this patch is based on a fact that
> heap pages don't have PageSpecial or it is of different size with btree
> pages Special area:
>
> CREATE TABLE test1 (a int8, b int4range);
>
> SELECT bt_page_items(get_raw_page('test1', 0));
>
>
> In the current state is is so, but it is not guaranteed. For example, in
> the proposed 64xid patch [1] we introduce PageSpecial into heap pages and
> its size on occasion equals to the size of btree page special so check from
> above is false. Even if we pass heap page into pageinspect pretending it is
> btree page. So the test will fail.
>
>
> Generally it seems not only a wrong test but the consequence of a bigger
> scale fact that we can not always distinguish different AM's pages. So I'd
> propose at least that we don't come to a conclusion that a page is valid
> based on PageSpecial size is right.

We don't assume that the page is valid, or of the correct AM, just based on the
special area size.  We do reject it if the size is invalid, but we also have
other tests based on what's actually possible to test (checking flag sanity,
magic values and so on).

Note that Peter G. suggested to add more checks for btree based on
palloc_btree_page(), did you check if those were enough to fix this test with
the 64bits xid patchset applied?