[Maria-developers] MDEV-10035 DBUG_ASSERT on CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE
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? Thanks.
Hi! 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?
Just now this parameter for VIEWs effectively (and silently) ignored. So error IMHO is better. [skip]
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? 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. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
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
Hi, Alexander! On May 16, Alexander Barkov wrote:
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;
That's different. See: CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2; This is different from SELECT * FROM t1, t2 FOR UPDATE; the first locks only t1, the second - both t1 and t2.
And by the way, it does not work even with TEMPTABLE. Here is an example:
Sure, I didn't say it does. I said it might work with TEMPTABLE and it cannot possibly work with MERGE. Either we fix it to work or we disallow it.
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?
I think we need to apply the same logic to all per-SELECT clauses. LOCK IN SHARE MODE, FOR UPDATE, HIGH_PRIORITY, STRAIGHT_JOIN, SQL_SMALL_RESULT, SQL_BIG_RESULT, SQL_BUFFER_RESULT. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi! On 16.05.2016 14:26, Sergei Golubchik wrote:
Hi, Alexander!
On May 16, Alexander Barkov wrote:
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; That's different. See:
CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2;
This is different from
SELECT * FROM t1, t2 FOR UPDATE;
the first locks only t1, the second - both t1 and t2. No, "FOR UPDATE" has no any traces in view.frm (because it implemented by writing properties to individual tables during parsing, which I think is bad thing in all senses). So if it is processed now differently it is only because FOR UPDATE works incorrectly and do not goes inside views (I did not checked it but it is probable).
[skip]
Hi, Oleksandr! On May 16, Oleksandr Byelkin wrote:
On May 16, Alexander Barkov wrote:
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; That's different. See:
CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2;
This is different from
SELECT * FROM t1, t2 FOR UPDATE;
the first locks only t1, the second - both t1 and t2.
No, "FOR UPDATE" has no any traces in view.frm
Yes, it doesn't work now. Above I meant, what the user expectations could be. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
On 16.05.2016 14:56, Sergei Golubchik wrote:
Hi, Oleksandr!
On May 16, Oleksandr Byelkin wrote:
On May 16, Alexander Barkov wrote:
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; That's different. See:
CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2;
This is different from
SELECT * FROM t1, t2 FOR UPDATE;
the first locks only t1, the second - both t1 and t2. No, "FOR UPDATE" has no any traces in view.frm Yes, it doesn't work now. Above I meant, what the user expectations could be.
Yes, but they are against at least standard, and we should think a lot before implement yet another non-standard feature. The thing is theoretically doable, but require changes in current algorithm of processing. Also the question which setup overwrite which. Whole query derived table or vice versa.
Hi, Oleksandr! On May 16, Oleksandr Byelkin wrote:
That's different. See:
CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2;
This is different from
SELECT * FROM t1, t2 FOR UPDATE;
the first locks only t1, the second - both t1 and t2. meant, what the user expectations could be.
Yes, but they are against at least standard, and we should think a lot before implement yet another non-standard feature.
Sure. FOR UPDATE, LOCK IN SHARE MODE, and clauses I've mentioned in another email are all non-standard. The question is not about implementing another non-standard feature, but about how the combination of existing features should work. This is a tradeoff between the principle of the least surprise, how much effort we want to put into this, and whether there's anyone who expects it to work :) Regards, Sergei Chief Architect MariaDB and security@mariadb.org
On 16.05.2016 16:05, Sergei Golubchik wrote:
Hi, Oleksandr!
On May 16, Oleksandr Byelkin wrote:
That's different. See:
CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE; SELECT * FROM v1, t2;
This is different from
SELECT * FROM t1, t2 FOR UPDATE;
the first locks only t1, the second - both t1 and t2. meant, what the user expectations could be. Yes, but they are against at least standard, and we should think a lot before implement yet another non-standard feature. Sure. FOR UPDATE, LOCK IN SHARE MODE, and clauses I've mentioned in another email are all non-standard.
The question is not about implementing another non-standard feature, but about how the combination of existing features should work.
This is a tradeoff between the principle of the least surprise, how much effort we want to put into this, and whether there's anyone who expects it to work :) OK, but you have not answered question about priority, because both has sense (at least for me).
participants (3)
-
Alexander Barkov -
Oleksandr Byelkin -
Sergei Golubchik