Re: INSERT ... VALUES... with ORDER BY / LIMIT

2010/10/3 Tom Lane <tgl@sss.pgh.pa.us>:
> Hitoshi Harada <umi.tanuki@gmail.com> writes:
>> 2010/10/2 Jeff Davis <pgsql@j-davis.com>:
>>> On Fri, 2010-10-01 at 18:52 +0900, Hitoshi Harada wrote:
>> While tackling the top-level CTEs patch, I found that INSERT ...
>> VALUES isn't aware of ORDER BY / LIMIT.
>
>> From my reading the source around transformInsertStmt(), VALUES in
>> INSERT is a bit apart from the one in SELECT. I see VALUES in INSERT
>> has to process DEFAULT and it doesn't accept NEW/OLD reference when it
>> is inside rule. But it doesn't seem like enough reason to explain why
>> the two are so different, at least to me.
>
> I think this is just an oversight here:
>
>    /*
>     * We have three cases to deal with: DEFAULT VALUES (selectStmt == NULL),
>     * VALUES list, or general SELECT input.  We special-case VALUES, both for
>     * efficiency and so we can handle DEFAULT specifications.
>     */
>    isGeneralSelect = (selectStmt && selectStmt->valuesLists == NIL);
>
> This test is failing to consider the possibility of optional clauses
> grafted onto the VALUES clause --- not just LIMIT, but ORDER BY etc
> (see insertSelectOptions()).  IMO we should simply consider that the
> presence of any of those options makes it a "general select".
> I don't believe that the SQL spec requires us to accept DEFAULT in
> such a context, and we don't need to be tense about efficiency for
> such weird cases either; so I don't want to clutter the special-purpose
> VALUES code path with extra code to handle those things.

Fair enough. I'll send the top-level DML in CTEs patch soon with the
test modified like:

isGeneralSelect = (selectStmt &&(selectStmt->valuesLists == NIL || selectStmt->sortClause || selectStmt->limitOffset ||
selectStmt->limitCount|| selectStmt->withClause)); 

And it fixes LIMIT and etc. case bugs.

DEFAULT is disallowed now in such VALUES list, but we can explain it
is allowed in a "simple" VALUES of INSERT case.

Regards,


--
Hitoshi Harada