Re: [PATCH] XLogReader v2 - Mailing list pgsql-hackers

Hi Alvaro, hi all,

On Tuesday, September 04, 2012 09:33:54 PM Alvaro Herrera wrote:
> 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.)
Hm. I am generally not very happy with the logging as is. I don't want to rely 
on elog() at all because that means the code suddently depends on just about 
the whole backend which sucks (see my god ulgy makefile hack for that...).

If we were to use that approach is there a platform that stops us from using 
vararg macros? I *think* it is C99...

I though about having a ->log(format, ...) callback, but that would mean loads 
of places add a unneccesary indirect function call :(

> * In the code beautification front, there are a number of cuddled braces
> and improperly indented function declarations.
I never seem to get those right. I really tried to make a pass over the whole 
file correcting them...

> * 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.
Yup. Readxlog was the name before someone (I think Simon) reminded me gently 
that it would be better placed in access/transam/ than replication/logical and 
that seemed to conform better to the local naming rules.

> * 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.
The whole error handling needs to be changed. It really depends on the use-
case how failures should be handled. I am just not sure what the best way 
is...

Just a ->error(severity, message, format) callback?

> * 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.)
When I tried it the code got even more state-machinery with individual parts 
returning status codes and switch()es around that handling the control flow 
from that... Maybe I have stared at it too long to see the way forward.

> * 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.
You seem to have a good grasp on that in the other thread...

> * why do we pass timeline_id to xlogdump?  I don't see that it's used
> anywhere, but maybe I'm missing something?
Its only unused because xlogdump as it submitted is just a POC hack... You 
need the timeline id to know which files to open. The only reason the parameter 
isn't parsed is that it is currently hardcoded in the callsites for 
XLogDumpXLogRead/write. At least there are FIXMEs arround it...

> 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.
Have you already started doing anything about it? I can redo a version but 
before we agree on the strategy for logging & error handling the only thing 
that would change is the cuddly braces and the IDENTIFCATION tags...

Thanks!

Andres
-- 
Andres Freund        http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services