Re: [PATCH 02/14] Add support for a generic wal reading facility dubbed XLogReader - Mailing list pgsql-hackers

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