On 2017-06-03 00:55:22 +0200, Petr Jelinek wrote:
> On 02/06/17 23:45, Andres Freund wrote:
> > Hi Petr,
> >
> > On 2017-06-02 22:57:37 +0200, Petr Jelinek wrote:
> >> On 02/06/17 20:51, Andres Freund wrote:
> >>> I don't understand why the new block is there, nor does the commit
> >>> message explain it.
> >>>
> >>
> >> Hmm, that particular change can actually be reverted. It was needed for
> >> one those custom replication commands which were replaced by normal
> >> query support. I have missed it during the rewrite.
> >
> > Doesn't appear to be quite that simple, I get regression test failures
> > in that case.
> >
>
> Hmm, looks like we still use it for normal COPY handling. So basically
> the problem is that if we run COPY TO STDOUT and then consume it using
> the libpqrcv_receive it will end with normal PGRES_COMMAND_OK but we
> need to call PQgetResult() in that case otherwise libpq thinks the
> command is still active and any following command will fail, but if we
> call PQgetResult on dead connection we get that error you complained about.
Should this possibly handled at the caller level? This is a bit too
much magic for my taste.
Looking at the callers, the new code isn't super-obvious either:
len = walrcv_receive(wrconn, &buf, &fd);
if (len != 0) { /* Process the data */ for (;;) {
CHECK_FOR_INTERRUPTS();
if (len == 0) { break; } else if (len < 0)
{ ereport(LOG, (errmsg("data stream from publisher has
ended"))); endofstream = true; break; }
The len < 0, hidden inside a len != 0, which in the loop again chcks if
len == 0 (because it's decremented in loop)? And there's two different[5~ len = walrcv_receive(wrconn, &buf, &fd);
statements?
> I guess it would make sense to do conditional exit on
> (PQstatus(streamConn) == CONNECTION_BAD) like libpqrcv_PQexec does. It's
> quite ugly code-wise though.
I think that's fine for now. It'd imo be a good idea to improve matters
here a bit, but for now I've just applied your patch.
- Andres