Re: BUG #16035: STATEMENT_TIMEOUT not working when we have single quote usage inside CTE which is used in inner sql

Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
>>> tl;dr: I do not think this is buggy in v10. But arguably there's
>>> a bug in later branches, and they need to go back to behaving
>>> like v10.

>> I understand the original reporter's complain. Also I understand Tom's
>> complain to v11's behavior. I will look into the v11 (and above) code.

> I admit v11's current behavior is inconstant, but I am not sue going
> to back V10's behavior is a good idea.
> With attached patch (against master), SET STATEMENT_TIMEOUT
> immediately affects to subsequent commands in the multi statement. I
> think this is not only more intuitive than v10's behavior but it meets
> the original reporter's expectation.

Hm. So, okay, that is a nicer API probably, but note that it also
has the effect that the timeout starts over again for each statement
in the string, while before it applied to the string as a whole.
Are we okay with that change? (I've not yet looked to see if it's
documented anywhere that it works that way...) It's kind of tossing
some of the optimization intended by f8e5f156b overboard, since when
a timeout is active we'll be doing timeout calculations for each
statement in the string.

Anyway, granting that we want the definitional change, I still
don't like anything about this patch. The proposed test

+ if (!doing_extended_query_message ||
+ (!stmt_timeout_active && doing_extended_query_message))

is unreadably complicated and repetitive, and it's undocumented,
and it makes timeout handling work quite differently in the simple
and extended query paths, and it will cause excess timeout calculations
(over and above the newly-necessary ones) because it will force a new
timeout calculation for each start_xact_command call in the simple query
path (yes, we do more than one of those per statement in common cases).
Plus it makes the semantics of stmt_timeout_active rather unclear.

I think if we want to do this, the way to do it is to add another
disable_statement_timeout call while finishing up a non-last query
in exec_simple_query, as per 0001 below. That adds no extra overhead
unless we have a multi-statement string, and it is much more parallel
to the way things are done for extended protocol.

However ... as I was looking at this, I realized that I didn't like
anything about commit f8e5f156b either. It introduces private state
in postgres.c that has to be kept in sync with state in timeout.c.
We already found one bug in that (cf be42015fc), and I have little
faith that there aren't others, and no faith at all that we won't
introduce more later. The right way to manage that, I think, is to
extend timeout.c to allow inquiring whether the timeout is active,
as per 0002 below, so that timeout.c's state is the single source of
truth about this. Maintaining the extra state needed to make this
cheap allows some other small simplifications/speedups, too, mainly
that we can skip useless searches of the active_timeouts[] array.

Not sure what I think about back-patching this. We probably can't
back-patch 0001; that's enough of a behavioral change that the cure
seems worse than the disease. I'm tempted though to argue that we
should back-patch 0002, on the grounds that it prevents possible
bugs and should save a few cycles too.

regards, tom lane