Re: [PATCH 02/14] Add support for a generic wal reading facility dubbed XLogReader

Forgot attachment..

On 11.12.2012 15:55, Heikki Linnakangas wrote:
> I've been molding this patch for a while now, here's what I have this
> far (also available in my git repository).
>
> The biggest change is in the error reporting. A stand-alone program that
> wants to use xlogreader.c no longer has to provide a full-blown
> replacement for ereport(). The only thing that xlogreader.c used
> ereport() was when it encounters an invalid record. And even there we
> had the emode_for_corrupt_record hack. I think it's a much better API
> that XLogReadRecord just returns NULL on an invalid record, and an error
> string, and the caller can do what it wants with that. In xlog.c, we'll
> pass the error string to ereport(), with the right emode as determined
> by emode_for_corrupt_record. xlog.c is no longer concerned with
> emode_for_corrupt_record, or error levels in general.
>
> We talked about this earlier, and Tom Lane argued that "it's basically
> insane to imagine that you can carve out a non-trivial piece of the
> backend that doesn't contain any elog calls."
> (http://archives.postgresql.org/pgsql-hackers/2012-09/msg00651.php), but
> having done just that, it doesn't seem insane to me. xlogreader.c really
> is a pretty well contained piece of code. All the complicated stuff that
> contains elog calls and pallocs and more is in the callback, which can
> freely use all the normal backend infrastructure.
>
> Now, here's some stuff that still need to be done:
>
> * A stand-alone program using xlogreader.c has to provide an
> implementation of tliInHistory(). Need to find a better way to do that.
> Perhaps "#ifndef FRONTEND" the tliInHistory checks in xlogreader.
>
> * In xlog.c, some of the variables that used to be statics like
> readFile, readOff etc. are now in the XLogPageReadPrivate struct. But
> there's still plenty of statics left in there - it would certainly not
> work correctly if xlog.c tried to open two xlog files at the same time.
> I think it's just confusing to have some stuff in the
> XLogPageReadPrivate struct, and others as static, so I think we should
> get rid of XLogPageReadPrivate struct altogether and put back the static
> variables. At least it would make the diff smaller, which might help
> with reviewing. xlog.c probably doesn't need to provide a "private"
> struct to xlogreader.c at all, which is okay.
>
> * It's pretty ugly that to use the rm_desc functions, you have to
> provide dummy implementations of a bunch of backend functions, including
> pfree() and timestamptz_to_str(). Should find a better way to do that.
>
> * It's not clear to me how we'd handle translating the strings in
> xlogreader.c, when xlogreader.c is used in a stand-alone program like
> pg_xlogdump. Maybe we can just punt on that...
>
> * How about we move pg_xlogdump to contrib? It doesn't feel like the
> kind of essential tool that deserves to be in src/bin.
>
> - Heikki

--
- Heikki