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

Hi Heikki,

On Monday, September 17, 2012 09:40:17 AM Heikki Linnakangas wrote:
> On 15.09.2012 03:39, Andres Freund wrote:
> > Features:
> > - streaming reading/writing
> > - filtering
> > - reassembly of records
> >
> > Reusing the ReadRecord infrastructure in situations where the code that
> > wants to do so is not tightly integrated into xlog.c is rather hard and
> > would require changes to rather integral parts of the recovery code
> > which doesn't seem to be a good idea.
>
> My previous objections to this approach still apply. 1. I don't want to
> maintain a second copy of the code to read xlog.
Yes. I aggree. And I am willing to provide an implementation of this if should
my xlogreader variant gets a bit more buyin.

> 2. We should focus on reading WAL, I don't see the point of mixing WAL
writing into this.
If you write something that filters/analyzes and then forwards WAL and you want
to do that without a big overhead (i.e. completely reassembling everything, and
then deassembling it again for writeout) its hard to do that without
integrating both sides.

Also, I want to read records incrementally/partially just as data comes in
which again is hard to combine with writing out the data again.

> 3. I don't like the callback-style API.
I tried to accomodate to that by providing:

extern XLogRecordBuffer* XLogReaderReadOne(XLogReaderState* state);

which does exactly that.

> I came up with the attached. I moved ReadRecord and some supporting
> functions from xlog.c to xlogreader.c, and made it operate on
> XLogReaderState instead of global global variables. As discussed before,
> I didn't like the callback-style API, I think the consumer of the API
> should rather just call ReadRecord repeatedly to get each record. So
> that's what I did.
The problem with that is that kind of API is that it, at least as far as I can
see, that it never can operate on incomplete/partial input. Your need to buffer
larger amounts of xlog somewhere and you need to be aware of record boundaries.
Both are things I dislike in a more generic user than xlog.c.

> There is still one callback, XLogPageRead(), to obtain a given page in
> WAL. The XLogReader facility is responsible for decoding the WAL into
> records, but the user of the facility is responsible for supplying the
> physical bytes, via the callback.
Makes sense.

> So the usage is like this:
>
> /*
> * Callback to read the page starting at 'RecPtr' into *readBuf. It's
> * up to you to do this any way you like. Typically you'd read from a
> * file. The WAL recovery implementation of this in xlog.c is more
> * complicated. It checks the archive, waits for streaming replication
> * etc.
> */
> static bool
> MyXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr RecPtr, char
> *readBuf, void *private_data)
> {
> ...
> }
>
>
> state = XLogReaderAllocate(&MyXLogPageRead);
>
> while ((record = XLogReadRecord(state, ...)))
> {
> /* do something with the record */
> }
If you don't want the capability to forward/filter the data and read partial
data without regard for record constraints/buffering your patch seems to be
quite a good start. It misses xlogreader.h though...

Do my aims make any sense to you?

Greetings,

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