pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedxl_heap_header alignment?
11+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2020-08-31 1e0512ff23 160c8a4d38 b1f52817bc e4263c4a56 787ccf5a5f fd9451d201 7d26d85f1b C comment: remove mention of use of t_hoff WAL structure member
* xl_heap_header alignment?
@ 2020-07-21 17:45 Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 11+ messages in thread
From: Antonin Houska @ 2020-07-21 17:45 UTC (permalink / raw)
To: pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
@ 2020-07-21 18:02 ` Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Andres Freund @ 2020-07-21 18:02 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org,Antonin Houska <ah@cybertec.at>; pgsql-hackers
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.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
@ 2020-07-21 18:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2020-07-21 18:33 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org, Antonin Houska <ah@cybertec.at>
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-07-22 04:58 ` Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 11+ messages in thread
From: Antonin Houska @ 2020-07-22 04:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
@ 2020-08-22 00:40 ` Bruce Momjian <bruce@momjian.us>
2020-08-22 03:07 ` Re: xl_heap_header alignment? David G. Johnston <david.g.johnston@gmail.com>
2020-08-22 18:48 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
0 siblings, 2 replies; 11+ messages in thread
From: Bruce Momjian @ 2020-08-22 00:40 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; pgsql-hackers@lists.postgresql.org
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.diff (text/x-diff, 690B) download
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?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
@ 2020-08-22 03:07 ` David G. Johnston <david.g.johnston@gmail.com>
2020-08-22 15:37 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 11+ messages in thread
From: David G. Johnston @ 2020-08-22 03:07 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Antonin Houska <ah@cybertec.at>; Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
2020-08-22 03:07 ` Re: xl_heap_header alignment? David G. Johnston <david.g.johnston@gmail.com>
@ 2020-08-22 15:37 ` Bruce Momjian <bruce@momjian.us>
2020-08-22 15:45 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Momjian @ 2020-08-22 15:37 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.diff (text/x-diff, 701B) download
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?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
2020-08-22 03:07 ` Re: xl_heap_header alignment? David G. Johnston <david.g.johnston@gmail.com>
2020-08-22 15:37 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
@ 2020-08-22 15:45 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 11+ messages in thread
From: Tom Lane @ 2020-08-22 15:45 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; Antonin Houska <ah@cybertec.at>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
@ 2020-08-22 18:48 ` Antonin Houska <ah@cybertec.at>
2020-08-22 19:00 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 11+ messages in thread
From: Antonin Houska @ 2020-08-22 18:48 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: tgl@sss.pgh.pa.us; andres@anarazel.de; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
2020-08-22 18:48 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
@ 2020-08-22 19:00 ` Antonin Houska <ah@cybertec.at>
2020-08-31 17:58 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 11+ messages in thread
From: Antonin Houska @ 2020-08-22 19:00 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: tgl@sss.pgh.pa.us; andres@anarazel.de; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: xl_heap_header alignment?
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Re: xl_heap_header alignment? Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Re: xl_heap_header alignment? Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Re: xl_heap_header alignment? Bruce Momjian <bruce@momjian.us>
2020-08-22 18:48 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-08-22 19:00 ` Re: xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
@ 2020-08-31 17:58 ` Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 11+ messages in thread
From: Bruce Momjian @ 2020-08-31 17:58 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: tgl@sss.pgh.pa.us; andres@anarazel.de; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2020-08-31 17:58 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 17:45 xl_heap_header alignment? Antonin Houska <ah@cybertec.at>
2020-07-21 18:02 ` Andres Freund <andres@anarazel.de>
2020-07-21 18:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-22 04:58 ` Antonin Houska <ah@cybertec.at>
2020-08-22 00:40 ` Bruce Momjian <bruce@momjian.us>
2020-08-22 03:07 ` David G. Johnston <david.g.johnston@gmail.com>
2020-08-22 15:37 ` Bruce Momjian <bruce@momjian.us>
2020-08-22 15:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-08-22 18:48 ` Antonin Houska <ah@cybertec.at>
2020-08-22 19:00 ` Antonin Houska <ah@cybertec.at>
2020-08-31 17:58 ` Bruce Momjian <bruce@momjian.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox