Re: delta relations in AFTER triggers

On 09/24/2014 12:22 AM, Kevin Grittner wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
>
>> instead of passing parameters to the SPI calls individually, you
>> invented SPI_register_tuplestore which affects all subsequent SPI
>> calls.
>
> 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:

declare
myvar int4;
tablename text;
begin
...
EXECUTE 'insert into ' || tablename ||' values (myvar)';
end;

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
> code?

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?

Sure:

> + /*
> + * _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