On 11 January 2013 15:57, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> I've moved this to the next CF. I'm planning to review this one first.
>>
>> Thank you.
>
> Just reviewing the patch now, making more sense with comments added.
Making more sense, but not yet making complete sense.
I'd like you to revisit the patch comments since some of them are
completely unreadable.
Examples
"Frames the original tuple which needs to be inserted into the heap by
decoding the WAL tuplewith the help of old Heap tuple."
"The delta tuples for update WAL is to eliminate copying the entire
the new record to WAL for the update operation."
I don't mind rewording the odd line here and there, that's just normal
editing, but this needs extensive work in terms of number of places
requiring change and the level of change at each place. That's not
easy for me to do when I'm trying to understand the patch in the first
place. My own written English isn't that great, so please read some of
the other comments in other parts of the code so you can see the level
of clarity that's needed in PostgreSQL.
Copying chunks of text from other comments doesn't help much either,
especially when you miss out parts of the explanation. You refer to a
"history tag" but don't define it that well, and don't explain why it
might sometimes be 3 bytes, or what that means. pg_lzcompress doesn't
call it that either, which is confusing. If you use a concept from
elsewhere you should either use the same name, or if you rename it,
rename it in both places.
/** Do only the delta encode when the update is going to the same page and* buffer doesn't need a backup block in case
offull-pagewrite is on.*/
if ((oldbuf == newbuf) && !XLogCheckBufferNeedsBackup(newbuf))
The comment above says nothing. I can see that oldbuf and newbuf must
be the same and the call to XLogCheckBufferNeedsBackup is clear
because the function is already well named.
What I'd expect to see here is a discussion of why this test is being
applied and maybe why it is applied here. Such an important test
deserves a long discussion, perhaps 10-20 lines of comment.
Thanks
-- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
