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

[Kirill Shcherbatov]

Hi again!
I've rebased my patch on Kirill's Y. branch kyukhin/gh-3482-remove-ipkey;
I've also have succeed in using  ON_CONFLICT_ACTION_DEFAULT instead of on_conflict_action_MAX.
This changes represented as "[draft] don't use on_conflict_default_MAX" commit on my branch.
I've have had to introduce small rule index_onconf that looks very similar with onconf. If I not mistaken,
you are going to get rid of Index on_conflict action; and this rudiment crap would disappear. 

On 20.07.2018 10:29, Kirill Shcherbatov wrote:
>>>> Why do you need on_conflict_action_MAX when you already have ON_CONFLICT_ACTION_DEFAULT?
>>>> Anyway, there is no action DEFAULT, it is sooner or later converted to ABORT.
>>> This is the central idea of the patch.  on_conflict_action_MAX is a marker that this field wasn't
>>> initialized yet manually. This allows to detect second attempt to specify NULL/NOT NULL etc.
>>> There is a comment about this concept in sqlite3AddColumn where on_conflict_action_MAX is set.
>>> The default behavior is  ON_CONFLICT_ACTION_NONE and we have to distinguish non-initialized
>>> columns and initialized with ON_CONFLICT_ACTION_DEFAULT.
>>
>> Well, it seems to be total mess. DEFAULT (for nullable on conflict action)
>> is always converted into ABORT. If you move this conversation right after
>> parser’s pass, you can get rid off using additional enum value:
>>
>> create table t1(a NULL PRIMARY KEY NOT NULL , b);
>>
>> a == DEFAULT —> NONE —> ABORT
>> b == DEFAULT —> NONE
> Please, read my previous message again. 
> 
> I need to distinguish no-initialized and initialized columns.
> +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);')
> +---
> +- error: 'SQL error: NULL declaration for column ''S1'' of table ''TE17'' has been
> +    already set to ''none'''
> 
> I need a marker. I've tried to use ON_CONFLICT_ACTION_DEFAULT in this role, but it looks not
> workable until Index->onError present: onconf parse.y rule should be universal.
> 
> Let's discuss this verbally if you need.
> 
>