PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?

On Thu, Apr 22, 2021 at 10:40 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

In the PageGetItemIdCareful() introduced by commit a9ce839a, it seems
like we are using btree page pd_special structure BTPageOpaqueData for
error case without max aligning it.
if (ItemIdGetOffset(itemid) + ItemIdGetLength(itemid) >
BLCKSZ - sizeof(BTPageOpaqueData))
ereport(ERROR,

I'm not sure if it is intentional. ISTM that this was actually not a
problem because the BTPageOpaqueData already has all-aligned(???)
members (3 uint32, 2 uint16). But it might be a problem if we add
unaligned bytes. PageInit always max aligns this structure, when we
initialize the btree page in _bt_pageini and in all other places we
max align it before use. Since this is an error throwing path, I think
we should max align it just to be on the safer side. While on it, I
think we can also replace BLCKSZ with PageGetPageSize(page).

Attaching a small patch. Thoughts?

+1 for changing to MAXALIGN(sizeof(BTPageOpaqueData)).

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 22, 2021 at 11:36 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Apr 22, 2021 at 10:40 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

In the PageGetItemIdCareful() introduced by commit a9ce839a, it seems
like we are using btree page pd_special structure BTPageOpaqueData for
error case without max aligning it.
if (ItemIdGetOffset(itemid) + ItemIdGetLength(itemid) >
BLCKSZ - sizeof(BTPageOpaqueData))
ereport(ERROR,

I'm not sure if it is intentional. ISTM that this was actually not a
problem because the BTPageOpaqueData already has all-aligned(???)
members (3 uint32, 2 uint16). But it might be a problem if we add
unaligned bytes. PageInit always max aligns this structure, when we
initialize the btree page in _bt_pageini and in all other places we
max align it before use. Since this is an error throwing path, I think
we should max align it just to be on the safer side. While on it, I
think we can also replace BLCKSZ with PageGetPageSize(page).

Attaching a small patch. Thoughts?

+1 for changing to MAXALIGN(sizeof(BTPageOpaqueData)).

Thanks for taking a look at it. I added a CF entry
https://commitfest.postgresql.org/33/3089/ so that we don't lose track
of it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 21, 2021 at 10:10 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

In the PageGetItemIdCareful() introduced by commit a9ce839a, it seems
like we are using btree page pd_special structure BTPageOpaqueData for
error case without max aligning it.
if (ItemIdGetOffset(itemid) + ItemIdGetLength(itemid) >
BLCKSZ - sizeof(BTPageOpaqueData))
ereport(ERROR,

I'm not sure if it is intentional. ISTM that this was actually not a
problem because the BTPageOpaqueData already has all-aligned(???)
members (3 uint32, 2 uint16). But it might be a problem if we add
unaligned bytes.

Fair point. I pushed a commit to fix this to HEAD just now. Thanks.

PageInit always max aligns this structure, when we
initialize the btree page in _bt_pageini and in all other places we
max align it before use. Since this is an error throwing path, I think
we should max align it just to be on the safer side. While on it, I
think we can also replace BLCKSZ with PageGetPageSize(page).

I didn't replace BLCKSZ with PageGetPageSize() in the commit, though.
We definitely don't want to rely on that being sane in amcheck (this
is also why we don't use PageGetSpecialPointer(), which is the usual
approach).

Actually, even if this wasn't amcheck code I might make the same call.
I personally don't think that most existing calls to PageGetPageSize()
make very much sense.

Attaching a small patch. Thoughts?

I'm curious: Was this just something that you noticed randomly, while
looking at the code? Or do you have a specific practical reason to
care about it? (I always like hearing about the ways in which people
use amcheck.)

--
Peter Geoghegan

On Sat, Apr 24, 2021 at 4:11 AM Peter Geoghegan <pg@bowt.ie> wrote:

PageInit always max aligns this structure, when we
initialize the btree page in _bt_pageini and in all other places we
max align it before use. Since this is an error throwing path, I think
we should max align it just to be on the safer side. While on it, I
think we can also replace BLCKSZ with PageGetPageSize(page).

I didn't replace BLCKSZ with PageGetPageSize() in the commit, though.
We definitely don't want to rely on that being sane in amcheck (this
is also why we don't use PageGetSpecialPointer(), which is the usual
approach).

If the PageGetPageSize can't be sane within amcheck, does it mean that
the page would have been corrupted somewhere?

Actually, even if this wasn't amcheck code I might make the same call.
I personally don't think that most existing calls to PageGetPageSize()
make very much sense.

Should we get rid of all existing PageGetPageSize and directly use
BLCKSZ instead? AFAICS, all the index and heap pages are of BLCKSZ
(PageInit has Assert(pageSize == BLCKSZ);).

Using PageGetPageSize to get the size that's been stored in the page,
we might catch errors early if at all the page is corrupted and the
size is overwritten . That's not the case if we use BLCKSZ which is
not stored in the page. In this case the size stored on the page
becomes redundant and the pd_pagesize_version could just be 2 bytes
storing the page version. While we save 2 bytes per page, I'm not sure
this is acceptable as PageHeader size gets changed.

I'm curious: Was this just something that you noticed randomly, while
looking at the code? Or do you have a specific practical reason to
care about it? (I always like hearing about the ways in which people
use amcheck.)

I found this while working on one internal feature but not while using amcheck.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com