Re: insert multiple rows attempt two - Mailing list pgsql-patches

Liam Stewart <liams@redhat.com> writes:
> Here is a second attempt at a patch for inserting multiple values with
> an INSERT ... VALUES. I did made some simple regression tests while
> working on this patch so I'm sending them along as well.
> Comments?

Getting there, but still needs work.

> *** src/backend/executor/nodeResult.c    2001/03/22 06:16:13    1.19
> --- src/backend/executor/nodeResult.c    2001/08/22 14:55:30
> ***************
> *** 148,157 ****
>           {

>               /*
> !              * if we don't have an outer plan, then we are just generating
> !              * the results from a constant target list.  Do it only once.
>                */
> !             resstate->rs_done = true;
>           }

>           /*
> --- 148,168 ----
>           {

>               /*
> !              * if we don't have an outer plan, then we are just
> !              * generating the results from a constant list of target
> !              * lists. Do it only while the list has elements in it.
> !              * An item in the list is placed in the pi_targetlist of
> !              * resstate->cstate.cs_ProjInfo for processing.
>                */
> !             if (node->valueslist == NIL)
> !                 resstate->rs_done = true;
> !             else
> !             {
> !                 if (length(node->valueslist) == 1)
> !                     resstate->rs_done = true;
> !                 resstate->cstate.cs_ProjInfo->pi_targetlist = lfirst(node->valueslist);
> !                 node->valueslist = lnext(node->valueslist);
> !             }
>           }

>           /*

This is not okay: you're physically destroying the contents of the
Result node, which means that the plan cannot be rerun.  In particular,
any attempt to rescan it will give wrong results.  (I'm not sure that
rescan can happen during a simple INSERT ... VALUES command, but there's
no point in having a Result-node feature that's broken in the general
case.)

What you need, I think, is a counter or pointer in the ResultState node
that points at the current targetlists-list element, and is initialized
(reset) by ExecInitResult and ExecReScanResult.  The Result node
structure needs to be treated as read-only here.

I'd also prefer not to see schizophrenia over whether Result pays
attention to a targetlist or a list-of-targetlists; IMHO there should be
only one code path dealing with the list.  Places that currently set up
Results with a single targetlist should have a makeList1() added to convert
their targetlist to a list of lists.

> !             if (qry->valuesList == NIL)
> !                 qry->valuesList = makeList1(transformTargetList(pstate, tl));
> !             else
> !                 lappend(qry->valuesList, transformTargetList(pstate, tl));

I see a number of places where you did list extension like that.  While
it works, it's verbose and not idiomatic.  Correct procedure is just

    qry->valuesList = lappend(qry->valuesList,
                  transformTargetList(pstate, tl));

There's no need for a special case for a currently-nil list this way.

> *** src/include/nodes/parsenodes.h    2001/08/21 16:36:06    1.142
> --- src/include/nodes/parsenodes.h    2001/08/22 14:55:33
> ***************
> *** 57,62 ****
> --- 57,65 ----

>       List       *targetList;        /* target list (of TargetEntry) */

> +     List       *valuesList;        /* list of lists of TargetEntries
> +                                  * (VALUES clauses) */
> +
>       List       *groupClause;    /* a list of GroupClause's */

I do not like the fact that you have added a field to Query that has
such a poorly-defined relation to targetList.  Which is primary?
Can it possibly be correct that there is only one place in the planner
and rewriter that needs to look at valuesList as well as targetList?
(I doubt it.)  Can it possibly be correct that targetList may point at
substructure of valueList?  (Definitely not; that will lead to two
passes of processing over the same node tree, which is not good.  One
case that'll almost certainly misbehave is where there are sub-SELECTs
in the targetlist/valuelist.)

The only way to do this that would give me any confidence in the code
at all is to *not* add a separate valuesList field, but to replace
targetList by a targetLists field that's defined as a list of lists of
TargetEntries.  Renaming the field guarantees that everyplace that
currently touches targetList breaks, and as you pass through the code
fixing that, you can fix anyplace that assumes it's dealing with a
single-level list of TargetEntries.  For some places it doesn't matter
(example: the node-copying code still just invokes Node_Copy); for some
you will need to replace a one-level loop with a two-level loop; and
for some it may be appropriate to assume there is only one list of
target entries and just insert an lfirst() to get to it.  (I'd suggest
adding Assert(length(targetLists) == 1) to places where you assume
this.)

>       /*
> !      * An INSERT statement has *either* VALUES or SELECT, never both. If
> !      * VALUES, a targetList is supplied (empty for DEFAULT VALUES). If
> !      * SELECT, a complete SelectStmt (or set-operation tree) is supplied.
>        */
> !     List       *targetList;        /* the target list (of ResTarget) */
>       Node       *selectStmt;        /* the source SELECT */
>   } InsertStmt;

> --- 808,819 ----
>       List       *cols;            /* optional: names of the target columns */

>       /*
> !      * An INSERT statement has *either* VALUES or SELECT, never
> !      * both. If VALUES, a list of lists of ResTarget's is supplied
> !      * (empty for DEFAULT VALUES). If SELECT, a complete SelectStmt
> !      * (or set-operation tree) is supplied.
>        */
> !     List       *values;            /* the values to insert */
>       Node       *selectStmt;        /* the source SELECT */
>   } InsertStmt;

See, you did InsertStmt cleanly.  But Query has to have the same kind of
fundamental alteration.

            regards, tom lane