Thread: xl_heap_header alignment?
I don't quite understand this part of the comment of the xl_heap_header structure: * NOTE: t_hoff could be recomputed, but we may as well store it because * it will come for free due to alignment considerations. What are the alignment considerations? The WAL code does not appear to assume any alignment, and therefore it uses memcpy() to copy the structure into a local variable before accessing its fields. For example, heap_xlog_insert(). -- Antonin Houska Web: https://www.cybertec-postgresql.com
Hi, On July 21, 2020 10:45:37 AM PDT, Antonin Houska <ah@cybertec.at> wrote: >I don't quite understand this part of the comment of the xl_heap_header >structure: > >* NOTE: t_hoff could be recomputed, but we may as well store it because > * it will come for free due to alignment considerations. > >What are the alignment considerations? The WAL code does not appear to >assume >any alignment, and therefore it uses memcpy() to copy the structure >into a >local variable before accessing its fields. For example, >heap_xlog_insert(). Unless you declare them as packed, structs will add padding to align members correctly (if, and only if, the whole structis stored well aligned). Regards, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> writes:
> On July 21, 2020 10:45:37 AM PDT, Antonin Houska <ah@cybertec.at> wrote:
>> I don't quite understand this part of the comment of the xl_heap_header
>> structure:
>> * NOTE: t_hoff could be recomputed, but we may as well store it because
>> * it will come for free due to alignment considerations.
> Unless you declare them as packed, structs will add padding to align members correctly (if, and only if, the whole
structis stored well aligned).
I think that comment may be out of date, because what's there now is
* NOTE: t_hoff could be recomputed, but we may as well store it because
* it will come for free due to alignment considerations.
*/
typedef struct xl_heap_header
{
uint16 t_infomask2;
uint16 t_infomask;
uint8 t_hoff;
} xl_heap_header;
I find it hard to see how tacking t_hoff onto what would have been a
4-byte struct is "free". Maybe sometime in the dim past there was
another field in this struct? (But I checked back as far as 7.4
without finding one.)
I don't particularly want to remove the field, but we ought to
change or remove the comment.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't particularly want to remove the field, but we ought to > change or remove the comment. I'm not concerned about the existence of the field as well. The comment just made me worried that I might be missing some fundamental concept. Thanks for your opinion. -- Antonin Houska Web: https://www.cybertec-postgresql.com
On Fri, Aug 21, 2020 at 5:41 PM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > I don't particularly want to remove the field, but we ought to
> > change or remove the comment.
>
> I'm not concerned about the existence of the field as well. The comment just
> made me worried that I might be missing some fundamental concept. Thanks for
> your opinion.
I have developed the attached patch to address this.
I would suggest either dropping the word "potentially" or removing the sentence. I'm not a fan of this in-between position on principle even if I don't understand the full reality of the implementation.
If leaving the word "potentially" is necessary it would be good to point out where the complexity is documented as a part of that - this header file probably not the best place to go into detail.
David J.
On Fri, Aug 21, 2020 at 08:07:34PM -0700, David G. Johnston wrote: > On Fri, Aug 21, 2020 at 5:41 PM Bruce Momjian <bruce@momjian.us> wrote: > > On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > > Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > I don't particularly want to remove the field, but we ought to > > > change or remove the comment. > > > > I'm not concerned about the existence of the field as well. The comment > just > > made me worried that I might be missing some fundamental concept. Thanks > for > > your opinion. > > I have developed the attached patch to address this. > > > I would suggest either dropping the word "potentially" or removing the > sentence. I'm not a fan of this in-between position on principle even if I > don't understand the full reality of the implementation. > > If leaving the word "potentially" is necessary it would be good to point out > where the complexity is documented as a part of that - this header file > probably not the best place to go into detail. Updated patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> Updated patch.
FWIW, I concur with the idea of just dropping that sentence altogether.
It's not likely that getting rid of that field is a line of development
that will ever be pursued; if anyone does get concerned about cutting
WAL size, there's a lot of more-valuable directions to go in.
regards, tom lane
Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > > Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > I don't particularly want to remove the field, but we ought to > > > change or remove the comment. > > > > I'm not concerned about the existence of the field as well. The comment just > > made me worried that I might be missing some fundamental concept. Thanks for > > your opinion. > > I have developed the attached patch to address this. Thanks. I wasn't sure if I'm expected to send the patch and then I forgot. If the comment tells that t_hoff can be computed (i.e. it's no necessary to include it in the structure), I think the comment should tell why it's yet included. Maybe something about "historical reasons"? Perhaps we can say that the storage used to be free due to padding, and that it's no longer so, but it's still "cheap", so it's not worth to teach the REDO functions to compute the value. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Antonin Houska <ah@cybertec.at> wrote: > If the comment tells that t_hoff can be computed (i.e. it's no necessary to > include it in the structure), I think the comment should tell why it's yet > included. Maybe something about "historical reasons"? Perhaps we can say that > the storage used to be free due to padding, and that it's no longer so, but > it's still "cheap", so it's not worth to teach the REDO functions to compute > the value. I've received some more replies to your email as soon as I had replied. I don't insist on my proposal, just go ahead with your simpler changes. -- Antonin Houska Web: https://www.cybertec-postgresql.com