New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: ban ON CONFLICT REPLACE for secondary indexes #2963

Closed
TheAviat0r opened this Issue Nov 28, 2017 · 2 comments

Comments

Projects
None yet
3 participants
@TheAviat0r
Copy link
Collaborator

TheAviat0r commented Nov 28, 2017

Consider we have a space with multiple UNIQUE constraints, which have different ON CONFLICT clauses. If there are no constraint with ON CONFLICT REPLACE, then constraints execution order doesn't matter, because uniqueness violation doesn't affect data in a given space. However, when we have secondary index with ON CONFLICT REPLACE, it can bring us to an interesting results. Consider next example:

box.cfg{}

box.sql.execute('create table t1(a primary key, b unique on conflict replace, c unique on conflict replace)')
box.sql.execute('insert into t1 values (1, 2, 3), (4, 5, 6), (7, 8, 9)')

tarantool> select * from t1;
---
- - [1, 2, 3]
  - [4, 5, 6]
  - [7, 8, 9]

That is space condition before constraints violation. Pay attention to the fact that PRIMARY KEY error action is ABORT, not REPLACE.

Now try to insert a tuple, which violates:

  1. PRIMARY KEY index for column a
  2. All secondary indexes with ON CONFLICT REPLACE
    and see what happens
tarantool> insert into t1 values (1, 2, 6);
---
...

tarantool> select * from t1;
---
- - [1, 2, 6]
  - [7, 8, 9]

As you can see, there was a duplicate in primary key index, however, the insertion was successful and as a result, error action was REPLACE for primary key index instead of ABORT, which should happen by definition of that index.

And the root of that problem is that execution of constraint with ON CONFLICT REPLACE happens BEFORE making an insertion into Tarantool, which performs a uniqueness checks for constraints with default error action (ABORT). Also ALL tuple with conflicting key in secondary index will be deleted from space, after that a whole new one will be inserted.

Solution is simple - SQL ON CONFLICT REPLACE semantics should be the same as in Tarantool, where REPLACE is allowed only for primary key index.

@TheAviat0r TheAviat0r added the sql label Nov 28, 2017

@kostja kostja added this to the 1.8.5 milestone Nov 29, 2017

TheAviat0r added a commit that referenced this issue Nov 30, 2017

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

TheAviat0r added a commit that referenced this issue Dec 10, 2017

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

TheAviat0r added a commit that referenced this issue Dec 10, 2017

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

TheAviat0r added a commit that referenced this issue Dec 10, 2017

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

TheAviat0r added a commit that referenced this issue Dec 10, 2017

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

TheAviat0r added a commit that referenced this issue Dec 26, 2017

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

@kostja kostja modified the milestones: 1.8.5, goo Feb 7, 2018

TheAviat0r added a commit that referenced this issue Feb 12, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

TheAviat0r added a commit that referenced this issue Feb 12, 2018

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

TheAviat0r added a commit that referenced this issue Feb 12, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

For #2963

@TheAviat0r TheAviat0r self-assigned this Feb 12, 2018

TheAviat0r added a commit that referenced this issue Feb 21, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

Closes #2963

TheAviat0r added a commit that referenced this issue Feb 21, 2018

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

TheAviat0r added a commit that referenced this issue Feb 21, 2018

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

TheAviat0r added a commit that referenced this issue Feb 21, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

Closes #2963

TheAviat0r added a commit that referenced this issue Mar 5, 2018

sql: ban multiple ON CONFLICT REPLACE
Ban an opportunity for user to create table with multiple constraints
with ON CONFLICT REPLACE option.

For #2963

TheAviat0r added a commit that referenced this issue Mar 5, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.

Closes #2963

TheAviat0r added a commit that referenced this issue Mar 5, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.
- Ban an opportunity for user to create table with multiple constraints
  with ON CONFLICT REPLACE option.

Closes #2963
@TheAviat0r

This comment has been minimized.

Copy link
Collaborator

TheAviat0r commented Mar 26, 2018

There are two possible solutions:
No. 1:
Do not do SQLite-style REPLACE logic for unique keys.
Do it only for primary keys, as in Tarantool/NoSQL.
Therefore ON CONFLICT REPLACE is only legal for one
key in the table (the primary key), and the problem
of multiple ON CONFLICT REPLACE actions disappears.
No. 2
Check the ON CONFLICT REPLACE constraint, and if
it says "violation" then DELETE with DELETE rules.
Do the insert's BEFORE triggers.
Do all constraint checks for unique, check, and foreign-key,
including the ones that have ON CONFLICT REPLACE clauses, but this
time the result of conflict is always an error.
Do the statement action itself (update|insert|delete).
Do all AFTER triggers.

The problem with 2nd approach is that the order of execution also involves execution of Tarantool triggers, which happens after checking all constraints.

So we need to decide which approach we should take into account.

TheAviat0r added a commit that referenced this issue Mar 26, 2018

sql: change constraints checks order
- Modify constraints checks execution order, if user specified ON
  CONFLICT REPLACE
- Execute constraint check with ON CONFLICT REPLACE before BEFORE
  triggers, if it fails, then remove conflicting entry and insert new,
  after that - execute query as usual.
- Ban an opportunity for user to create table with multiple constraints
  with ON CONFLICT REPLACE option.

Closes #2963

@TheAviat0r TheAviat0r added the blocked label Mar 27, 2018

@TheAviat0r

This comment has been minimized.

Copy link
Collaborator

TheAviat0r commented Mar 27, 2018

Blocked by #3293

@TheAviat0r TheAviat0r changed the title sql: forbid multiple ON CONFLICT REPLACE in constraints sql: ban ON CONFLICT REPLACE for secondary indexes Apr 13, 2018

TheAviat0r added a commit that referenced this issue Apr 13, 2018

sql: allow ON CONFLICT REPLACE only for PK index
The problem is that constraints which require VDBE bytecode
will be executed before making insertion into Tarantool. For
three major constraints - UNIQUE, FOREIGN-KEY, CHECK the
execution order doesn't matter if error action is not REPLACE,
because one constraint violation doesn't affect others.

But violation of constraint with ON CONFLICT REPLACE leads to a
removal of whole tuple. When only PRIMARY KEY has REPLACE as
error action, it is not a problem and such behavior is absolutely
similar to Tarantool. But when we are dealing with secondary index
/NOT NULL constraint with ON CONFLICT REPLACE, conflict in
secondary index leads to a whole tuple removal, and as a result
when there is also a conflict in primary key index, the insertion
will lead to success instead of raising error.

In this patch possibility to create constraint non PRIMARY KEY
constraint with ON CONFLICT REPLACE was banned, that kind of
behavior will lead to a proper error message.

Closes #2963

TheAviat0r added a commit that referenced this issue Apr 22, 2018

sql: allow ON CONFLICT REPLACE only for PK index
The problem is that constraints which require VDBE bytecode
will be executed before making insertion into Tarantool. For
three major constraints - UNIQUE, FOREIGN-KEY, CHECK the
execution order doesn't matter if error action is not REPLACE,
because one constraint violation doesn't affect others.

But violation of constraint with ON CONFLICT REPLACE leads to a
removal of whole tuple. When only PRIMARY KEY has REPLACE as
error action, it is not a problem and such behavior is absolutely
similar to Tarantool. But when we are dealing with secondary index
/NOT NULL constraint with ON CONFLICT REPLACE, conflict in
secondary index leads to a whole tuple removal, and as a result
when there is also a conflict in primary key index, the insertion
will lead to success instead of raising error.

In this patch possibility to create constraint non PRIMARY KEY
constraint with ON CONFLICT REPLACE was banned, that kind of
behavior will lead to a proper error message.

Closes #2963

Korablev77 added a commit that referenced this issue May 3, 2018

sql: allow ON CONFLICT REPLACE only for PK index
Constraints which require VDBE bytecode will be executed before making
insertion into Tarantool. For three major constraints - UNIQUE,
FOREIGN-KEY, CHECK the execution order doesn't matter if error action is
not REPLACE, because one constraint violation doesn't affect others.

But violation of constraint with ON CONFLICT REPLACE leads to a
removal of the whole tuple. When only PRIMARY KEY has REPLACE as
error action, it is not a problem and such behavior is absolutely
similar to Tarantool. But when we are dealing with secondary index
NOT NULL constraint with ON CONFLICT REPLACE, conflict in
secondary index leads to a whole tuple removal, and as a result
when there is also a conflict in primary key index, insertion
will lead to success instead of raising error.

In this patch possibility to create constraint non PRIMARY KEY
constraint with ON CONFLICT REPLACE has been banned, that kind of
behavior leads to a proper error message.

Closes #2963

Korablev77 added a commit that referenced this issue May 3, 2018

sql: allow ON CONFLICT REPLACE only for PK index
Constraints which require VDBE bytecode will be executed before making
insertion into Tarantool. For three major constraints - UNIQUE,
FOREIGN-KEY, CHECK the execution order doesn't matter if error action is
not REPLACE, because one constraint violation doesn't affect others.

But violation of constraint with ON CONFLICT REPLACE leads to a
removal of the whole tuple. When only PRIMARY KEY has REPLACE as
error action, it is not a problem and such behavior is absolutely
similar to Tarantool. But when we are dealing with secondary index
NOT NULL constraint with ON CONFLICT REPLACE, conflict in
secondary index leads to a whole tuple removal, and as a result
when there is also a conflict in primary key index, insertion
will lead to success instead of raising error.

In this patch possibility to create constraint non PRIMARY KEY
constraint with ON CONFLICT REPLACE has been banned, that kind of
behavior leads to a proper error message.

Closes #2963

@kyukhin kyukhin closed this in f74f806 May 15, 2018

@locker locker removed the blocked label Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment