Re: logical changeset generation v6.2

On Mon, Sep 30, 2013 at 6:44 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> The series from friday was a bit too buggy - obviously I was too
> tired. So here's a new one:
>
> * fix pg_recvlogical makefile (Thanks Steve)
> * fix two commits not compiling properly without later changes (Thanks Kevin)
> * keep track of commit timestamps
> * fix bugs with option passing in test_logical_decoding
> * actually parse option values in test_decoding instead of just using the
> option name
> * don't use anonymous structs in unions. That's compiler specific (msvc
> and gcc) before C11 on which we can't rely. That unfortunately will
> break output plugins because ReorderBufferChange need to qualify
> old/new tuples now
> * improve error handling/cleanup in test_logical_decoding
> * some minor cleanups
>
> Patches attached, git tree updated.

I spent some time looking at the sample plugin (patch 9/12). Here are
some review comments:

- I think that the decoding plugin interface should work more like the
foreign data wrapper interface. Instead of using pg_dlsym to look up
fixed names, I think there should be a struct of function pointers
that gets filled in and registered somehow.

- pg_decode_init() only warns when it encounters an unknown option.
An error seems more appropriate.

- Still wondering how we'll use this from a bgworker.

- The output format doesn't look very machine-parseable. I really
think we ought to provide something that is. Maybe a CSV-like format,
or maybe something else, but I don't see why someone who wants to do
change logging should be forced to write and install C code. If
something like Bucardo can run on an unmodified system and extract
change-sets this way without needing a .so file, that's going to be a
huge win for usability.

Other than that, I don't have too many concerns about the plugin
interface. I think it provides useful flexibility and it generally
seems well-designed. I hope in the future we'll be able to decode
transactions on the fly instead of waiting until commit time, but I've
resigned myself to the fact that we may not get that in version one.

More generally on this patch set, if I'm going to be committing any of
this, I'd prefer to start with what is currently patches 3 and 4, once
we reach agreement on those.

Are we hoping to get any of this committed for this CF? If so, let's
make a plan to get that done; time is short. If not, let's update the
CF app accordingly.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company