[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Mon, 16 Jul 2018 13:20:30 +0300

Hello. Thanks for the fixes! See 3 comments below.

On 16/07/2018 12:43, Kirill Shcherbatov wrote:

I have pushed my review fixes on the branch, and they
correct some of the comments. Please, finish the patch
after my fixes including rest of the comments.
Thank you for review, I've fixed all problems on branch and squashed.

1. Please, remove is_primkey. It is not needed here and can be easily
removed together with merging this cycle into the code below. The
only place where it is still used except here is
check_on_conflict_replace_entries, that can be spread over other
primary key checks.
2. Please, use ER_NULLABLE_PRIMARY.
3. This is checked already, so the rest of hunk below can be removed.
Fixed with review fixes.

1. No, is_primkey still is not removed. I see it in struct Column.



============================================


This patch dissallows define multiple "NULL", "NOT NULL"
options per column and fixes silent implicit behavior
for invalid "NULL PRIMARY KEY" construction.

Closes #3473.
---
  src/box/sql/build.c           | 86 +++++++++++++++++++++++++++++++++----------
  src/box/sql/parse.y           |  7 +++-
  test/sql/on-conflict.result   | 21 +++++++++++
  test/sql/on-conflict.test.lua |  8 ++++
  4 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..e2bdc4a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,7 +276,12 @@ ccons ::= DEFAULT id(X).              {
  // In addition to the type name, we also care about the primary key and
  // UNIQUE constraints.
  //
-ccons ::= NULL onconf.
+ccons ::= NULL onconf(R).        {
+    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
+    /* Trigger nullability mismatch error if required. */
+    if (R != ON_CONFLICT_ACTION_DEFAULT)

2. Why do you need this check?

3. Why sqlite3AddNotNull is called when NULL is allowed?
Please, rename the function or split it or something.

+        sqlite3AddNotNull(pParse, R);
+}
  ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
  ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
                                   {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018