Обсуждение: PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?
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?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Вложения
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