Обсуждение: Let's get rid of premature execution

Hi,

Under some circumstances, the driver currently does so-called "premature
execution". This means that when the application does something like this:

SQLPrepare("SELECT function()");
SQLNumResultCols();

but does not run SQLExecute(), the driver executes the statement anyway.
This is documented, and there's an option to disable this
(DisallowPremature=1), and it only happens with UseServerSidePrepare=0,
and only with statements starting with SELECT, but IMHO it's
nevertheless scary as hell.

Currently, with DisallowPremature=1, the driver simply does not return
any information for calls like SQLNumResultCols() that are called before
SQLExecute().

I propose changes to this:

1. The driver should never do premature execution. That's just evil.
Remove DisallowPremature option.

2. When SQLNumResultCols() is called before SQLExecute(), the driver
will parse and describe the query in the server, even if
UseServerSidePrepare=0. This does not affect how the query will be
executed later. In the query that is sent to the server, the parameter
markers are replaced with NULLs, similar to how they are replaced with
the real values later when the query is executed. (This code exists
already; we did the same thing in DisallowPremature mode when we sent
the "dummy cursor" query, e.g. "BEGIN; DECLARE foo CURSOR FOR <real
query>; FETCH 1 backwards;").

Patch attached. This simplifies the code a little bit, removes the scary
premature execution, and allows us to always return column information
before SQLExecute().

The Parse + Describe didn't work before protocol version 3, but we
already decided that we already dropped support for version 2 in commit
341a6fcc.

- Heikki

On Mon, Jan 26, 2015 at 5:36 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> Under some circumstances, the driver currently does so-called "premature
> execution". This means that when the application does something like this:
>
> SQLPrepare("SELECT function()");
> SQLNumResultCols();
>
> but does not run SQLExecute(), the driver executes the statement anyway.
> This is documented, and there's an option to disable this
> (DisallowPremature=1), and it only happens with UseServerSidePrepare=0,
> and only with statements starting with SELECT, but IMHO it's nevertheless
> scary as hell.

Just by looking at the commit history this has been introduced by
2336af8 of 2001, followed a couple of years later by that, this
feature is indeed not the best reassuring thing ever:
commit: c7fe7bd2cbf01edd47f95ea452a87db0546ba9dd
author: Ludek Finstrle <luf@pzkagis.cz>
date: Tue, 31 Jan 2006 09:53:02 +0000
quick hack against access violation with Disallow premature - the
feature is still broken

> Currently, with DisallowPremature=1, the driver simply does not return any
> information for calls like SQLNumResultCols() that are called before
> SQLExecute().
>
> I propose changes to this:
>
> 1. The driver should never do premature execution. That's just evil. Remove
> DisallowPremature option.
OK with that.

> 2. When SQLNumResultCols() is called before SQLExecute(), the driver will
> parse and describe the query in the server, even if UseServerSidePrepare=0.
> This does not affect how the query will be executed later. In the query that
> is sent to the server, the parameter markers are replaced with NULLs,
> similar to how they are replaced with the real values later when the query
> is executed.
OK. This is in line with ODBC that specifies that for example calls to
SQLDescribeCol can be issues before SQLExecute:
https://msdn.microsoft.com/en-us/library/ms716289%28v=vs.85%29.aspx
IMO, this behavior should be properly documented in the section of
UseServerSidePrepare, aka there is one case where server-side prepare
is used with NULL values even if this parameter is disabled.

> The Parse + Describe didn't work before protocol version 3, but we already
> decided that we already dropped support for version 2 in commit 341a6fcc.
This argument alone sounds sufficient to remove this option, looking
at the docs this option has been introduced to compensate the lack of
support of PREPARE on the server side which has been added in 7.3 (?)
in Postgres.

The header of this file is incorrect, DisallowPremature does not exist
anymore with your patch and this test is made to test if premature
calls of SQLNumResultCols does not influence prepared queries:
--- /dev/null
+++ b/test/src/premature-test.c
@@ -0,0 +1,201 @@
+/*
+ * This test case tests the so-called "premature execution" behaviour, which
+ * can be influenced with the DisallowPremature setting.
+ */
Regression tests passed with your patch btw.
Regards,
--
Michael

On 01/29/2015 09:36 AM, Michael Paquier wrote:
> IMO, this behavior should be properly documented in the section of
> UseServerSidePrepare, aka there is one case where server-side prepare
> is used with NULL values even if this parameter is disabled.

Added a notice about that. Not sure if it's makes much sense to a user
who's not familiar with the FE/BE protoocl, but anyway..

> The header of this file is incorrect, DisallowPremature does not exist
> anymore with your patch and this test is made to test if premature
> calls of SQLNumResultCols does not influence prepared queries:
> --- /dev/null
> +++ b/test/src/premature-test.c
> @@ -0,0 +1,201 @@
> +/*
> + * This test case tests the so-called "premature execution" behaviour, which
> + * can be influenced with the DisallowPremature setting.
> + */

Fixed that. I also refactored and expanded the test case a bit, and
fixed another bug that I bumped into.

Committed with those fixes. Thanks for the review!

- Heikki