Re: use less space in xl_xact_commit patch

Hi,

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.

Leonardo

On Mon, May 16, 2011 at 11:20 AM, Leonardo Francalanci <m_lists(at)yahoo(dot)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

> 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

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...

Leonardo

On Wed, May 18, 2011 at 9:11 AM, Leonardo Francalanci <m_lists(at)yahoo(dot)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

On Wed, May 18, 2011 at 2:11 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)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

> Da: Simon Riggs <simon(at)2ndQuadrant(dot)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

> 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

On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)it> wrote:
>> Da: Simon Riggs <simon(at)2ndQuadrant(dot)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

On Wed, May 25, 2011 at 3:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)it> wrote:
>>> Da: Simon Riggs <simon(at)2ndQuadrant(dot)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

On Wed, May 25, 2011 at 10:41 AM, Simon Riggs <simon(at)2ndquadrant(dot)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 nrels>0 and nmsgs>0
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 nsubxacts>0. 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

On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, May 25, 2011 at 3:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)it> wrote:
>>>> Da: Simon Riggs <simon(at)2ndQuadrant(dot)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

On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Tue, Jun 14, 2011 at 4:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Tue, Jun 14, 2011 at 2:31 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

> 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

> On Tue, Jun 14, 2011 at 2:31 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

> > On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Wed, Jun 15, 2011 at 10:55 AM, Leonardo Francalanci <m_lists(at)yahoo(dot)it> wrote:
>> >  On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)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

> 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.

On Thu, Jun 16, 2011 at 2:00 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)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

On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Thu, Jun 16, 2011 at 9:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Thu, Jun 16, 2011 at 2:22 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

> 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_COMMIT 0x00

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Jun 16, 2011 at 12:12 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Thu, Jun 16, 2011 at 5:34 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 16, 2011 at 12:12 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Thu, Jun 16, 2011 at 3:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> On Thu, Jun 16, 2011 at 7:25 AM, Simon Riggs <simon(at)2ndquadrant(dot)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