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(at)anarazel(dot)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