Re: xl_heap_header alignment?
On Sat, Aug 22, 2020 at 09:00:15PM +0200, Antonin Houska wrote: > Antonin Houska 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. Patch applied. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: xl_heap_header alignment?
Antonin Houska 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
Re: xl_heap_header alignment?
Bruce Momjian wrote: > On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > > Tom Lane 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
Re: xl_heap_header alignment?
Bruce Momjian 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
Re: xl_heap_header alignment?
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 wrote: > > On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > > Tom Lane 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 https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index aa17f7df84..08ab025e67 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -137,8 +137,7 @@ typedef struct xl_heap_truncate * or updated tuple in WAL; we can save a few bytes by reconstructing the * fields that are available elsewhere in the WAL record, or perhaps just * plain needn't be reconstructed. These are the fields we must store. - * NOTE: t_hoff could be recomputed, but we may as well store it because - * it will come for free due to alignment considerations. + * FYI, t_hoff could be recomputed each time it is needed. */ typedef struct xl_heap_header {
Re: xl_heap_header alignment?
On Fri, Aug 21, 2020 at 5:41 PM Bruce Momjian wrote: > On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > > Tom Lane 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.
Re: xl_heap_header alignment?
On Wed, Jul 22, 2020 at 06:58:33AM +0200, Antonin Houska wrote: > Tom Lane 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. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index aa17f7df84..38683893c0 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -137,8 +137,7 @@ typedef struct xl_heap_truncate * or updated tuple in WAL; we can save a few bytes by reconstructing the * fields that are available elsewhere in the WAL record, or perhaps just * plain needn't be reconstructed. These are the fields we must store. - * NOTE: t_hoff could be recomputed, but we may as well store it because - * it will come for free due to alignment considerations. + * FYI, t_hoff could potentially be recomputed. */ typedef struct xl_heap_header {
Re: xl_heap_header alignment?
Tom Lane 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
Re: xl_heap_header alignment?
Andres Freund writes: > On July 21, 2020 10:45:37 AM PDT, Antonin Houska 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 struct is 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
Re: xl_heap_header alignment?
Hi, On July 21, 2020 10:45:37 AM PDT, Antonin Houska 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 struct is stored well aligned). Regards, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.