Re: SQL:2011 Application Time Update & Delete

On 13.03.26 18:06, Paul A Jungwirth wrote:
>> 3.7)
>>
>> +-- UPDATE ... RETURNING returns only the updated values (not the
>> inserted side values)
>>
>> This test looks redundant with earlier tests. Otherwise, maybe add a
>> comment about how it's different.
>
> I don't think a top-level RETURNING test is redundant with the CTE
> test. I expanded the comment here a bit to clarify the goal. It
> addresses your question above: Should RETURNING include the inserted
> leftovers? I don't think that makes sense:
>
> 1. Our docs say, "The optional RETURNING clause causes UPDATE to
> compute and return value(s) based on each row actually updated." The
> leftovers were not updated.
>
> 2. Conceptually, the leftovers represent what *didn't* change.
>
> 3. If you implemented this with a trigger, you also wouldn't get the
> inserted leftovers.
>
> 4. The SQL standard doesn't have RETURNING. But it does say that to
> insert the leftovers the system should execute a separate insert
> "statement". So we should do something very similar to the trigger
> case.

UPDATE ... RETURNING is in my mind equivalent to the SQL standard SELECT
... FROM NEW TABLE (UPDATE ...) (see <data change delta table>). So I
was hoping for an answer there, but it just says:

"If TP simply contains a <data change delta table> DCDT, then let S be
the <data change statement> simply contained in TP. S shall not contain
FOR PORTION OF."

So we can pick our own behavior. Your explanation makes sense. (I
suppose an alternative is that we also don't allow using FOR PORTION OF
together with RETURNING?)

>> 5) NULL bounds
>>
>> A general comment: In particular after studying these tests in detail,
>> I'm suspicious that it's a good idea to interpret null bounds as
>> unbounded. Expressions could return null for nested reasons, it would
>> be very hard to follow that. Null values should mean "unknown",
>> unbounded should be explicit. We have the keyword UNBOUNDED already,
>> maybe you could use that? Or do you want to be able to return
>> unboundedness from an expression?
>
> I like the idea of a keyword. I tried adding UNBOUNDED but it caused a
> few hundred S/R and R/R conflicts that I couldn't easily resolve. A
> year or two ago I had keywords here (MINVALUE/MAXVALUE IIRC), but it
> required some nasty parser hacks. This is a pretty delicate area of
> the grammar, because we have a_expr with FROM and TO and no
> punctuation. I'm already doing some contortions to handle `FOR PORTION
> OF valid_at FROM t1 + INTERVAL '1' YEAR TO MONTH TO t2`.
>
> A keyword is not offered by the standard here, so it would just be
> custom syntactic sugar. No other RDBMS has one (I think).
>
> I think NULL is the right choice for unbounded. It is what range types
> use, and we want this to mesh well with them. More important it works
> for *any type*. We don't always have +/-Infinity.
>
> Also I think we should expand user choice rather than restrict it. If
> users want to forbid nulls, they can (e.g. by using a domain type).
> But if we forbid it, there is no way to override that decision.
>
> Going back to the UNBOUNDED keyword: if we forbid nulls, then a
> keyword doesn't really add clarity, since users would already say
> `-Infinity` or `Infinity`. It's really just a way to express what null
> means in this context. Assuming we keep nulls, I'd like to keep
> working on a keyword. But I think we could add it later.

Yeah, this seems like something we could change later with relative
ease. Maybe solicit some input from the public during beta?

> Btw what do you think of the READ COMMITTED issues I brought up in my
> third patch? We follow MariaDB here, but not DB2. DB2's behavior is
> less problematic for users, although their isolation levels don't
> quite match ours. If we're not okay with those results, we should
> address them before merging the main patch.

It's still hard to understand. I would be ok in general to say that
results might be unexpected unless you use REPEATABLE READ. Especially
as it seems that a technical solution to improve this would be possible
later. But we should document this in more detail. The verbal
explanations are hard to interpret. Could you maybe come up with a
couple of ASCII-art flow charts that explains how things could go
strange that we could put into the documentation?

Could we actually put some of these strange/unexpected behaviors into
the isolation tests? Right now we only test that the workaround works
but not the initial problem. Is this possible? (Would we need
injection points?)

Could we cut back the isolation tests a bit? They are the second slowest
isolation test now, and the second largest expected file. Maybe we
don't need to test SERIALIZE separately? (Assume that SERIALIZE is as
good as or better than REPEATABLE READ?)

Attached is a patch with a few small cosmetic corrections. In
ExecForPortionOfLeftovers(), the comment and code that I delete is
duplicated before and inside the loop. The one before the
loop is probably sufficient.

Other than all that, this patch set (0001 through 0003) seems good to me.