From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 1CC952A05F for ; Sun, 26 Aug 2018 15:44:30 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kFd5lL1qPOSp for ; Sun, 26 Aug 2018 15:44:30 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 3B87129AEF for ; Sun, 26 Aug 2018 15:44:29 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server From: "n.pettik" In-Reply-To: Date: Sun, 26 Aug 2018 22:44:20 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <63833016-A944-4856-ADDB-DAB672BC887B@tarantool.org> References: <893c37395b2536af932aac033f6c54718720b63f.1534001739.git.korablev@tarantool.org> <685EC40E-4A7E-4A5A-8BAD-A597ADC67E67@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >>> On 13 Aug 2018, at 23:24, Vladislav Shpilevoy = wrote: >>>=20 >>> Are you sure it is the only way to pass autoinc fieldno? And that it = can >>> not be dropped and calculated from other fields without significant = problems? >>>=20 >>> Now this flag looks very ugly inside _space tuples. >>>=20 >>> I think, autoinc is rather primary index option than the space, and = that it >>> can be detected via checking >>>=20 >>> pk->part_count =3D=3D 1 and space->sequence !=3D NULL >>>=20 >>> then pk->parts[0].fieldno is autoinc field. It is not? >> Okay, it seems to be possible. Check this out: >> sql: remove autoincrement field number from Table >> During INSERT SQL statement execution we may need to know = field which >> stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to = get rid >> of struct Table, lets remove this member from struct Table. = Instead, it >> can be calculated using struct space: if PK consists from one = part and >> sequence not NULL - table features autoincrement. >> Part of #3561 >=20 > I have done some review fixes, but the patch crashes with them, > and I do not understand why. Could you please investigate? Yep, surely. >=20 > Assertion is 100% repeatable on analyze9.test: > Assertion failed: (0), function vdbe_emit_constraint_checks, file = /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/insert.c, = line 916. >=20 > Right after test case 4.9. >=20 > The diff: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D >=20 > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 56ddb10eb..4cc33e830 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -858,12 +858,12 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, > struct sqlite3 *db =3D parse_context->db; > struct Vdbe *v =3D sqlite3GetVdbe(parse_context); > assert(v !=3D NULL); > - struct space_def *def =3D tab->def; > - /* Insertion into VIEW is prohibited. */ > - assert(!def->opts.is_view); > bool is_update =3D upd_cols !=3D NULL; > struct space *space =3D space_by_id(tab->def->id); > assert(space !=3D NULL); > + struct space_def *def =3D space->def; > + /* Insertion into VIEW is prohibited. */ > + assert(!def->opts.is_view); > uint32_t autoinc_fieldno =3D sql_space_autoinc_fieldno(space); > /* Test all NOT NULL constraints. */ > for (uint32_t i =3D 0; i < def->field_count; i++) { > @@ -882,7 +882,7 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, > /* ABORT is a default error action. */ > if (on_conflict_nullable =3D=3D = ON_CONFLICT_ACTION_DEFAULT) > on_conflict_nullable =3D = ON_CONFLICT_ACTION_ABORT; > - struct Expr *dflt =3D = space_column_default_expr(tab->def->id, i); > + struct Expr *dflt =3D def->fields[i].default_value_expr; > if (on_conflict_nullable =3D=3D = ON_CONFLICT_ACTION_REPLACE && > dflt =3D=3D NULL) > on_conflict_nullable =3D = ON_CONFLICT_ACTION_ABORT; > @@ -923,7 +923,7 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, > if (on_conflict =3D=3D ON_CONFLICT_ACTION_DEFAULT) > on_conflict =3D ON_CONFLICT_ACTION_ABORT; > /* Test all CHECK constraints. */ > - struct ExprList *checks =3D = space_checks_expr_list(tab->def->id); > + struct ExprList *checks =3D def->opts.checks; > enum on_conflict_action on_conflict_check =3D on_conflict; > if (on_conflict =3D=3D ON_CONFLICT_ACTION_REPLACE) > on_conflict_check =3D ON_CONFLICT_ACTION_ABORT; I slightly changed your diff. Final version is (no assertions happen and = all tests are passed): +++ b/src/box/sql/insert.c @@ -858,12 +858,12 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, struct sqlite3 *db =3D parse_context->db; struct Vdbe *v =3D sqlite3GetVdbe(parse_context); assert(v !=3D NULL); - struct space_def *def =3D tab->def; - /* Insertion into VIEW is prohibited. */ - assert(!def->opts.is_view); bool is_update =3D upd_cols !=3D NULL; struct space *space =3D space_by_id(tab->def->id); assert(space !=3D NULL); + struct space_def *def =3D space->def; + /* Insertion into VIEW is prohibited. */ + assert(!def->opts.is_view); uint32_t autoinc_fieldno =3D sql_space_autoinc_fieldno(space); /* Test all NOT NULL constraints. */ for (uint32_t i =3D 0; i < def->field_count; i++) { @@ -878,11 +878,11 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, continue; enum on_conflict_action on_conflict_nullable =3D on_conflict !=3D ON_CONFLICT_ACTION_DEFAULT ? - on_conflict : = tab->def->fields[i].nullable_action; + on_conflict : def->fields[i].nullable_action; /* ABORT is a default error action. */ if (on_conflict_nullable =3D=3D = ON_CONFLICT_ACTION_DEFAULT) on_conflict_nullable =3D = ON_CONFLICT_ACTION_ABORT; - struct Expr *dflt =3D = space_column_default_expr(tab->def->id, i); + struct Expr *dflt =3D space_column_default_expr(def->id, = i); if (on_conflict_nullable =3D=3D = ON_CONFLICT_ACTION_REPLACE && dflt =3D=3D NULL) on_conflict_nullable =3D = ON_CONFLICT_ACTION_ABORT; @@ -923,7 +923,7 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, if (on_conflict =3D=3D ON_CONFLICT_ACTION_DEFAULT) on_conflict =3D ON_CONFLICT_ACTION_ABORT; /* Test all CHECK constraints. */ - struct ExprList *checks =3D = space_checks_expr_list(tab->def->id); + struct ExprList *checks =3D space_checks_expr_list(def->id); enum on_conflict_action on_conflict_check =3D on_conflict; if (on_conflict =3D=3D ON_CONFLICT_ACTION_REPLACE) on_conflict_check =3D ON_CONFLICT_ACTION_ABORT; @@ -965,7 +965,7 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct Table *tab, if (part_count =3D=3D 1) { uint32_t fieldno =3D pk->def->key_def->parts[0].fieldno; int reg_pk =3D new_tuple_reg + fieldno; - if (tab->def->fields[fieldno].affinity =3D=3D = AFFINITY_INTEGER) { + if (def->fields[fieldno].affinity =3D=3D = AFFINITY_INTEGER) { int skip_if_null =3D sqlite3VdbeMakeLabel(v); if (autoinc_fieldno !=3D UINT32_MAX) { sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, Your initial diff led to assertion fault since = def->fields[i].nullable_action and tab->def->fields[i].nullable_action had different values - DEFAULT and = NONE respectively. So I just replaced all usages of tab->def->=E2=80=A6 to simply def->=E2=80= =A6 Why they had different nullable actions? _sql_stat4 is created from Lua = and has no specified is_nullable property, so nullable action is set to DEFAULT: alter.cc (373 lie) if (is_action_missing) { field->nullable_action =3D field->is_nullable ? ON_CONFLICT_ACTION_NONE : ON_CONFLICT_ACTION_DEFAULT; } For SQL-land tables nullable action is never set to DEFAULT: it is either ABORT or NONE. In fact, DEFAULT makes no sense, so it is always substituted with ABORT (field_def_create_for_pk() in = build.c). Hence, I guess setting default nullable action to DEFAULT in alter.cc = seems to be messy and should be changed to ABORT. DEFAULT is really helpful for statement action (e.g. INSERT OR REPLACE): in this case it allows to tell INSERT OR ABORT (ABORT action) from simple INSERT (DEFAULT action). Do you agree with me? Btw now I see in trunk commit: = https://github.com/tarantool/tarantool/commit/db37dd2d663e23d55fe2caecbf91= 1e822e9182d4 IMHO it makes things even worse...