Re: pg_receivexlog and replication slots

On 2014-09-01 20:58:29 +0900, Michael Paquier wrote:
> On Sun, Aug 31, 2014 at 10:45 PM, Magnus Hagander <magnus(at)hagander(dot)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).

I don't really see a need to that for slot specific code. The locations
where we more lenient are ones where < 9.4 is ok, or a better message is
following shortly afterwards.

> > 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.

The reason for the formulation of the current error message is that it's
the same across all callsites emitting it to make it easier for
translators. It used to be more specific at some point and then was
changed. Since these aren't expected to be hit much I don't really see a
need to be very detailed.

> > 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.

Agreed.

> > 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.

Why not return all four columns from RunIdentifySystem(), setting plugin
to NULL if not available. Then the caller can error out.

Greetings,

Andres Freund