Re: Always show correct error message for statement timeouts, fixes random buildfarm failures

Hi,

I've encountered a related issue which is fixed by Jelte's patch. When
a stmt timeout is triggered between the last sent row and the call
disable_statement_timeout:
- the stmt timeout indicator is set
- disable_statement_timeout does nothing as the stmt timeout is now inactive
- CommandComplete is sent
- the next CHECK_FOR_INTERRUPTS process the fired timeout, sending a
stmt timeout error
So the client receives a CommandComplete, followed by an error for the
same command.

+ if (DoingCommandRead)
+ {
+ /*
+ * If we are reading a command from the client, just ignore the
+ * cancel request --- sending an extra error message won't
+ * accomplish anything. Otherwise, go ahead and throw an error
+ * with the correct reason.
+ */
+ }
+ else if (lock_timeout_occurred)

The comment feels a bit misleading. This isn't limited to cancel
requests: all timeouts will be ignored and discarded when reading a
command from the client. Which is the desired behaviour from what I
understand: The previous command was finished and if we have timeouts
triggered, they happened in the timeframe before reading the new
command.

- * Disable statement timeout, if active.
+ * Disable statement timeout, if active. We preserve the indicator flag
+ * though, otherwise we'd lose the knowledge in ProcessInterupts that the
+ * SIGINT came from a statement timeout.
*/
static void
disable_statement_timeout(void)
{
if (get_timeout_active(STATEMENT_TIMEOUT))
- disable_timeout(STATEMENT_TIMEOUT, false);
+ disable_timeout(STATEMENT_TIMEOUT, true);

Is it still possible to have both the statement timeout active and
fired? You added the change to enable_statement_timeout to avoid
restarting it when it was already triggered, so I would expect an
active timeout to not have the indicator flag set. I guess there's the
possible race where the timeout is triggered between
get_timeout_active and disable_timeout.

Regards,
Anthonin Bonnefoy