Hi Sergei, On 05/16/2016 10:35 AM, Sergei Golubchik wrote:
Hi, Oleksandr!
On May 16, Oleksandr Byelkin wrote:
On 16.05.2016 06:00, Alexander Barkov wrote:
Hi Sanja, Sergei,
Sanja, you asked me to review a patch for MDEV-10035:
revision-id: 4ffe2295e78538dde93df078421726f0c5a7d2a2 (mariadb-10.2.0-29-g4ffe229) parent(s): b79944950e5e5db40cf7ad49061edf5f105512c4 committer: Oleksandr Byelkin timestamp: 2016-05-15 15:25:33 +0200 message:
I earlier also proposed the same idea to disallow FOR UPDATE, and even created a patch disallowing this in the parser syntactically (see attached).
But Sergei worried that we should not do it this way and proposed some other solutions. Please see Sergei's comments in:
MDEV-10063 VIEWs and subqueries with FOR UPDATE
Sergei, are you still in doubts?
Yes :) Out of all approaches, I think, I'd prefer to issue a warning like "Clause FOR UPDATE" won't work unless you specify TEMPTABLE algorithm. Making it an error in the strict mode, as usual.
Just now this parameter for VIEWs effectively (and silently) ignored. So error IMHO is better.
It will break existing application where users have FOR UPDATE in the view definition. But, probably, there won't be many?
I think there won't be many. For me it looks like when we added FOR UPDATE, we just forgot to disallow this in VIEW. A more clear way is to use FOR UPDATE when invoking views than when creating views: SELECT * FROM view1 FOR UPDATE; And by the way, it does not work even with TEMPTABLE. Here is an example: # Connection#1: DROP TABLE IF EXISTS t1,t2; DROP VIEW IF EXISTS v1; CREATE TABLE t1 (a INT); CREATE TABLE t2 (a INT); INSERT INTO t1 VALUES (10); INSERT INTO t2 VALUES (10); CREATE ALGORITHM=TEMPTABLE VIEW v1 AS SELECT * FROM t1 UNION ALL (SELECT * FROM t2 FOR UPDATE); START TRANSACTION; SELECT * FROM v1 FOR UPDATE; # Connection#2 (does not lock, returns results immediately): START TRANSACTION; SELECT * FROM v1 FOR UPDATE; COMMIT; +------+ | a | +------+ | 10 | | 10 | +------+ # Connection#3 (does not lock either, returns results immediately): START TRANSACTION; SELECT * FROM t2 FOR UPDATE; COMMIT; +------+ | a | +------+ | 10 | +------+
But also this patch goes directly against the work that Bar is doing now. This patch adds new manual syntax checks into the code, while Bar is trying to have the grammar to allow only syntaxically corect queries.
I even earlier made a patch disallowing this syntactically. It was in the previous letter. I'm attaching it again. I made it before you commented in MDEV-10063 :) But then would not sent for review, because we had a discussing that it might be useful with TEMPTABLE... But now when we know that TEMPTABLE does not really work... Ok to push the attached patch? Thanks!
Regards, Sergei Chief Architect MariaDB and security@mariadb.org