Re: [HACKERS] exactly what is COPY BOTH mode supposed to do in case of an error?
On 27 April 2013 20:12, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] exactly what is COPY BOTH mode supposed to do in case of an error?
On Sat, Apr 27, 2013 at 6:02 AM, Simon Riggs wrote: > On 27 April 2013 03:22, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] exactly what is COPY BOTH mode supposed to do in case of an error?
Simon Riggs 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] exactly what is COPY BOTH mode supposed to do in case of an error?
On 27 April 2013 03:22, Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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/500 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