Re: [PATCHES] prepareable statements

nconway(at)klamath(dot)dyndns(dot)org (Neil Conway) writes:
> The attached patch implements per-backend prepareable statements.

Finally some feedback:

> The syntax is:
> PREPARE name_of_stmt(param_types) FROM <some query>;
> EXECUTE name_of_stmt [INTO relation] [USING args];
> DEALLOCATE [PREPARE] name_of_stmt;

> I don't really like the 'FROM' keyword in PREPARE (I was planning to
> use 'AS'), but that's what SQL92 specifies.

Actually not. SQL92 defines this command as

<prepare statement> ::=
PREPARE <SQL statement name> FROM <SQL statement variable>

<SQL statement variable> ::= <simple value specification>

where

<simple value specification> ::=
<parameter name>
| <embedded variable name>

(the normal <literal> case for <simple value specification> is
disallowed). So what they are really truly defining here is an
embedded-SQL operation in which the statement-to-prepare comes from
some kind of string variable in the client program. (SQL99 makes this
even clearer by moving PREPARE into Part 5, Host Language Bindings.)

AFAICT, the syntax we are setting up with actual SQL following the
PREPARE keyword is *not* valid SQL92 nor SQL99. It would be a good
idea to look and see whether any other DBMSes implement syntax that
is directly comparable to the feature we want. (Oracle manuals handy,
anyone?)

Assuming we do not find any comparable syntax to steal, my inclination
would be to go back to your original syntax and use "AS" as the
delimiter. That way we're not creating problems for ourselves if we
ever want to implement the truly spec-compliant syntax (in ecpg, say).

> The syntax is largely SQL92 compliant, but not totally. I'm not sure how
> the SQL spec expects parameters to be set up in PREPARE, but I doubt
> it's the same way I used.

I can't see any hint of specifying parameter types in SQL's PREPARE at
all. So we're on our own there, unless we can take some guidance
from other systems.

> And the SQL92 spec for EXECUTE is functionally
> similar, but uses a different syntax (EXECUTE ... USING INTO <rel>, I
> think).

It's not really similar at all. Again, the assumed context is an
embedded SQL program, and the real targets of INTO are supposed to be
host-program variable names. (plpgsql's use of SELECT INTO is a lot
more similar to the spec than our main grammar's use of it.)

While I won't strongly object to implementing EXECUTE INTO as you've
shown it, I think a good case could be made for leaving it out, on the
grounds that our form of SELECT INTO is a mistake and a compatibility
problem, and we shouldn't propagate it further. Any opinions out there?

In general, this is only vaguely similar to what SQL92 contemplates,
and you're probably better off not getting too close to their syntax...

Moving on to coding issues of varying significance:

> The patch stores queries in a hash table in TopMemoryContext.

Fine with me. No reason to change to a linked list. (But see note below.)

> Also, I'm not entirely sure my approach to memory management is
> correct. Each entry in the hash table stores its data in its
> own MemoryContext, which is deleted when the statement is
> DEALLOCATE'd. When actually running the prepared statement
> through the executor, CurrentMemoryContext is used. Let me know
> if there's a better way to do this.

I think it's all right. On entry to ExecuteQuery, current context
should be TransactionCommandContext, which is a perfectly fine place
for constructing the querytree-to-execute. You do need to copy the
querytree as you're doing because of our lamentable tendency to scribble
on querytrees in the executor.

* In PrepareQuery: plan_list must be same len as query list (indeed you
have an Assert for that later); this code will blow it if a UTILITY_CMD
is produced by the rewriter. (Can happen: consider a NOTIFY produced
by a rule.) Insert a NULL into the plan list to keep the lists in step.

* In StoreQuery, the MemoryContextSwitchTo(TopMemoryContext) should be
unnecessary. The hashtable code stuffs its stuff into its own context.
You aren't actually storing anything into TopMemoryContext, only into
children thereof.

* DeallocateQuery is not prepared for uninitialized hashtable.

* RunQuery should NOT do BeginCommand; that was done by postgres.c.

* Sending output only for last query is wrong; this makes incorrect
assumptions about what the rewriter will produce. AFAIK there is no
good reason you should not execute all queries with the passed-in dest;
that's what postgres.c does.

* Is it really appropriate to be doing Show_executor_stats stuff here?
I think only postgres.c should do that.

* This is certainly not legal C:

+ if (Show_executor_stats)
+ ResetUsage();
+
+ QueryDesc *qdesc = CreateQueryDesc(query, plan, dest, NULL);
+ EState *state = CreateExecutorState();

You must be using a C++ compiler.

* The couple of pfrees at the bottom of ExecuteQuery are kinda silly
considering how much else got allocated and not freed there.

* transformPrepareStmt is not doing the right thing with extras_before
and extras_after. Since you only allow an OptimizableStmt in the
syntax, probably these will always remain NIL, but I'd suggest throwing
in a test and elog.

* What if the stored query is replaced between the time that
transformExecuteStmt runs and the time the EXECUTE stmt is actually
executed? All your careful checking of the parameters could be totally
wrong --- and ExecuteQuery contains absolutely no defenses against a
mismatch. One answer is to store the expected parameter typelist
(array) in the ExecuteStmt node during transformExecuteStmt, and then
verify that this matches after you look up the statement in
ExecuteQuery.

* transformExecuteStmt must disallow subselects and aggregate functions
in the parameter expressions, since you aren't prepared to generate
query plans for them. Compare the processing of default or
check-constraint expressions. BTW, you might as well do the fix_opids
call at transform time not runtime, too.

* In gram.y: put the added keywords in the appropriate keyword-list
production (hopefully the unreserved one).

* Syntax for prepare_type_list is not good; it allows
( , int )
Probably best to push the () case into prepare_type_clause.

* typeidToString is bogus. Use format_type_be instead.

* Why does QueryData contain a context field?

* prepare.h should contain a standard header comment.

* You missed copyfuncs/equalfuncs support for the three added node types.

regards, tom lane