Re: pg_receivexlog and replication slots

On Mon, Sep 8, 2014 at 8:50 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Sep 3, 2014 at 11:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Aug 31, 2014 at 9:45 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> 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.
>>
>> I have no opinion on whether we want these particular Assert() calls,
>> but I note that using Assert() in front-end code only became possible
>> in February of 2013, as a result of commit
>> e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9. So the lack of assertions
>> there may not be so much because people thought it was a bad idea as
>> that it didn't use to work. Generally, I favor the use of Assert() in
>> front-end code in the same scenarios in which we use it in back-end
>> code: for checks that shouldn't burden production builds but are
>> useful during development.
>
> Well that was exactly why they have been added first. The assertions
> have been placed in some functions to check for incompatible
> combinations of argument values when those functions are called. I
> don't mind re-adding them if people agree that they make sense. IMO
> they do and they help as well for the development of future utilities
> using replication protocol in src/bin/pg_basebackup as much as the
> refactoring work done on this thread.
Let's keep them then.

>>> 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.
Re-changed back to that.

>> 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.
This seems the cleaner approach, do did so.

New patches taking into account all those comments are attached.
Thanks for the feedback.
Regards,
--
Michael