Re: Patch: add --if-exists to pg_recvlogical

On Tue, Sep 19, 2017 at 1:12 PM, Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:

> On 9/17/17 18:21, Rosser Schwarz wrote:
> > On Fri, Sep 1, 2017 at 10:22 AM, Peter Eisentraut
> > <peter(dot)eisentraut(at)2ndquadrant(dot)com
> > <mailto:peter(dot)eisentraut(at)2ndquadrant(dot)com>> wrote:
> >> I understand the --drop-slot part. But I don't understand what it means
> >> to ignore a missing replication slot when running --start
>

> > I'm not sure I do either, honestly. I followed the Principle of Least
> > Surprise in making it a no-op when those switches are used and the slot
> > doesn't exist, over "no one will ever do that". Because someone will.
>

> > I'm happy to hear suggestions on what to do in that case beyond exit
> > cleanly.
>

> Nonsensical option combinations should result in an error.
>

The more I think about it, I don't think it's nonsensical, though.
--create-slot --if-exists or --drop-slot --if-not-exists, obviously. I
mean, do you even logic?

Those aside, --if-exists just means a non-existent slot isn't an error
condition, doesn't it? --start --if-exists will start, if the slot exists.
Otherwise it won't, in neither case raising an error. Exactly what it says
on the tin. Perhaps the docs could make clear that combination implies
--no-loop (or at least means we'll exit immediately) if the slot does not,
in fact, exist?

Because I started work on this patch in the context of doing some scripting
around pg_recvlogical, I guess I had some vague notion that someone might
have logic in their own scripts where that state was possible (and, of
course, appropriately handled). The program exits either way: one assumes
the operator meant to do that; the other says they were wrong to have done
so.

I'm having trouble seeing a combination of options that does what it prima
facie implies as an error state. If there's broader consensus that's how it
should behave, I'll happily defer, though.

To that end, could I solicit some input from the broader audience?

It appears that you have removed the interaction of --start and
> --if-exists in your last patch, but the documentation patch still
> mentions it. Which is correct?
>

Pardon? I've had a bit on my plate recently, so it's entirely possible, if
not likely, that I missed something, but isn't that this block?

@@ -267,6 +271,17 @@ StreamLogicalLog(void)
res = PQexec(conn, query->data);
if (PQresultStatus(res) != PGRES_COPY_BOTH)
{
+ const char* sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
+
+ if (slot_not_exists_ok &&
+ sqlstate &&
+ strcmp(sqlstate, ERRCODE_UNKNOWN_OBJECT) == 0)
+ {
+ destroyPQExpBuffer(query);
+ PQclear(res);
+ disconnect_and_exit(0);
+ }
+
fprintf(stderr, _("%s: could not send replication command \"%s\": %s"),
progname, query->data, PQresultErrorMessage(res));
PQclear(res);

--
:wq