[tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server

[Vladislav Shpilevoy]

> @@ -965,7 +965,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
>          if (part_count == 1) {
>                  uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
>                  int reg_pk = new_tuple_reg + fieldno;
> -               if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) {
> +               if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
>                          int skip_if_null = sqlite3VdbeMakeLabel(v);
>                          if (autoinc_fieldno != 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->… to simply def->…
> 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 = 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/db37dd2d663e23d55fe2caecbf911e822e9182d4
> 
> IMHO it makes things even worse...

Thanks for the debugging for me! Sure, DEFAULT from Lua is bad idea as we
can see in this particular example. I believe, DEFAULT should never be set
from non-SQL DDL. If a field is set to is_nullable false, it is false always,
not default. So the commit db37dd2d663e23d55fe2caecbf911e822e9182d4 is wrong,
I think. Unfortunately, it was pushed without a review, as I see.