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