On 11 January 2013 15:57, Simon Riggs <si...@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 of full-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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
