[tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash

["n.pettik" <korablev@tarantool.org>]

> On 26 Jun 2018, at 19:13, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> As new _trigger format contain space_id,

Typo: ‘contains’.

> we can do not

https://www.urbandictionary.com/define.php?term=Can%20Don%27t

It is better to say: ‘we can avoid storing AST pointers in HASH’.

> store AST pointers in HASH. Requested AST could be found
> by name in appropriate space.

Personally I would squash this commit with ’sql: move Triggers to server’,
but it is up to you.

> @@ -3973,10 +3973,14 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
> #endif				/* !defined(SQLITE_OMIT_CTE) */
> 
> int
> -vdbe_emit_halt_if_exists(struct Parse *parser, int space_id, int index_id,
> -			 const char *name_src, int tarantool_error_code,
> -			 const char *error_src, bool no_error)
> -{
> +vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
> +				  int index_id,
> +				  const char *name_src,

You can fit index_id and name_src in one line.

> @@ -4103,16 +4102,24 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
>  */
> void sql_trigger_finish(Parse *, TriggerStep *, Token *);
> 
> -void sqlite3DropTrigger(Parse *, SrcList *, int);
> +/**
> + * This function is called from parser to generate drop trigger
> + * VDBE code.
> + *
> + * @param parser Parser context.
> + * @param name The name of trigger to drop.
> + * @param no_err Suppress errors if the trigger already exists.
> + */
> +void vdbe_code_drop_trigger(Parse *, SrcList *, int);

void
vdbe_code_drop_trigger...

And add arg names to prototype.

> 
> /**
>  * Drop a trigger given a pointer to that trigger.
>  *
> - * @param parser Parse context.
> - * @param trigger Trigger to drop.
> + * @param parser Parser context.
> + * @param name The name of trigger to drop.
>  */
> void
> -vdbe_code_drop_trigger_ptr(struct Parse *parser, struct sql_trigger *trigger);
> +vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name);
> 
> /**
>  * Return a list of all triggers on table pTab if there exists at
> @@ -4784,8 +4791,8 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
> 
> /**
>  * Generate VDBE code to halt execution with correct error if
> - * the object with specified name is already present in
> - * specified space.
> + * the object with specified name is already present (or doesn't
> + * present is raise_if_not_exists flag is set

I don’t see such flag.

> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 4c28580..ca01e6e 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -125,11 +125,11 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
> 		const char *error_msg =
> 			tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS),
> 				   trigger_name);
> -		if (vdbe_emit_halt_if_exists(parse, BOX_TRIGGER_ID,
> -					     0, trigger_name,
> -					     ER_TRIGGER_EXISTS,
> -					     error_msg,
> -					     (no_err != 0)) != 0)
> +		if (vdbe_emit_halt_with_presence_test(parse, BOX_TRIGGER_ID, 0,
> +						      trigger_name,
> +						      ER_TRIGGER_EXISTS,
> +						      error_msg, (no_err != 0),
> +						      OP_NoConflict) != 0)
> 			goto trigger_cleanup;
> 	}
> 
> @@ -441,21 +441,36 @@ sql_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
> 	sqlite3DbFree(db, trigger);
> }
> 
> -/*
> - * This function is called to drop a trigger from the database
> - * schema.
> - *
> - * This may be called directly from the parser and therefore
> - * identifies the trigger by name.  The sql_drop_trigger_ptr()
> - * routine does the same job as this routine except it takes a
> - * pointer to the trigger instead of the trigger name.
> - */
> void
> -sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
> +vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name)

Lets drop ‘_ptr’ suffix.

> {
> -	struct sql_trigger *trigger = NULL;
> -	const char *zName;
> -	sqlite3 *db = pParse->db;
> +	sqlite3 *db = parser->db;
> +	assert(db->pSchema != NULL);
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	if (v == NULL)
> +		return;
> +	/*
> +	 * Generate code to delete entry from _trigger and
> +	 * internal SQL structures.
> +	 */
> +	int trig_name_reg = ++parser->nMem;
> +	int record_to_delete = ++parser->nMem;
> +	sqlite3VdbeAddOp4(v, OP_String8, 0, trig_name_reg, 0,
> +			  sqlite3DbStrDup(db, trigger_name), P4_DYNAMIC);
> +	sqlite3VdbeAddOp3(v, OP_MakeRecord, trig_name_reg, 1,
> +			  record_to_delete);
> +	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRIGGER_ID,
> +			  record_to_delete);
> +	if (parser->nested == 0)
> +		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> +	sqlite3ChangeCookie(parser);
> +}
> +
> +void
> +vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)

Lets call it with ’sql_’ prefix - this function is invoked from parser
(see for instance sql_drop_table(), sql_drop_index() etc).

> @@ -562,24 +549,19 @@ sql_trigger_replace(struct sqlite3 *db, const char *name,
> 			trigger->tr_tm = TRIGGER_AFTER;
> 	}
> 
> -
> -	*old_trigger = sqlite3HashInsert(hash, name, trigger);
> -	if (*old_trigger == trigger) {
> -		diag_set(OutOfMemory, sizeof(struct HashElem),
> -			 "sqlite3HashInsert", "ret");
> -		return -1;
> +	struct sql_trigger **ptr = &space->sql_triggers;
> +	while (*ptr != NULL && strcmp((*ptr)->zName, name) != 0)
> +		ptr = &((*ptr)->next);
> +	if (*ptr != NULL) {
> +		*old_trigger = *ptr;
> +		*ptr = (*ptr)->next;
> 	}
> 
> -	if (*old_trigger != NULL) {
> -		struct sql_trigger **pp;
> -		for (pp = &space->sql_triggers; *pp != *old_trigger;
> -		     pp = &((*pp)->next));
> -		*pp = (*pp)->next;
> -	}
> 	if (trigger != NULL) {
> 		trigger->next = space->sql_triggers;
> 		space->sql_triggers = trigger;
> 	}
> +

Redundant diff.