Re: [PATCH] V3: Idle in transaction cancellation

On Wednesday 15 December 2010 20:12:45 Robert Haas wrote:
> On Wed, Dec 15, 2010 at 10:02 AM, Andres Freund <andres@anarazel.de> wrote:
> >> Is there a way that errstart() and/or errfinish() can know enough
> >> about the state of the communication with the frontend to decide
> >> whether to suppress edata->output_to_client?  In other words, instead
> >> of explicitly passing in a flag that says whether to inform the
> >> client, it would be better for the error-reporting machinery to
> >> intrinsically know whether it's right to send_message_to_frontend().
> >> Otherwise, an error thrown from an unexpected location might not have
> >> the flag set correctly.
> > 
> > You could use "DoingCommandRead" to solve that specific use-case, but the
> > COMERROR ones I don't see as being replaced that easily.
> 
> Well, again, I'm not an expert on this, but why would we need to unify
> the two mechanisms?  Asynchronous rollbacks (what we're trying to do
> here) and protocol violations (which is what COMMERROR looks to be
> used for) are really sort of different.
I think its not only protocol violations. Data-Leakage during auth are also 
handled with it I think.

> I'm not really sure we need to handle them in the same way.  Let's think
> about a recovery conflict where ProcessInterrupts() has been called.  Right
> now, if that situation occurs and we are not DoingCommandRead, then we just
> throw an error.  That's either safe, or an already-existing bug.
That should be safe.

> So the question is what to do if we ARE DoingCommandRead.  Right now, we
> throw a fatal error.  There's no comment explaining why, but I'm
> guessing that the reason is the same problem we're trying to fix here:
> the protocol state gets confused - but if we throw a FATAL then the
> client goes away and we don't have to worry about it any more.  Our
> goal here, as I understand it, is to handle that case without a FATAL.
Yes, thats it.

> So let's see... if we're DoingCommandRead at that point, and
> whereToSendOutput == DestRemote then we set whereToSendOutput =
> DestNone before throwing the error, and restore it just after we reset
> DoingCommandRead?  <stabs blindly at target>
That won't be enough unfortunately. A transaction-aborted error will get re-
raised before the client is ready to re-accept it. Thats what the 
silent_error_while_idle flag is needed for.
Also I think such a simple implementation would cause problems with single 
user mode. Now that could get fixed by saving and resetting the old mode - that 
might have exception handling problems in turn (COMERRORS during an ssl 
connection for example) because it will leave the section without having reset 
whereToSendOutput. Sprinkling PG_TRY everywhere hardly seems simpler...

Andres