On 09/24/2014 12:22 AM, Kevin Grittner wrote:
Heikki Linnakangas <hlinnakan...@vmware.com> wrote:

instead of passing parameters to the SPI calls individually, you
invented SPI_register_tuplestore which affects all subsequent SPI

All subsequent SPI calls on that particular SPI connection until it
is closed, except for any tuplestores are later unregistered.
Nested SPI connections do not automatically inherit the named
tuplestores; whatever code opens an SPI connection would need to
register them for the new context, if desired.  This seemed to me
to provide minimal disruption to the existing SPI callers who might
want to use this.

Yeah, I got that. And note that I'm not saying that's necessarily a bad design per se - it's just that it's different from the way parameters work, and I don't like it for that reason.

You could imagine doing the same for parameters; have a SPI_register_param() function that you could use to register parameter types, and the parameters could then be referenced in any SPI calls that follow (within the same connection). But as the code stands, SPI is stateless wrt. to parameters, and tuplestores or relation parameters should follow the lead.

The next question is how to pass the new hooks and tuplestores
However, there doesn't seem to be any way to do one-shot queries
with a ParserSetupHook and ParamListInfo. That seems like an
oversight, and would be nice to address that anyway.

There are dozens of SPI_prepare* and SPI_exec* calls scattered all
over the backend, pl, and contrib code which appear to get along
fine without that.

Yeah. None of the current callers have apparently needed that functionality. But it's not hard to imagine cases where it would be needed. For example, imagine a variant of EXECUTE '...' where all the PL/pgSQL variables can be used in the query, like they can in static queries:

  myvar int4;
  tablename text;
  EXECUTE 'insert into ' || tablename ||' values (myvar)';

Currently you have to use $1 in place of the variable name, and pass the variable's current value with USING. If you wanted to make the above work, you would need a variant of SPI_execute that can run a one-shot query, with a parser-hook.

Whether you want to use a parser-hook or is orthogonal to whether or not you want to run a one-shot query or prepare it and keep the plan.

 Partly it may be because it involves something
of a modularity violation; the comment for the function used for
this call (where it *is* used) says:

  * plpgsql_parser_setup     set up parser hooks for dynamic parameters
  * Note: this routine, and the hook functions it prepares for, are logically
  * part of plpgsql parsing.  But they actually run during function execution,
  * when we are ready to evaluate a SQL query or expression that has not
  * previously been parsed and planned.

No, that's something completely different. The comment points out that even though plpgsql_parser_setup is in pl_comp.c, which contains code related to compiling a PL/pgSQL function, it's actually called at execution time, not compilation time.

Can you clarify what benefit you see to modifying the SPI API the
way you suggest, and what impact it might have on existing calling

Well, we'll have to keep the existing functions anyway, to avoid breaking 3rd party code that use them, so there would be no impact on existing code. The benefit would be that you could use the parser hooks and the ParamListInfo struct even when doing a one-shot query.

Or perhaps you could just use SPI_prepare_params + SPI_execute_plan_with_paramlist even for one-shot queries. There is some overhead when a SPIPlan has to be allocated, but maybe it's not big enough to worry about. That would be worth measuring before adding new functions to the SPI.

PS. the copy/read/write functions for TuplestoreRelation in the
patch are broken; TuplestoreRelation is not a subclass of Plan.

I'm not sure what you mean by "broken" -- could you elaborate?


+ /*
+  * _copyTuplestoreRelation
+  */
+ static TuplestoreRelation *
+ _copyTuplestoreRelation(const TuplestoreRelation *from)
+ {
+       TuplestoreRelation   *newnode = makeNode(TuplestoreRelation);
+       /*
+        * copy node superclass fields
+        */
+       CopyPlanFields((const Plan *) from, (Plan *) newnode);
+       /*
+        * copy remainder of node
+        */
+       COPY_STRING_FIELD(refname);
+       return newnode;
+ }

You cast the TuplestoreRelation to Plan, and pass it to CopyPlanFields. That will crash, because TuplestoreRelation is nothing like a Plan:

+ /*
+  * TuplestoreRelation -
+  *       synthetic node for tuplestore passed in to the query by name
+  *
+  * This is initially added to support trigger transition tables, but may find
+  * other uses, so we try to keep it generic.
+  */
+ typedef struct TuplestoreRelation
+ {
+       NodeTag         type;
+       char       *refname;
+ } TuplestoreRelation;

The corresponding code in outfuncs.c is similarly broken.

- Heikki

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to