Re: 9.5: UPDATE/DELETE .. ORDER BY .. LIMIT ..

On 05/13/2014 10:45 PM, Rukh Meski wrote:
> On Sun, May 11, 2014 at 4:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The $64 question is whether we'd accept an implementation that fails
>> if the target table has children (ie, is partitioned). That seems
>> to me to not be up to the project's usual quality expectations, but
>> maybe if there's enough demand for a partial solution we should do so.
>>
>> It strikes me that a big part of the problem here is that the current
>> support for this case assumes that the children don't all have the
>> same rowtype. Which is important if you think of "child table" as
>> meaning "subclass in the OO sense". But for ordinary partitioning
>> cases it's just useless complexity, and ModifyTable isn't the only
>> thing that suffers from that useless complexity.
>>
>> If we had a notion of "partitioned table" that involved a restriction
>> that all the child tables have the exact same rowtype, we could implement
>> UPDATE/DELETE in a much saner fashion --- just one plan tree, not one
>> per child table --- and it would be possible to support UPDATE/DELETE
>> ORDER BY LIMIT with no more work than for the single-table case.
>> So that might shift the calculation as to whether we're willing to
>> accept a partial implementation.
>
> None of the use cases I have in mind would ever (have to) use this on
> a parent table; in the worst case it might make sense to do it on the
> child tables individually. Personally, I think that just refusing to
> operate on tables with children is a reasonable start. I have no
> interest in working on improving partitioning, but I don't think
> pushing this feature back in the hopes that someone else will would
> help anyone.

IMHO this needs to work with inheritance if we are to accept it. It
would be a rather strange limitation for no apparent reason, other than
that we didn't bother to implement it. It doesn't seem very difficult in
theory to add the table OID to the plan as a junk column, and use that
in the ModifyTable node to know which table a row came from.

In any case, the patch as it stands is clearly not acceptable, because
it just produces wrong results with inheritance. I'm marking it as
returned with feedback in the commitfest app. I'd suggest that you solve
the inheritance problems and resubmit.

Per the docs in the patch:

> + <para>
> + If the <literal>LIMIT</> (or <literal>FETCH FIRST</>) clause
> + is present, processing will stop after the system has attempted
> + to delete the specified amount of rows. In particular, if a row
> + was concurrently changed not to match the given <literal>WHERE</>
> + clause, it will count towards the <literal>LIMIT</> despite it
> + not being actually deleted. Unlike in <literal>SELECT</>, the
> + <literal>OFFSET</literal> clause is not available in
> + <literal>DELETE</>.
> + </para>

That behavior with READ COMMITTED mode and concurrent changes is
surprising. Do we really want it to behave like that, and if so, why?

Why is OFFSET not supported? Not that I care much for that, but I'm curious.

- Heikki