[tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name

[n.pettik]

I see that in this patch you provide changes which are related not
only to renaming, but to refactoring of some functions, to removing
#ifdef SQLITE_OMIT_TRIGGER macros etc.

Lets mention these facts in commit message.

Moreover, if you placed this patch before ’sql: move Triggers to server ‘,
you would avoid additional diffs in alter.cc and trigger.c

> @@ -457,9 +456,10 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
> 		/* Mask of OLD.* columns in use */
> 		/* TODO: Could use temporary registers here. */
> 		uint32_t mask =
> -		    sqlite3TriggerColmask(parse, trigger_list, 0, 0,
> -					  TRIGGER_BEFORE | TRIGGER_AFTER, table,
> -					  onconf);
> +			sql_trigger_colmask(parse, trigger_list, 0, 0,
> +					    TRIGGER_BEFORE | TRIGGER_AFTER,
> +					    table,
> +					    onconf);

You can fit ’table’ and ‘onconf’ args in one line.

> @@ -3018,21 +3019,23 @@ struct Parse {
> 					 */
> 
> /*
> - * Each trigger present in the database schema is stored as an instance of
> - * struct Trigger.
> + * Each trigger present in the database schema is stored as an
> + * instance of struct sql_trigger.
>  *
>  * Pointers to instances of struct Trigger are stored in two ways.
> - * 1. In the "trigHash" hash table (part of the sqlite3* that represents the
> - *    database). This allows Trigger structures to be retrieved by name.
> - * 2. All triggers associated with a single table form a linked list, using the
> - *    pNext member of struct Trigger. A pointer to the first element of the
> - *    linked list is stored as the "pTrigger" member of the associated
> - *    struct Table.
> - *
> - * The "step_list" member points to the first element of a linked list
> - * containing the SQL statements specified as the trigger program.
> - */
> -struct Trigger {
> + * 1. In the "trigHash" hash table (part of the sqlite3* that
> + *    represents the database). This allows Trigger structures to
> + *    be retrieved by name.
> + * 2. All triggers associated with a single table form a linked
> + *    list, using the next member of struct sql_trigger. A pointer
> + *    to the first element of the linked list is stored as the
> + *    "pTrigger" member of the associated struct Table.
> + *
> + * The "step_list" member points to the first element of a linked
> + * list containing the SQL statements specified as the trigger
> + * program.
> + */
> +struct sql_trigger {
> 	char *zName;		/* The name of the trigger                        */
> 	/** The ID of space the trigger refers to. */
> 	uint32_t space_id;
> @@ -3042,7 +3045,8 @@ struct Trigger {
> 	IdList *pColumns;	/* If this is an UPDATE OF <column-list> trigger,
> 				   the <column-list> is stored here */
> 	TriggerStep *step_list;	/* Link list of trigger program steps             */
> -	Trigger *pNext;		/* Next trigger associated with the table */
> +	/** Next trigger associated with the table. */
> +	struct sql_trigger *next;
> };

I would also fix comments inside struct definition.

> @@ -4064,17 +4069,143 @@
> +void
> +sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
> +		  int op, struct IdList *columns, struct SrcList *table,
> +		  struct Expr *when, int no_err);
> +
> +/**
> + * This routine is called after all of the trigger actions have
> + * been parsed in order to complete the process of building the
> + * trigger.
> + *
> + * @param parse Parser context.
> + * @param step_list The triggered program.
> + * @param token Token that describes the complete CREATE TRIGGER.
> + */
> +void sql_trigger_finish(Parse *, TriggerStep *, Token *);

Place type of return value at separate line. Also add names of args to prototype.

> -/*
> - * This routine is called after all of the trigger actions have been parsed
> - * in order to complete the process of building the trigger.
> - */
> void
> -sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
> -		     TriggerStep * pStepList,	/* The triggered program */
> -		     Token * pAll	/* Token that describes the complete CREATE TRIGGER */
> -    )
> +sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
> +		   struct Token *token)
> {
> 	/* Trigger being finished. */
> -	Trigger *pTrig = pParse->parsed_ast.trigger;
> -	char *zName;		/* Name of trigger */
> -	char *zSql = 0;		/* SQL text */
> -	char *zOpts = 0;	/* MsgPack containing SQL options */
> -	sqlite3 *db = pParse->db;	/* The database */
> -	DbFixer sFix;		/* Fixer object */
> -	Token nameToken;	/* Trigger name for error reporting */
> -
> -	pParse->parsed_ast.trigger = NULL;
> -	if (NEVER(pParse->nErr) || !pTrig)
> +	struct sql_trigger *trigger = parse->parsed_ast.trigger;
> +	/* Name of trigger. */
> +	char *trigger_name;

You don’t need to declare all variables beforehand: its obsolete style
used in SQLite.

> +	/* SQL text. */
> +	char *sql_str = NULL;
> +	/* MsgPack containing SQL options. */
> +	char *opts_buff = NULL;
> +	/* The database. */
> +	struct sqlite3 *db = parse->db;
> +
> +	parse->parsed_ast.trigger = NULL;
> +	if (NEVER(parse->nErr) || trigger == NULL)
> 		goto triggerfinish_cleanup;
> -	zName = pTrig->zName;
> -	pTrig->step_list = pStepList;
> -	while (pStepList) {
> -		pStepList->pTrig = pTrig;
> -		pStepList = pStepList->pNext;
> +	trigger_name = trigger->zName;
> +	trigger->step_list = step_list;
> +	while (step_list != NULL) {
> +		step_list->trigger = trigger;
> +		step_list = step_list->pNext;
> 	}
> -	sqlite3TokenInit(&nameToken, pTrig->zName);
> -	sqlite3FixInit(&sFix, pParse, "trigger", &nameToken);
> -	if (sqlite3FixTriggerStep(&sFix, pTrig->step_list)
> -	    || sqlite3FixExpr(&sFix, pTrig->pWhen)
> -	    ) {
> +
> +	/* Trigger name for error reporting. */
> +	struct Token trigger_name_token;
> +	/* Fixer object. */
> +	struct DbFixer fixdb;
> +	sqlite3TokenInit(&trigger_name_token, trigger->zName);
> +	sqlite3FixInit(&fixdb, parse, "trigger", &trigger_name_token);
> +	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list)
> +	    || sqlite3FixExpr(&fixdb, trigger->pWhen))

Put ‘or’ operator at previous line:

	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
	    sqlite3FixExpr(&fixdb, trigger->pWhen))

> -/*
> +/**
>  * Create and populate a new TriggerPrg object with a sub-program
>  * implementing trigger pTrigger with ON CONFLICT policy orconf.
> + *
> + * @param parser Current parse context.
> + * @param trigger sql_trigger to code.
> + * @param table trigger is attached to.
> + * @param orconf ON CONFLICT policy to code trigger program with.
> + *
> + * @retval not NULL on success.
> + * @retval NULL on error.
>  */
> static TriggerPrg *
> -codeRowTrigger(Parse * pParse,	/* Current parse context */
> -	       Trigger * pTrigger,	/* Trigger to code */
> -	       Table * pTab,	/* The table pTrigger is attached to */
> -	       int orconf	/* ON CONFLICT policy to code trigger program with */
> -    )
> +sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
> +			struct Table *table, int orconf)

Lets call it vdbe_code_row_trigger() or smth like that.

> @@ -880,34 +868,37 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
> 	sql_parser_create(pSubParse, db);
> 	memset(&sNC, 0, sizeof(sNC));
> 	sNC.pParse = pSubParse;
> -	pSubParse->pTriggerTab = pTab;
> +	pSubParse->pTriggerTab = table;
> 	pSubParse->pToplevel = pTop;
> -	pSubParse->eTriggerOp = pTrigger->op;
> -	pSubParse->nQueryLoop = pParse->nQueryLoop;
> +	pSubParse->eTriggerOp = trigger->op;
> +	pSubParse->nQueryLoop = parser->nQueryLoop;
> 
> -	v = sqlite3GetVdbe(pSubParse);
> -	if (v) {
> +	/* Temporary VM. */

Why VM is temporary?

> +vdbe_code_row_trigger_direct(struct Parse *parser, struct sql_trigger *trigger,
> +			     struct Table *table, int reg, int orconf,
> +			     int ignore_jump)
> {
> -	Vdbe *v = sqlite3GetVdbe(pParse);	/* Main VM */
> -	TriggerPrg *pPrg;
> -	struct session *user_session = current_session();
> +	/* Main VM. */
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> 
> -	pPrg = getRowTrigger(pParse, p, pTab, orconf);
> -	assert(pPrg || pParse->nErr || pParse->db->mallocFailed);
> +	TriggerPrg *pPrg = sql_row_trigger(parser, trigger, table, orconf);
> +	assert(pPrg != NULL || parser->nErr != 0 ||
> +	       parser->db->mallocFailed != 0);
> 
> -	/* Code the OP_Program opcode in the parent VDBE. P4 of the OP_Program
> -	 * is a pointer to the sub-vdbe containing the trigger program.
> +	/*
> +	 * Code the OP_Program opcode in the parent VDBE. P4 of
> +	 * the OP_Program is a pointer to the sub-vdbe containing
> +	 * the trigger program.
> 	 */
> -	if (pPrg) {
> -		int bRecursive = (p->zName &&
> -				  0 ==
> -				  (user_session->
> -				   sql_flags & SQLITE_RecTriggers));
> -
> -		sqlite3VdbeAddOp4(v, OP_Program, reg, ignoreJump,
> -				  ++pParse->nMem, (const char *)pPrg->pProgram,
> -				  P4_SUBPROGRAM);
> -		VdbeComment((v, "Call: %s.%s", (p->zName ? p->zName : "fkey"),
> -			     onErrorText(orconf)));
> +	if (pPrg == NULL)
> +		return;
> 
> -		/* Set the P5 operand of the OP_Program instruction to non-zero if
> -		 * recursive invocation of this trigger program is disallowed. Recursive
> -		 * invocation is disallowed if (a) the sub-program is really a trigger,
> -		 * not a foreign key action, and (b) the flag to enable recursive triggers
> -		 * is clear.
> -		 */
> -		sqlite3VdbeChangeP5(v, (u8) bRecursive);
> -	}
> +	struct session *user_session = current_session();
> +	bool recursive = (trigger->zName && !(user_session->sql_flags &
> +					      SQLITE_RecTriggers));

We call bool variables with ‘is_’ or ‘if_’  prefix, as a rule.

> u32
> -sqlite3TriggerColmask(Parse * pParse,	/* Parse context */
> -		      Trigger * pTrigger,	/* List of triggers on table pTab */
> -		      ExprList * pChanges,	/* Changes list for any UPDATE OF triggers */
> -		      int isNew,	/* 1 for new.* ref mask, 0 for old.* ref mask */
> -		      int tr_tm,	/* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
> -		      Table * pTab,	/* The table to code triggers from */
> -		      int orconf	/* Default ON CONFLICT policy for trigger steps */
> -    )
> +sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
> +		    ExprList *changes_list, int new, int tr_tm,
> +		    Table *table, int orconf)
> {
> -	const int op = pChanges ? TK_UPDATE : TK_DELETE;
> +	const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE;
> 	u32 mask = 0;
> -	Trigger *p;
> 
> -	assert(isNew == 1 || isNew == 0);
> -	for (p = pTrigger; p; p = p->pNext) {
> +	assert(new == 1 || new == 0);

We call bool variables with ‘is_’ or ‘if_’ prefixes, as a rule.

> 
>