Re: [PATCH] Improve treatment of page special and page header alignment during page init.

ср, 7 апр. 2021 г. в 17:55, Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com>:

> On Wed, Apr 7, 2021 at 5:32 PM Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)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.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>