Thread: [PATCH] Improve treatment of page special and page header alignment during page init.
[PATCH] Improve treatment of page special and page header alignment during page init.
From
Pavel Borisov
Date:
I was looking at changes in Sp-Gist by commit 4c0239cb7a7775e3183cb575e62703d71bf3302d
(discussionhttps://postgr.es/m/CALj2ACViOo2qyaPT7krWm4LRyRTw9kOXt+g6PfNmYuGA=YHj9A@mail.gmail.com ) and realized that during PageInit, both page header and page special are expected to be maxaligned but in reality, their treatment is quite different:
1. page special size is silently enforced to be maxaligned by PageInit() even if caller-specified specialSize is not of a maxalign'ed size.
2. page header size alignment is not checked at all (but we expect it maxalign'ed, yes).
I'd propose do both things in the same way: just Assert both sizes are maxalign'ed during page init.
I dived further and it appears that the only caller, who provides not properly aligned page special is fill_seq_with_data() and corrected it.
I am really convinced, that _callers_ should care about proper special size. So now PageInit() just checks the right lengths of page special and page header with assert, not enforcing size change silently. PFA my small patch on this. I'd propose it to commit if in the HEAD only likewise the commit 4c0239cb7a7775e3183cb575e62703d71bf3302d.
What do you think?
Attachment
Re: [PATCH] Improve treatment of page special and page header alignment during page init.
From
Bharath Rupireddy
Date:
On Wed, Apr 7, 2021 at 5:32 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > I was looking at changes in Sp-Gist by commit 4c0239cb7a7775e3183cb575e62703d71bf3302d > (discussion > https://postgr.es/m/CALj2ACViOo2qyaPT7krWm4LRyRTw9kOXt+g6PfNmYuGA=YHj9A@mail.gmail.com ) and realized that during PageInit,both page header and page special are expected to be maxaligned but in reality, their treatment is quite different: How can we say that in PageInit the SizeOfPageHeaderData is expected to be max aligned? Am I missing something? There are lots of other places where SizeOfPageHeaderData is used, not MAXALIGN(SizeOfPageHeaderData). > 1. page special size is silently enforced to be maxaligned by PageInit() even if caller-specified specialSize is not ofa maxalign'ed size. > 2. page header size alignment is not checked at all (but we expect it maxalign'ed, yes). > > I'd propose do both things in the same way: just Assert both sizes are maxalign'ed during page init. > > I dived further and it appears that the only caller, who provides not properly aligned page special is fill_seq_with_data()and corrected it. > > I am really convinced, that _callers_ should care about proper special size. So now PageInit() just checks the right lengthsof page special and page header with assert, not enforcing size change silently. PFA my small patch on this. I'd proposeit to commit if in the HEAD only likewise the commit 4c0239cb7a7775e3183cb575e62703d71bf3302d. > > What do you think? I still feel that for special size let callers call PageInit with sizeof(special_structure) and PageInit do the alignment. Others may have different opinion. On the patch itself, how can we say that other special sizes are max aligned except sequence_magic structure? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: [PATCH] Improve treatment of page special and page header alignment during page init.
From
Pavel Borisov
Date:
ср, 7 апр. 2021 г. в 17:55, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
On Wed, Apr 7, 2021 at 5:32 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> I was looking at changes in Sp-Gist by commit 4c0239cb7a7775e3183cb575e62703d71bf3302d
> (discussion
> https://postgr.es/m/CALj2ACViOo2qyaPT7krWm4LRyRTw9kOXt+g6PfNmYuGA=YHj9A@mail.gmail.com ) and realized that during PageInit, both page header and page special are expected to be maxaligned but in reality, their treatment is quite different:
How can we say that in PageInit the SizeOfPageHeaderData is expected
to be max aligned? Am I missing something? There are lots of other
places where SizeOfPageHeaderData is used, not
MAXALIGN(SizeOfPageHeaderData).
Its maxalign is ensured by its size of 24bytes (which is maxalign'ed). I think if we change this to not-maxalign'ed value bad things can happen. So I've added assert checking for this value. I think it is similar situation for both page header and page special, I wonder why they've been treated differently in PageInit.
> 1. page special size is silently enforced to be maxaligned by PageInit() even if caller-specified specialSize is not of a maxalign'ed size.
> 2. page header size alignment is not checked at all (but we expect it maxalign'ed, yes).
>
> I'd propose do both things in the same way: just Assert both sizes are maxalign'ed during page init.
>
> I dived further and it appears that the only caller, who provides not properly aligned page special is fill_seq_with_data() and corrected it.
>
> I am really convinced, that _callers_ should care about proper special size. So now PageInit() just checks the right lengths of page special and page header with assert, not enforcing size change silently. PFA my small patch on this. I'd propose it to commit if in the HEAD only likewise the commit 4c0239cb7a7775e3183cb575e62703d71bf3302d.
>
> What do you think?
I still feel that for special size let callers call PageInit with
sizeof(special_structure) and PageInit do the alignment. Others may
have different opinion.
On the patch itself, how can we say that other special sizes are max
aligned except sequence_magic structure?
Alike for page header, it is ensured by the current size of page special in all access methods now (except the size of sequence_magic, which I've corrected in the call). If someone wants to break this in the future, there is an added assert checking in PageInit.
I think we should not maxalign both SizeOfPageHeaderData and specialSize manually, just check they have the right (already maxalign'ed) length to be safe in the future.
Re: [PATCH] Improve treatment of page special and page header alignment during page init.
From
Tom Lane
Date:
Pavel Borisov <pashkin.elfe@gmail.com> writes:
>> How can we say that in PageInit the SizeOfPageHeaderData is expected
>> to be max aligned? Am I missing something? There are lots of other
>> places where SizeOfPageHeaderData is used, not
>> MAXALIGN(SizeOfPageHeaderData).
> Its maxalign is ensured by its size of 24bytes (which is maxalign'ed). I
> think if we change this to not-maxalign'ed value bad things can happen. So
> I've added assert checking for this value. I think it is similar situation
> for both page header and page special, I wonder why they've been treated
> differently in PageInit.
No, that's wrong. What follows the page header is the line pointer
array, which is only int-aligned. We need to maxalign the special
space because tuples are stored working backwards from that, and
we want maxalignment for tuples.
regards, tom lane
Re: [PATCH] Improve treatment of page special and page header alignment during page init.
From
Pavel Borisov
Date:
No, that's wrong. What follows the page header is the line pointer
array, which is only int-aligned. We need to maxalign the special
space because tuples are stored working backwards from that, and
we want maxalignment for tuples.
Ok, I realized. Thanks!
Then I'd call off the proposal.