Re: pg_receivexlog and replication slots

On Tue, Aug 26, 2014 at 9:43 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Aug 19, 2014 at 2:49 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Mon, Aug 18, 2014 at 4:01 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Mon, Aug 18, 2014 at 3:48 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> On Mon, Aug 18, 2014 at 2:38 PM, Michael Paquier
>>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> And now looking at your patch there is additional refactoring possible
>>> with IDENTIFY_SYSTEM and pg_basebackup as well...
>> And attached is a rebased patch doing so.
> I reworked this patch a bit to take into account the fact that the
> number of columns to check after running IDENTIFY_SYSTEM is not always
> 4 as pointed out by Andres:
> - pg_basebackup => 3
> - pg_receivexlog => 3
> - pg_recvlogical => 4

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)

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? 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"? And we might want to include a hint about the reason
(wrong version)?

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?

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?

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/