Re: [HACKERS] use less space in xl_xact_commit patch
With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. If you'd like to rework like that please, otherwise I can take it from here if you'd like. I think I did it; while doing it, I think I've found a bug: I didn't update recoveryStopsHere. Please double check that, as I really don't know what I'm doing there... Should I also change the struct name from xl_xact_commit to xl_xact_commit_fast_path? How can I test it with weird stuff as subtransactions, shared cache invalidation messages...? make installcheck should cover those. Ok, all tests passed. commitlog_lessbytes_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, Jun 15, 2011 at 10:55 AM, Leonardo Francalanci m_li...@yahoo.it wrote: On Wed, May 25, 2011 at 3:05 PM, Simon Riggs si...@2ndquadrant.com wrote: Leonardo, can you submit an updated version of this patch today that incorporates Simon's suggestion? Mmmh, maybe it was simpler than I thought; I must be missing something... patch attached No, I think it is that simple. Patch looks OK on initial eyeball. More detailed review to follow. With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. If you'd like to rework like that please, otherwise I can take it from here if you'd like. How can I test it with weird stuff as subtransactions, shared cache invalidation messages...? make installcheck should cover those. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 2:00 PM, Leonardo Francalanci m_li...@yahoo.it wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. If you'd like to rework like that please, otherwise I can take it from here if you'd like. I think I did it; while doing it, I think I've found a bug: I didn't update recoveryStopsHere. Please double check that, as I really don't know what I'm doing there... Should I also change the struct name from xl_xact_commit to xl_xact_commit_fast_path? Yes please. How can I test it with weird stuff as subtransactions, shared cache invalidation messages...? make installcheck should cover those. Ok, all tests passed. Even better. Will review, thanks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 9:22 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. At any rate we shouldn't name the stuct one way (xl_xact_commit and xl_xact_commit_with_info) and the constants the other way (XLOG_XACT_COMMIT and XLOG_XACT_COMMIT_FASTPATH). They should match. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 2:22 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. The important thing is that we retain backwards compatibility with current XLOG_XACT_COMMIT. I'm not worried what we call the other one. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
The important thing is that we retain backwards compatibility with current XLOG_XACT_COMMIT. I'm not worried what we call the other one. Ok, let me see if I got it right: #define XLOG_XACT_COMMIT0x00 should become: #define XLOG_XACT_COMMIT_WITH_INFO 0x00 and I'll add a #define XLOG_XACT_COMMIT 0x60 Than I'll leave 2 structs, xl_xact_commit_with_info and xl_xact_commit. Leonardo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
Robert Haas robertmh...@gmail.com writes: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. Yes. There is no need to preserve backwards compatibility here, so let's just design the records in a way that makes sense on its own. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
Excerpts from Leonardo Francalanci's message of jue jun 16 09:00:15 -0400 2011: Should I also change the struct name from xl_xact_commit to xl_xact_commit_fast_path? Yes, please. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. Yes. There is no need to preserve backwards compatibility here, so let's just design the records in a way that makes sense on its own. The only difference I'm proposing is the naming. It was foolish of me to propose that the data structure that is exactly the same should have a different name, yet the new structure should have the same name as the previous version. That will lead to confusion to no benefit. My second suggestion makes sense on its own, for no other reason. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 12:12 PM, Simon Riggs si...@2ndquadrant.com wrote: On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. Yes. There is no need to preserve backwards compatibility here, so let's just design the records in a way that makes sense on its own. The only difference I'm proposing is the naming. It was foolish of me to propose that the data structure that is exactly the same should have a different name, yet the new structure should have the same name as the previous version. That will lead to confusion to no benefit. My second suggestion makes sense on its own, for no other reason. That's a reasonable point, but I still don't really like the name fastpath, because it's not faster, and it's not a path. It's just smaller. How about xl_xact_commit_simple or xl_xact_commit_compact or something like that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
Robert Haas robertmh...@gmail.com writes: That's a reasonable point, but I still don't really like the name fastpath, because it's not faster, and it's not a path. It's just smaller. How about xl_xact_commit_simple or xl_xact_commit_compact or something like that? bikeshed xl_xact_commit_short ? /bikeshed _simple would probably be my next choice. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Thu, Jun 16, 2011 at 5:34 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 16, 2011 at 12:12 PM, Simon Riggs si...@2ndquadrant.com wrote: On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs si...@2ndquadrant.com wrote: With regards to the naming, I think it would be better if we kept XLOG_XACT_COMMIT record exactly as it is now, and make the second record an entirely new record called XLOG_XACT_COMMIT_FASTPATH. That way we retain backwards compatibility. I liked your previous suggestion of commit and commit-with-info better. There's nothing particularly fast about this; it's just less info. So to speak. Yes. There is no need to preserve backwards compatibility here, so let's just design the records in a way that makes sense on its own. The only difference I'm proposing is the naming. It was foolish of me to propose that the data structure that is exactly the same should have a different name, yet the new structure should have the same name as the previous version. That will lead to confusion to no benefit. My second suggestion makes sense on its own, for no other reason. That's a reasonable point, but I still don't really like the name fastpath, because it's not faster, and it's not a path. It's just smaller. How about xl_xact_commit_simple or xl_xact_commit_compact or something like that? Sure, anything like that is fine for me. Rather annoyed at my email client because I lost my first email on this topic, then had to retype it all from memory. The second copy omitted things like or similar name, that's not important which would have avoided the last couple of mails. :-( -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
We don't need to be in a hurry here. As the reviewer I'm happy to give Leonardo some time, obviously no more than the end of the commit fest. If he doesn't respond at all, I'll do it, but I'd like to give him the chance and the experience if possible. Sorry I couldn't update the patch (in fact, it's more of a total-rewrite than an update). How much time do I have? Leonardo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Tue, Jun 14, 2011 at 2:31 PM, Simon Riggs si...@2ndquadrant.com wrote: Well, we certainly have the option to review and commit the patch any time up until feature freeze. However, I don't want the CommitFest application to be full of entries for patches that are not actually being worked on, because it makes it hard for reviewers to figure out which patches in a state where they can be usefully looked at. AIUI, this one is currently not, because it was reviewed three weeks ago and hasn't been updated. Yes it's true: I thought I could find the time to work on it, but I didn't. Let me know the deadline for it, and I'll see if I can make it (or I'll make it in the next commit fest). Leonardo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 3:05 PM, Simon Riggs si...@2ndquadrant.com wrote: Leonardo, can you submit an updated version of this patch today that incorporates Simon's suggestion? Mmmh, maybe it was simpler than I thought; I must be missing something... patch attached How can I test it with weird stuff as subtransactions, shared cache invalidation messages...? Leonardo commitlog_lessbytes_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 3:05 PM, Simon Riggs si...@2ndquadrant.com wrote: Yes, that's correct. We can remove them from the normal commit record when nmsgs == 0. Leonardo, can you submit an updated version of this patch today that incorporates Simon's suggestion? The CommitFest starts tomorrow. If not, please feel free to resubmit to the next CommitFest. Simon or I may find the time to review it sooner rather than later even if it misses the deadline for this CommitFest, because I think it's an important optimization and I know you have other work that depends on it. But for now we should mark it Returned with Feedback if you don't have an updated version ready to go. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Tue, Jun 14, 2011 at 4:21 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, May 25, 2011 at 3:05 PM, Simon Riggs si...@2ndquadrant.com wrote: Yes, that's correct. We can remove them from the normal commit record when nmsgs == 0. Leonardo, can you submit an updated version of this patch today that incorporates Simon's suggestion? The CommitFest starts tomorrow. If not, please feel free to resubmit to the next CommitFest. Simon or I may find the time to review it sooner rather than later even if it misses the deadline for this CommitFest, because I think it's an important optimization and I know you have other work that depends on it. But for now we should mark it Returned with Feedback if you don't have an updated version ready to go. We don't need to be in a hurry here. As the reviewer I'm happy to give Leonardo some time, obviously no more than the end of the commit fest. If he doesn't respond at all, I'll do it, but I'd like to give him the chance and the experience if possible. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Tue, Jun 14, 2011 at 2:31 PM, Simon Riggs si...@2ndquadrant.com wrote: We don't need to be in a hurry here. As the reviewer I'm happy to give Leonardo some time, obviously no more than the end of the commit fest. Well, we certainly have the option to review and commit the patch any time up until feature freeze. However, I don't want the CommitFest application to be full of entries for patches that are not actually being worked on, because it makes it hard for reviewers to figure out which patches in a state where they can be usefully looked at. AIUI, this one is currently not, because it was reviewed three weeks ago and hasn't been updated. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 18, 2011 at 2:11 PM, Leonardo Francalanci m_li...@yahoo.it wrote: this is a second version: now using int counts[1]; /* variable-length array of counts */ in xl_xact_commit to keep track of number of different arrays at the end of the struct. Waiting for feedbacks... I can't find a clear discussion of what you are trying to do, and how, just a URL back to a complex discussion on another topic. I can't see any analysis that shows whether this would be effective in reducing space of WAL records and what the impacts might be. The patch contains very close to zero comments. Please explain what you are trying to do, and what the likely benefits of doing it will be. Add comments to the patch to make that very clear, e.g. As of 9.2, the record format changed to reduce space... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
Da: Simon Riggs si...@2ndquadrant.com I can't find a clear discussion of what you are trying to do, and how, just a URL back to a complex discussion on another topic. While trying to write a patch to allow changing an unlogged table into a logged one, I had to add another int field to xl_xact_commit. Robert Haas said: I have to admit I don't like this approach very much. I can't see adding 4 bytes to every commit record for this feature. which is a correct remark. xl_xact_commit can contain some arrays (relation to drops, committed sub-trans, shared invalidation msgs). The length of these arrays is specified using 3 ints in the struct. So, to avoid adding more ints to the struct, I've been suggested to remove all the ints, and use xl_xact_commit.xinfo to flag which arrays are, in fact, present. So the whole idea is: - remove nrels, nsubxacts and nmsgs from xl_xact_commit - use bits in xinfo to signal which arrays are present at the end of xl_xact_commit - for each present array, add the length of the array (as int) at the end ofxl_xact_commit - add each present array after all the lengths I can't see any analysis that shows whether this would be effective in reducing space of WAL records and what the impacts might be. The space of WAL records should always be = than without the patch: in the worst case scenario, all ints are present (as they would without the patch). In the best case, which I guess is the more common, each xl_xact_commit will be 3 ints shorter. I don't think the effect will be perceivable: the whole idea is to allow more variable-length structures in xl_xact_commit in the future, without incrementing the space on disk. The patch contains very close to zero comments. I tried to add some. Leonardo commitlog_lessbytes01.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
commitlog_lessbytes02.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
Sorry, email sent without body. Fixed some English mistakes. commitlog_lessbytes02.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci m_li...@yahoo.it wrote: Da: Simon Riggs si...@2ndquadrant.com I can't find a clear discussion of what you are trying to do, and how, just a URL back to a complex discussion on another topic. While trying to write a patch to allow changing an unlogged table into a logged one, I had to add another int field to xl_xact_commit. Robert Haas said: I have to admit I don't like this approach very much. I can't see adding 4 bytes to every commit record for this feature. which is a correct remark. xl_xact_commit can contain some arrays (relation to drops, committed sub-trans, shared invalidation msgs). The length of these arrays is specified using 3 ints in the struct. So, to avoid adding more ints to the struct, I've been suggested to remove all the ints, and use xl_xact_commit.xinfo to flag which arrays are, in fact, present. So the whole idea is: - remove nrels, nsubxacts and nmsgs from xl_xact_commit - use bits in xinfo to signal which arrays are present at the end of xl_xact_commit - for each present array, add the length of the array (as int) at the end of xl_xact_commit - add each present array after all the lengths OK, thats clear. Thanks. That formatting sounds quite complex. I would propose we split this into 2 WAL records: xl_xact_commit and xl_xact_commit_with_info xl_xact_commit doesn't have any flags, counts or arrays. xl_xact_commit_with_info always has all 3 counts, even if zero. Arrays follow the main record I think it might also be possible to removed dbId and tsId from xl_act_commit if we use that definition. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 3:41 PM, Simon Riggs si...@2ndquadrant.com wrote:
On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci m_li...@yahoo.it
wrote:
Da: Simon Riggs si...@2ndquadrant.com
I can't find a clear discussion of what you are trying to do, and how,
just a URL back to a complex discussion on another topic.
While trying to write a patch to allow changing an unlogged table into
a logged one, I had to add another int field to xl_xact_commit.
Robert Haas said:
I have to admit I don't like this approach very much. I can't see
adding 4 bytes to every commit record for this feature.
which is a correct remark.
xl_xact_commit can contain some arrays (relation to drops,
committed sub-trans, shared invalidation msgs). The length of
these arrays is specified using 3 ints in the struct.
So, to avoid adding more ints to the struct, I've been suggested to
remove all the ints, and use xl_xact_commit.xinfo to flag which
arrays are, in fact, present.
So the whole idea is:
- remove nrels, nsubxacts and nmsgs from xl_xact_commit
- use bits in xinfo to signal which arrays are present at the end
of xl_xact_commit
- for each present array, add the length of the array (as int) at
the end of xl_xact_commit
- add each present array after all the lengths
OK, thats clear. Thanks.
That formatting sounds quite complex.
I would propose we split this into 2 WAL records: xl_xact_commit and
xl_xact_commit_with_info
xl_xact_commit doesn't have any flags, counts or arrays.
xl_xact_commit_with_info always has all 3 counts, even if zero.
Arrays follow the main record
I think it might also be possible to removed dbId and tsId from
xl_act_commit if we use that definition.
Yes, that's correct. We can remove them from the normal commit record
when nmsgs == 0.
I think we can get away with these 2 definitions, but pls check. Using
static definitions works better for me because we can see what they
contain, rather than having the info flags imply that the record can
contain any permutation of settings when that's not really possible.
{
TimestampTz xact_time; /* time of commit */
int nsubxacts; /* number of
subtransaction XIDs */
/* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
} xl_xact_commit;
{
TimestampTz xact_time; /* time of commit */
uint32 xinfo; /* info flags */
int nrels; /* number of
RelFileNodes */
int nsubxacts; /* number of
subtransaction XIDs */
int nmsgs; /* number of shared inval msgs
*/
Oid dbId; /* MyDatabaseId */
Oid tsId; /* MyDatabaseTableSpace
*/
/* Array of RelFileNode(s) to drop at commit */
RelFileNode xnodes[1]; /* VARIABLE LENGTH ARRAY */
/* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
/* ARRAY OF SHARED INVALIDATION MESSAGES FOLLOWS */
} xl_xact_commit_with_info;
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 10:41 AM, Simon Riggs si...@2ndquadrant.com wrote: OK, thats clear. Thanks. That formatting sounds quite complex. I would propose we split this into 2 WAL records: xl_xact_commit and xl_xact_commit_with_info xl_xact_commit doesn't have any flags, counts or arrays. xl_xact_commit_with_info always has all 3 counts, even if zero. Arrays follow the main record I think it might also be possible to removed dbId and tsId from xl_act_commit if we use that definition. That's an interesting idea. I hadn't noticed that if nmsgs=0 then those fields aren't needed either. Also, both nrels0 and nmsgs0 will probably only happen if some DDL has been done, which most transactions probably don't, so it would be logical to have one record type that excludes nrels, nmsgs, dbId, tsId, and another record type that includes all of those. In fact, we could probably throw in xinfo as well: if there's no DDL involved, we shouldn't need to set XACT_COMPLETION_UPDATE_RELCACHE_FILE or XACT_COMPLETION_FORCE_SYNC_COMMIT either. But I'm not so sure about nsubxacts. It seems to me that we might lose a lot of the benefit of having two record types if we have to use the larger one whenever nsubxacts0. I'm not exactly sure how common subtransactions are, but if I had to guess I'd speculate that they are far more frequent than DDL operations. Maybe we should think about making the xl_xact_commit record contain just xact_time, nsubxacts, and a subxact array; and then have the xl_xact_commit_with_info (or maybe xl_xact_commit_full, or whatever we decide to call it) contain everything else. That would shave off 20 bytes from the commit records of any non-DDL operation, which would be pretty nice. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 25, 2011 at 3:05 PM, Simon Riggs si...@2ndquadrant.com wrote:
On Wed, May 25, 2011 at 3:41 PM, Simon Riggs si...@2ndquadrant.com wrote:
On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci m_li...@yahoo.it
wrote:
Da: Simon Riggs si...@2ndquadrant.com
I can't find a clear discussion of what you are trying to do, and how,
just a URL back to a complex discussion on another topic.
While trying to write a patch to allow changing an unlogged table into
a logged one, I had to add another int field to xl_xact_commit.
Robert Haas said:
I have to admit I don't like this approach very much. I can't see
adding 4 bytes to every commit record for this feature.
which is a correct remark.
xl_xact_commit can contain some arrays (relation to drops,
committed sub-trans, shared invalidation msgs). The length of
these arrays is specified using 3 ints in the struct.
So, to avoid adding more ints to the struct, I've been suggested to
remove all the ints, and use xl_xact_commit.xinfo to flag which
arrays are, in fact, present.
So the whole idea is:
- remove nrels, nsubxacts and nmsgs from xl_xact_commit
- use bits in xinfo to signal which arrays are present at the end
of xl_xact_commit
- for each present array, add the length of the array (as int) at
the end of xl_xact_commit
- add each present array after all the lengths
OK, thats clear. Thanks.
That formatting sounds quite complex.
I would propose we split this into 2 WAL records: xl_xact_commit and
xl_xact_commit_with_info
xl_xact_commit doesn't have any flags, counts or arrays.
xl_xact_commit_with_info always has all 3 counts, even if zero.
Arrays follow the main record
I think it might also be possible to removed dbId and tsId from
xl_act_commit if we use that definition.
Yes, that's correct. We can remove them from the normal commit record
when nmsgs == 0.
I think we can get away with these 2 definitions, but pls check. Using
static definitions works better for me because we can see what they
contain, rather than having the info flags imply that the record can
contain any permutation of settings when that's not really possible.
{
TimestampTz xact_time; /* time of commit */
int nsubxacts; /* number of
subtransaction XIDs */
/* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
} xl_xact_commit;
{
TimestampTz xact_time; /* time of commit */
uint32 xinfo; /* info flags */
int nrels; /* number of
RelFileNodes */
int nsubxacts; /* number of
subtransaction XIDs */
int nmsgs; /* number of shared inval msgs
*/
Oid dbId; /* MyDatabaseId */
Oid tsId; /*
MyDatabaseTableSpace */
/* Array of RelFileNode(s) to drop at commit */
RelFileNode xnodes[1]; /* VARIABLE LENGTH ARRAY */
/* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
/* ARRAY OF SHARED INVALIDATION MESSAGES FOLLOWS */
} xl_xact_commit_with_info;
Wow, that is identical to the conclusion that I came to about 60 seconds ago.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Wed, May 18, 2011 at 9:11 AM, Leonardo Francalanci m_li...@yahoo.it wrote: this is a second version: now using int counts[1]; /* variable-length array of counts */ in xl_xact_commit to keep track of number of different arrays at the end of the struct. Waiting for feedbacks... Looks reasonable on a quick once-over; please add it to the next CommitFest so that it gets a more detailed review. https://commitfest.postgresql.org/action/commitfest_view/open -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
int counts[1]; /* variable-length array of counts, xinfo flags define length of array and meaning of counts */ Damn, that's much cleaner than what I did. I don't know why I stuck with the idea that it had to be: int array int array ... instead of: int int ... array array ... which makes much more sense. Then, I'd make macros like this: #define XactCommitNumberOfDroppedRelFileNodes(xlrec) \ ((xlref-xinfo XACT_COMMIT_DROPPED_RELFILENODES) ? xlrec-counts[0] : 0) #define XactCommitNumberOfCommittedSubXids(xlrec) \ ((xlref-xinfo XACT_COMMITED_SUBXDIDS) ? xlrec-counts[(xlrec-xinfo XACT_COMMIT_DROPPED_RELFILENODES) ? 1 : 0] : 0) ...etc... ehm I don't know if macros would be enough; that's ok for the first 2, then I think it would become a mess... Maybe I'll use a simple function that gets all ints at once. ...and a similar set of macros that will return a pointer to the beginning of the corresponding array, if it's present. I'd lay out the record like this: - main record - array of counts (might be zero-length) - array of dropped relfilnodes (if any) - array of committed subxids (if any) - array of sinval messages (if any) ok Also, it's important not to confuse xact completion with xact commit, as I think some of your naming does. Completion could perhaps be thought to include abort. mmh... I don't know if I got it... but I'll give a look, and ask questions... Thank you very much for the help Leonardo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
this is a second version: now using intcounts[1]; /* variable-length array of counts */ in xl_xact_commit to keep track of number of different arrays at the end of the struct. Waiting for feedbacks... Leonardo commitlog_lessbytes00.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] use less space in xl_xact_commit patch
On Mon, May 16, 2011 at 11:20 AM, Leonardo Francalanci m_li...@yahoo.it wrote: following the conversation at http://postgresql.1045698.n5.nabble.com/switch-UNLOGGED-to-LOGGED-tp4290461p4382333.html I tried to remove some bytes from xl_xact_commit. The way I did it needs palloc+memcpy. I guess it could be done reusing the memory for smgrGetPendingDeletes. But I don't think it's that important. I guess there are other ways of doing it; let me know what you think. I don't think there's much point to the xl_xact_commit_opt structure; it doesn't really do anything. What I would do is end the xl_xact_commit structure with something like: int counts[1]; /* variable-length array of counts, xinfo flags define length of array and meaning of counts */ Then, I'd make macros like this: #define XactCommitNumberOfDroppedRelFileNodes(xlrec) \ ((xlref-xinfo XACT_COMMIT_DROPPED_RELFILENODES) ? xlrec-counts[0] : 0) #define XactCommitNumberOfCommittedSubXids(xlrec) \ ((xlref-xinfo XACT_COMMITED_SUBXDIDS) ? xlrec-counts[(xlrec-xinfo XACT_COMMIT_DROPPED_RELFILENODES) ? 1 : 0] : 0) ...etc... ...and a similar set of macros that will return a pointer to the beginning of the corresponding array, if it's present. I'd lay out the record like this: - main record - array of counts (might be zero-length) - array of dropped relfilnodes (if any) - array of committed subxids (if any) - array of sinval messages (if any) Also, it's important not to confuse xact completion with xact commit, as I think some of your naming does. Completion could perhaps be thought to include abort. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
