Re: [PATCH] XLogReader v2

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