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

From: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH 02/14] Add support for a generic wal reading facility dubbed XLogReader
Date: 2012-12-11 13:55:35
Message-ID: 50C73B57.1050603@vmware.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Heikki Linnakangas 2012-12-11 13:56:38 Re: [PATCH 02/14] Add support for a generic wal reading facility dubbed XLogReader
Previous Message Daniele Varrazzo 2012-12-11 13:15:27 Re: allowing multiple PQclear() calls