Re: multiple inserts - Mailing list pgsql-patches

Liam Stewart <liams@redhat.com> writes:
> Here is another version of the multiple insert patch that contains
> modifications as per Tom's post on the 24th.

I poked at this a little bit and find that it still has problems,
for example:

regression=# create table foo (f1 int, f2 int);
CREATE

regression=# insert into foo values (1,2), (3,4);
INSERT 0 2

regression=# insert into foo values (0,4), (3,1+(select max(f2) from foo));
ERROR:  fmgr_info: function 0: cache lookup failed

regression=# insert into foo values (0,4),
regression-# (3,case when 1 = any (select max(f2) from foo) then 1 else 0 end);
server closed the connection unexpectedly
(postmaster log shows backend died with SIGSEGV)

While these particular problems could doubtless be fixed (they are due
to failure to consider Result's additional targetlists in setrefs.c),
I'm beginning to have serious doubts about the approach of adding
multiple targetlists to Query and Result nodes.  The patch is much
larger and uglier than I'd hoped it would turn out, and yet there are
still many places that arbitrarily assume they are dealing with
single-targetlist cases.  This does not look like an approach that will
scale up easily to the place we'd hoped to get to (allowing VALUES
constructs as sub-selects).

I am very sorry for leading you down the garden path, but at this point
I wonder whether you shouldn't scrap the multi-targetlist approach and
try again.  The desired feature could be added without any impact beyond
the parser, if analyze.c were to turn a multi-VALUES-list INSERT into
something that looked like it had been produced by

    INSERT INTO targettable
        SELECT first-values-list
        UNION ALL
        SELECT second-values-list
        UNION ALL
        ...

You could do this quite easily in transformInsertStmt: if there's more
than one sub-list in stmt->values, transform each sublist as a
targetlist and then build a Query node just for that targetlist, finally
assembling the Query nodes into a SetOp tree.  Note you'd want to apply
prepareInsertTargetList to each of the sub-targetlists separately (to
coerce the result datatypes to the target column types) rather than
running the type resolution code that UNION normally uses to resolve
disparate datatypes in the arms of the UNION.  Otherwise it seems a
fairly straightforward, localized exercise.

I think we had talked about this idea earlier and decided to shoot for
propagating the notion of multiple targetlists into Query and Result
nodes instead.  But now it's looking like that was the wrong decision to
make.  I think we'd better add the general capability to our list of
things-to-think-about-when-we-redesign-querytrees, and just go for
fixing INSERT ... VALUES for the moment.

The good news is your time was not wasted, since you now know a lot more
about pieces of the system beyond the parser than you would've without
making this venture ;-).

            regards, tom lane