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

On 2012-12-11 15:55:35 +0200, 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).

On a very quick this looks good. I will try to rebase the decoding stuff
and read a bit around in the course of that...

> 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.

This is pretty good. I was a bit afraid of making this change - thus my
really ugly emode callback hack - but this is way better.

> 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.

We could just leave it in xlogreader in the first place. Having an
#ifdef'ed out version in there seems to be schizophrenic to me, all the
maintenance overhead, none of the fun...

> * 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.

Fine with me. I find the grouping that the struct provides somewhat
helpful when reading the code, but its more than offset by duplicating
some of the variables.
The reasons to have it are fewer compared when you'd introduced the
struct - xlogreader does more now, so less needs to be handled outside.

> * 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.

I think most of the cases requiring those ugly hacks can be fixed to
just use a caller-provided buffer, there's not that much left.

timestamptz_to_str() is probably the most complex case. I just noticed
there's already a second implementation in
ecpg/pgtypeslib/dt_common.c. Yuck. It seems to already have diverged in
a number of cases :(

> * 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...

I personally would have no problem with that. It's probably either going
to be used during in-depth-debugging or when developing pg. In both
cases english seems to be fine. But I seldomly use translated programs
so maybe I am not the right person to ask.

> * 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.

contrib would be fine, but I think src/bin is better. There have been
quite some bugs by now where it would have been useful to have a
reliable xlogdump in core so its really installed.

Greetings,

Andres Freund

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