From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 827D2225CE for ; Wed, 27 Jun 2018 11:57:15 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id p20FD1g_Wllv for ; Wed, 27 Jun 2018 11:57:15 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0F5282248B for ; Wed, 27 Jun 2018 11:57:14 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name From: "n.pettik" In-Reply-To: <81986d1f9307191bd3d3e37514dc32fadb7e6970.1530029141.git.kshcherbatov@tarantool.org> Date: Wed, 27 Jun 2018 18:57:12 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <81986d1f9307191bd3d3e37514dc32fadb7e6970.1530029141.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov 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 =E2=80=99sql: move Triggers to = server =E2=80=98, 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 =3D > - 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 =E2=80=99table=E2=80=99 and =E2=80=98onconf=E2=80=99 args in = one line. > @@ -3018,21 +3019,23 @@ struct Parse { > */ >=20 > /* > - * 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 = trigger, > the 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 =3D pParse->parsed_ast.trigger; > - char *zName; /* Name of trigger */ > - char *zSql =3D 0; /* SQL text */ > - char *zOpts =3D 0; /* MsgPack containing SQL options */ > - sqlite3 *db =3D pParse->db; /* The database */ > - DbFixer sFix; /* Fixer object */ > - Token nameToken; /* Trigger name for error reporting */ > - > - pParse->parsed_ast.trigger =3D NULL; > - if (NEVER(pParse->nErr) || !pTrig) > + struct sql_trigger *trigger =3D parse->parsed_ast.trigger; > + /* Name of trigger. */ > + char *trigger_name; You don=E2=80=99t need to declare all variables beforehand: its obsolete = style used in SQLite. > + /* SQL text. */ > + char *sql_str =3D NULL; > + /* MsgPack containing SQL options. */ > + char *opts_buff =3D NULL; > + /* The database. */ > + struct sqlite3 *db =3D parse->db; > + > + parse->parsed_ast.trigger =3D NULL; > + if (NEVER(parse->nErr) || trigger =3D=3D NULL) > goto triggerfinish_cleanup; > - zName =3D pTrig->zName; > - pTrig->step_list =3D pStepList; > - while (pStepList) { > - pStepList->pTrig =3D pTrig; > - pStepList =3D pStepList->pNext; > + trigger_name =3D trigger->zName; > + trigger->step_list =3D step_list; > + while (step_list !=3D NULL) { > + step_list->trigger =3D trigger; > + step_list =3D 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 =E2=80=98or=E2=80=99 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 =3D pSubParse; > - pSubParse->pTriggerTab =3D pTab; > + pSubParse->pTriggerTab =3D table; > pSubParse->pToplevel =3D pTop; > - pSubParse->eTriggerOp =3D pTrigger->op; > - pSubParse->nQueryLoop =3D pParse->nQueryLoop; > + pSubParse->eTriggerOp =3D trigger->op; > + pSubParse->nQueryLoop =3D parser->nQueryLoop; >=20 > - v =3D 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 =3D sqlite3GetVdbe(pParse); /* Main VM */ > - TriggerPrg *pPrg; > - struct session *user_session =3D current_session(); > + /* Main VM. */ > + struct Vdbe *v =3D sqlite3GetVdbe(parser); >=20 > - pPrg =3D getRowTrigger(pParse, p, pTab, orconf); > - assert(pPrg || pParse->nErr || pParse->db->mallocFailed); > + TriggerPrg *pPrg =3D sql_row_trigger(parser, trigger, table, = orconf); > + assert(pPrg !=3D NULL || parser->nErr !=3D 0 || > + parser->db->mallocFailed !=3D 0); >=20 > - /* 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 =3D (p->zName && > - 0 =3D=3D > - (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 =3D=3D NULL) > + return; >=20 > - /* 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 =3D current_session(); > + bool recursive =3D (trigger->zName && !(user_session->sql_flags = & > + SQLITE_RecTriggers)); We call bool variables with =E2=80=98is_=E2=80=99 or =E2=80=98if_=E2=80=99= 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 =3D pChanges ? TK_UPDATE : TK_DELETE; > + const int op =3D changes_list !=3D NULL ? TK_UPDATE : TK_DELETE; > u32 mask =3D 0; > - Trigger *p; >=20 > - assert(isNew =3D=3D 1 || isNew =3D=3D 0); > - for (p =3D pTrigger; p; p =3D p->pNext) { > + assert(new =3D=3D 1 || new =3D=3D 0); We call bool variables with =E2=80=98is_=E2=80=99 or =E2=80=98if_=E2=80=99= prefixes, as a rule. >=20 >=20