On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
> As this is a number of patches rolled into one - do you happen to keep
> them separate in your local repo? If so can you send them as separate
> ones (refactor identify_system should be quite unrelated to supporting
> replication slots, right?), for easier review? (if not, I'll just
> split them apart mentally, but it's easier to review separately)
Thanks for your review!
OK, here are 2 patches, the 2nd needing the 1st one:
1) Refactor IDENTIFY_SYSTEM and replslot create/drop APIs
2) Support for --create and --drop in pg_receivexlog
> On the identify_system part - my understanding of the code is that
> what you pass in as num_cols is the number of columns required for it
> to work, right?
The argument is I would say cross-version compatibility and
consistency with the existing 9.4 code, but... (see below for the rest
of the story).
> We probably need to adjust the error message as well
> in that case, because it's no longer what's "expected", it's what's
> "required"?
OK, changed this way.
> And we might want to include a hint about the reason (wrong version)?
I am not sure about that, a simple error message looks fine IMO, and
there is no notion of error hinting in the other client utilities as
well.
> There's also a note "get LSN start position if necessary", but it
> tries to do it unconditionally. What is the "if necessary" supposed to
> refer to?
That's remnant of some old code, so I removed it. Thanks for spotting that.
> Actually - why do we even care about the 3 vs 4 in RunIdentifySystem,
> as it never actually looks at the 4th column anyway? If we do
> specifically want it to fail in the case of pg_recvlogical, we really
> need to think up a better error message for it, and perhaps a
> different way of specifying it?
Hm. I'd vote to simplify the code a bit based on the argument that the
current API only looks at the 3 first columns and does not care about
the 4th which is the plugin name.
> Do we really want those Asserts? There is not a single Assert in
> bin/pg_basebackup today - as is the case for most things in bin/. We
> typically use regular if statements for things that "can happen", and
> just ignore the others I think - since the callers are fairly simple
> to trace.
OK, removed.
Regards,
--
Michael