Excerpts from Andres Freund's message of jue jul 19 06:29:03 -0400 2012: > Hi, > > Attached is v2 of the patch.
Hello, I gave this code a quick read some days ago. Here's the stuff I would change: * There are way too many #ifdef VERBOSE_DEBUG stuff for my taste. It might look better if you had macros such as elog_debug() that are defined to empty if VERBOSE_DEBUG is not defined. (The problem with such an approach is that you have to get into the business of creating one macro for each different param count, so elog_debug1(), elog_debug2() and so on. It also means you have to count the number of args in each call to ensure you're calling the right one.) * In the code beautification front, there are a number of cuddled braces and improperly indented function declarations. * I noticed that you have the IDENTIFICATION tag wrong in both .c and .h files: evidently you renamed the files from readxlog.[ch] to xlogreader. * There are a few elog(PANIC) calls. I am not sure that's a very good idea. It seems to me that you should be using elog(FATAL) there instead ... or do you really want to make the whole server crash? OTOH if we want to make it a true client program, all those elog() calls need to go. * XLogReaderRead() seems a bit too long to me. I would split it with auxiliary functions -- say "read a header" and "read a record". (I mentioned this to Andres on IM and he says he tried that but couldn't find any nice way to do it. I may still try to do it.) * xlogdump's Makefile trick to get all backend object files is ... ugly (an understatement). Really we need the *_desc() routines split so that it can use only those functions, and have a client-side replacement for StringInfo (discussed elsewhere) and some auxilliary functions such as relpathbackend() so that it can compile like a normal client. * why do we pass timeline_id to xlogdump? I don't see that it's used anywhere, but maybe I'm missing something? This is not a full review. After a new version with these fixes is published (either by Andres or myself) some more review might find more serious issues -- I didn't hunt for architectural problems in XLogReader. -- Álvaro Herrera 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