Confused about "set statement timeout"

[mjgarton]

Our tests for timeouts check "set statement_timeout" using queries of the form: set statement_timeout '5000ms' and obviously they currently pass.

However, I don't think this functionality is working correctly. The tests pass because they pass a &str directly to the function being tested. In the real application, it comes from a Statement that has been converted to a string.

I don't see how set statement_timeout '5000ms' can come from a Statement because it won't parse into a Statement initially. I tried it manually on datafusion-postgres-cli and got:

postgres=> set statement_timeout  '5000ms';
ERROR:  sql parser error: Expected: equals sign or TO, found: '5000ms' at Line: 1, Column: 24

Also, against a real postgresql instance this is an error too:

postgres=# set statement_timeout  '5000ms';
ERROR:  syntax error at or near "'5000ms'"
LINE 1: set statement_timeout  '5000ms';
                               ^

Also, looking at the postgresql documentation for SET, it's clear that the format should either include TO or = in the statement:

SET [ SESSION | LOCAL ] configuration_parameter { TO | = } { value | 'value' | DEFAULT }

I propose we:

• Change the test to use standard postgresql syntax (e.g., set statement_timeout to '5000ms')
• Update try_respond_set_statements to take &Statement instead of &str
• Update the timeout parsing logic to assume this syntax.

If we agree, I will make these changes.

[sunng87]

Sounds good to me! Please make the changes.

[mjgarton]

See #211

I might take a look at similar for SHOW next.

[mjgarton]

I did the same with SHOW handling in #213

If you are happy with that too @sunng87 , we can close this issue now.

[sunng87]

Fixed in #211 and #213