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 struct is 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 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
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 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.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
xl.difftext/x-diff; charset=us-asciiDownload
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
{
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
Attachments:
xl.difftext/x-diff; charset=us-asciiDownload
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
{
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
On Sat, Aug 22, 2020 at 09:00:15PM +0200, Antonin Houska wrote:
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.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee