Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: Bulat Niatshin <niatshin@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH V2] sql: fix non-working REPLACE optimization
Date: Mon, 9 Apr 2018 15:03:09 +0300	[thread overview]
Message-ID: <9A6F0200-0C9B-41A8-8184-FFA395864139@tarantool.org> (raw)
In-Reply-To: <20180409051748.91507-1-niatshin@tarantool.org>

Hello. I have only minor remarks mostly concerning codestyle.
As for functional part of this patch, it seems to be OK.

>This patch contains fix for that, in addition
>related code was refactored and necessary comments were added.

From this message it is not clear what bug you have fixed.
I guess that this fix is for particular case, when ‘ON CONFLICT REPLACE’
is applied to PK. But it would be better to describe origins of bug in ticket/commit message.

> @@ -1038,10 +1042,12 @@ checkConstraintUnchanged(Expr * pExpr, int *aiChng)
>  *
>  *  CHECK            REPLACE      Illegal.  The results in an exception.
>  *
> - * Which action to take is determined by the overrideError parameter.
> - * Or if overrideError==ON_CONFLICT_ACTION_DEFAULT, then the pParse->onError parameter
> - * is used.  Or if pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the onError value
> - * for the constraint is used.
> + * Which action to take is determined by the override_error parameter

Nitpicking: doesn’t fit into 66 chars.

> 	int seenReplace = 0;	/* True if REPLACE is used to resolve INT PK conflict */
> 	int nPkField;		/* Number of fields in PRIMARY KEY. */
> 	u8 isUpdate;		/* True if this is an UPDATE operation */
> 	u8 bAffinityDone = 0;	/* True if the OP_Affinity operation has been run */
> 	struct session *user_session = current_session();
> +	enum on_conflict_action override_error = on_conflict->override_error;
> +	enum on_conflict_action on_error;

Nitpicking: you don’t have to declare variables
beforehand as it happens in C89.

> +		on_error = table_column_nullable_action(pTab, i);
> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = override_error;
> +		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = ON_CONFLICT_ACTION_ABORT;

Nitpicking: I guess, this else-if is redundant.
It would be enough just to use ‘else’, since you have only two variants:
on_error equals to default action or not.
Moreover, for this reason, on_error = table_column_nullable… -
is redundant function call.

> -		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
> -		       || onError == ON_CONFLICT_ACTION_ABORT
> -		       || onError == ON_CONFLICT_ACTION_FAIL
> -		       || onError == ON_CONFLICT_ACTION_IGNORE
> -		       || onError == ON_CONFLICT_ACTION_REPLACE);

I guess, this assert used to check that on_error action is not NONE.
And since you are using enum, add simple asset(on_error != …_NONE)


> -		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
> -			onError = overrideError;
> -		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> -			onError = ON_CONFLICT_ACTION_ABORT;
> +
> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = override_error;
> +		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = ON_CONFLICT_ACTION_ABORT;
> 		}

Nitpicking: the same is here: 'else-if’ can be replaced with simple ‘else’.

> 		if ((ix == 0 && pIdx->pNext == 0)	/* Condition 2 */
> -		    && onError == ON_CONFLICT_ACTION_REPLACE	/* Condition 1 */
> -		    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)	/* Condition 3 */
> +		    /* Condition 1 */
> +		    && (on_error == ON_CONFLICT_ACTION_REPLACE ||
> +			on_error == ON_CONFLICT_ACTION_IGNORE)
> +		    /* Condition 3 */
> +		    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)
> 			||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))

Nitpicking: place space after ||. Overall, this huge ‘if' looks bad-formatted.

> -		/* Generate code that executes if the new index entry is not unique */
> -		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
> -		       || onError == ON_CONFLICT_ACTION_ABORT
> -		       || onError == ON_CONFLICT_ACTION_FAIL
> -		       || onError == ON_CONFLICT_ACTION_IGNORE
> -		       || onError == ON_CONFLICT_ACTION_REPLACE);

The same is here: add simple assert(on_error != ON_CONFLICT_ACTION_NONE).

> void
> -vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
> +			       struct on_conflict *on_conflict)

Update comment at function declaration in sqliteInt.h
Also, you have noticeably changed function’s logic,
so reflect it in comments somewhere.

> -	else
> +	} else {
> 		opcode = OP_IdxInsert;
> +	}
> 

Nitpicking: don’t separate one-line ‘if-else' with brackets.

> 	u16 pik_flags = OPFLAG_NCHANGE;
> -	if (on_error == ON_CONFLICT_ACTION_IGNORE)
> +	if (override_error == ON_CONFLICT_ACTION_IGNORE)
> +		pik_flags |= OPFLAG_OE_IGNORE;
> +	if (override_error == ON_CONFLICT_ACTION_IGNORE ||
> +	    (optimized_action == ON_CONFLICT_ACTION_IGNORE &&
> +	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
> 		pik_flags |= OPFLAG_OE_IGNORE;

You can merge this ‘if’ and previous into one.

> -	else if (on_error == ON_CONFLICT_ACTION_FAIL)
> +	}
> +	else if (override_error == ON_CONFLICT_ACTION_FAIL) {

Put 'else-if’ on previous line.

  reply	other threads:[~2018-04-09 12:03 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-09  5:17 [tarantool-patches] " Bulat Niatshin
2018-04-09 12:03 ` n.pettik [this message]
2018-04-22  7:25   ` [tarantool-patches] Re: [tarantool-patches] " Bulat Niatshin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9A6F0200-0C9B-41A8-8184-FFA395864139@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=niatshin@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH V2] sql: fix non-working REPLACE optimization' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox