Обсуждение: exactly what is COPY BOTH mode supposed to do in case of an error?
It seems the backend and libpq don't agree. The backend makes no
special provision to wait for a CopyDone message if an error occurs
during copy-both. It simply sends an ErrorResponse and that's it.
libpq, on the other hand, treats either CopyDone or ErrorResponse as a
cue to transition to PGASYNC_COPY_IN (see pqGetCopyData3).
And that's a problem, because in PGASYNC_COPY_IN mode, libpq is
unwilling to process the ErrorResponse. getCopyDataMessage doesn't do
anything with it, and if the user calls PQgetResult(), pqParseInput3()
won't process it either, because it's only willing to do that when the
status is PGASYNC_BUSY. So the bottom line is that after the server
throws an error, the server gets back to thinking that the connection
is idle, while the client ends up thinking that we're still in copy-in
mode. The client can try to recover by doing PQputCopyEnd(), which
will get libpq back to an idle state, but if the extended-query
protocol is in use, the Sync it sends will cause the server to send an
extra ReadyForQuery that libpq isn't expecting. Hilarity ensues.
It's perhaps not coincidental that "48.2.5 COPY operations" is silent
about what is supposed to happen when an error occurs in copy-both
mode, though it does talk about both copy-in and copy-out. On
copy-in, it says:
In the event of a backend-detected error during copy-in mode
(including receipt of a CopyFail message), the backend will issue an
ErrorResponse message. If the COPY command was issued via an
extended-query message, the backend will now discard frontend messages
until a Sync message is received, then it will issue ReadyForQuery and
return to normal processing.
And regarding copy-out, it says:
In the event of a backend-detected error during copy-out mode, the
backend will issue an ErrorResponse message and revert to normal
processing. The frontend should treat receipt of ErrorResponse as
terminating the copy-out mode.
There's no corresponding statement about error-handling in the
copy-both case. However, since the apparent intent is that an error
message from the server trumps anything the client may have had in
mind, it seems reasonable to decide that an ErrorResponse is intended
to fully terminate copy-both mode (not just switch to copy-in) and
initiate a skip-until-sync. That's what the server actually does, but
libpq has other ideas.
One way to see the practical effect of this is to set up a streaming
replication slave but modify the backup label to reference some future
WAL location not yet written (I just changed the "0" before the slash
to a "1"). This will cause the server to throw the following error:
ereport(ERROR,
(errmsg("requested starting
point %X/%X is ahead of the WAL flush position of this server %X/%X",
(uint32)
(cmd->startpoint >> 32),
(uint32)
(cmd->startpoint),
(uint32)
(FlushPtr >> 32),
(uint32) (FlushPtr))));
Doing this will cause the PQgetCopyData in libpqrcv_receive to return
-1, so you might expected that slave would get the following error:
ereport(ERROR,
(errmsg("could not receive
data from WAL stream: %s",
PQerrorMessage(streamConn))));
But you don't, because PQgetResult() returns PGRES_COPY_IN. So the
slave thinks that the master made a *normal* termination of streaming
replication, due to a timeline change, and it prints this:
LOG: replication terminated by primary server
DETAIL: End of WAL reached on timeline 1 at 0/0
It then calls PQputCopyEnd to end a copy that the server thinks is no
longer in progress and then invokes PQgetResult again. And now it
gets the error message:
FATAL: error reading result of streaming command: ERROR: requested
starting point 1/5000000 is ahead of the WAL flush position of this
server 0/6000348
This doesn't in practice matter very much, because either way, we're
going to slam shut the server connection at this point. But it's
clearly messed up - the error is actually NOT in response to the
CopyDone we sent, but rather in response to the START_STREAMING
command that preceded it. libpq, however, refuses to receive the error
message until after the unnecessary CopyDone has been sent.
I believe that the only other place where this coding pattern arises
is in receivelog.c. That code has actually adopted the opposite
convention from the backend: it thinks it needs to send CopyDone
whether or not an error has occurred. This turns out not to matter
particularly, because the server is just going to try to close the
connection anyway. But if it did anything else after ending the copy,
I suspect the wheels would come off.
I'm attaching a patch which adopts the position that the backend is
right and libpq is wrong. The opposite approach is also possible, but
I haven't tried to implement it. Or maybe there's a third way which
is better still.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Вложения
On 27 April 2013 03:22, Robert Haas <robertmhaas@gmail.com> wrote: > It seems the backend and libpq don't agree. The backend makes no > special provision to wait for a CopyDone message if an error occurs > during copy-both. It simply sends an ErrorResponse and that's it. > libpq, on the other hand, treats either CopyDone or ErrorResponse as a > cue to transition to PGASYNC_COPY_IN (see pqGetCopyData3). Well spotted, and good detective work. > I'm attaching a patch which adopts the position that the backend is > right and libpq is wrong. The opposite approach is also possible, but > I haven't tried to implement it. Or maybe there's a third way which > is better still. I guess if we assume this only affects replication we could change it at either end, not sure about that. libpq updates are much harder to roll out, so it would be better to assume that it is correct and the backend is wrong if we want to backpatch the fix. Not sure if that is a lot of work? --Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
> libpq updates are much harder to roll out, so it would be better to
> assume that it is correct and the backend is wrong if we want to
> backpatch the fix.
I don't think that's a particularly sound argument.
If we view this as a protocol definitional issue, which it is, then
changing the backend means that we're changing the protocol and thus
that the changes might impact other clients besides libpq. Now,
it's quite possible that libpq is the only client-side code that's
supporting COPY BOTH mode at all ... but we don't know that do we?
Also, I think that expecting the backend to do something else
after throwing an error is probably doomed to failure --- consider
the case where what it's sending is a FATAL ereport about a SIGTERM
interrupt, for example. It's not going to be hanging around to
clean up any bidirectional copies.
So I'm inclined to think that Robert's patch is headed in the right
direction, though I've not tried to review the changes in detail.
regards, tom lane
On Sat, Apr 27, 2013 at 6:02 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 27 April 2013 03:22, Robert Haas <robertmhaas@gmail.com> wrote: >> It seems the backend and libpq don't agree. The backend makes no >> special provision to wait for a CopyDone message if an error occurs >> during copy-both. It simply sends an ErrorResponse and that's it. >> libpq, on the other hand, treats either CopyDone or ErrorResponse as a >> cue to transition to PGASYNC_COPY_IN (see pqGetCopyData3). > > Well spotted, and good detective work. Thanks. >> I'm attaching a patch which adopts the position that the backend is >> right and libpq is wrong. The opposite approach is also possible, but >> I haven't tried to implement it. Or maybe there's a third way which >> is better still. > > I guess if we assume this only affects replication we could change it > at either end, not sure about that. > > libpq updates are much harder to roll out, so it would be better to > assume that it is correct and the backend is wrong if we want to > backpatch the fix. > > Not sure if that is a lot of work? My feeling is that it would be better not to back-patch this, but just fix it in master. Given the present uses of COPY-BOTH mode, the problems seem to be limited to bad error messages, so it's arguably not a critical bug fix. Also, I think that no matter which way we fix it, people who upgrade the master to a new point release, but not pg_receivexlog, would in some unlikely cases actually experience a regression in the quality of error messages. I would say we have to live with that if the consequences were any worse than bad error messages in the first place, but as far as I can tell they're not. If someone can contrive a scenario where this causes outright breakage, that would tip the balance for me, but I don't at present see such a hazard. On a practical level, the main thing I didn't like about trying to fix the server was the same issue that Tom mentioned: we'd need code in the server to track whether COPY-BOTH mode is active and skip client messages until we hit a CopyDone or CopyFail message. And I suspect that code would be somewhat fragile, because having sent an ErrorResponse already, we'd have no straightforward way to report a further error - we'd need to report follow-on errors via NOTICE or FATAL. Now this is not a disaster, but it's not great, either, because there's a lot of code (including, notably, palloc) which assumes that it can throw an ERROR whenever it likes. And in this case, it couldn't. The second thing I didn't like about that approach was that it would make COPY-BOTH quite asymmetrical with both COPY-OUT and COPY-IN. That didn't seem like a great idea, either. A further point is that the problems in the back branches are less serious anyway, because the timeline-switching code is the only thing that ever tries to exit COPY-BOTH mode without closing the connection, and that's new in 9.3. So for all those reasons, my vote is for a client-side, master-only fix. ...Robert
On 27 April 2013 20:12, Robert Haas <robertmhaas@gmail.com> wrote: > My feeling is that it would be better not to back-patch this, but just > fix it in master. Given the present uses of COPY-BOTH mode, the > problems seem to be limited to bad error messages, so it's arguably > not a critical bug fix. Also, I think that no matter which way we fix > it, people who upgrade the master to a new point release, but not > pg_receivexlog, would in some unlikely cases actually experience a > regression in the quality of error messages. I would say we have to > live with that if the consequences were any worse than bad error > messages in the first place, but as far as I can tell they're not. If > someone can contrive a scenario where this causes outright breakage, > that would tip the balance for me, but I don't at present see such a > hazard. > > On a practical level, the main thing I didn't like about trying to fix > the server was the same issue that Tom mentioned: we'd need code in > the server to track whether COPY-BOTH mode is active and skip client > messages until we hit a CopyDone or CopyFail message. And I suspect > that code would be somewhat fragile, because having sent an > ErrorResponse already, we'd have no straightforward way to report a > further error - we'd need to report follow-on errors via NOTICE or > FATAL. Now this is not a disaster, but it's not great, either, > because there's a lot of code (including, notably, palloc) which > assumes that it can throw an ERROR whenever it likes. And in this > case, it couldn't. > > The second thing I didn't like about that approach was that it would > make COPY-BOTH quite asymmetrical with both COPY-OUT and COPY-IN. > That didn't seem like a great idea, either. > > A further point is that the problems in the back branches are less > serious anyway, because the timeline-switching code is the only thing > that ever tries to exit COPY-BOTH mode without closing the connection, > and that's new in 9.3. > > So for all those reasons, my vote is for a client-side, master-only fix. +1, very sound --Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services