[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! See 16 comments below.

On 31/01/2019 20:32, n.pettik wrote:
> I’ve took into consideration your comments and pushed
> new version of this patch:
> 
> Branch: np/gh-3914-fix-create-index-v2
> 
> If it is OK (except for minor things mb) I will add it to
> the main patch-set and rebase the rest.
> 
> Author: Nikita Pettik <korablev@tarantool.org>
> Date:   Wed Jan 9 12:28:09 2019 +0200
> 
>      sql: introduce structs assembling DDL arguments during parsing
>      
>      Parser's rules implementing DDL have a lot in common. For instance,
>      to drop any entity it is enough to know its name and name of table it is
>      related to.
>      Thus, it was suggested to arrange arguments of DDL rules into
>      hierarchical structure. The root of chain always includes name of table
>      to be altered: all existing entities are related to some table.  Then
>      comes one of drop/create/rename rules. Indeed, each DDL operation can be
>      classified in these terms, at least until we introduce ALTER TABLE ALTER
>      CONSTRAINT statement. Drop is represented by single structure (as it was
>      mentioned); rename can be applied only to table; create can be applied
>      to CONSTRAINT (indexes are considered as constraints) and TRIGGER, which
>      in turn are different in arguments required to create those objects. And
>      so forth.
>      
>      What is more, we are going to introduce ALTER TABLE ADD CONSTRAINT
>      UNIQUE With new hierarchy we can extend ALTER TABLE statement with ease:
>      basic structures (alter -> create entity -> create constraint) are the
>      same for .. FOREIGN KEY/UNIQUE, but the last one will be different.
>      
>      Patch itself is made up of refactoring; no functional changes are
>      provided.
>      
>      Needed for #3097
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index f92f39d8e..b6f099cf5 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -383,18 +383,16 @@ sql_table_new(Parse *parser, char *name)
>    * when the "TEMP" or "TEMPORARY" keyword occurs in between
>    * CREATE and TABLE.
>    *
> - * The new table record is initialized and put in pParse->pNewTable.
> + * The new table record is initialized and put in pParse->new_table.

1. No member new_table in struct Parse.

>    * As more of the CREATE TABLE statement is parsed, additional action
>    * routines will be called to add more information to this record.
>    * At the end of the CREATE TABLE statement, the sqlite3EndTable() routine
>    * is called to complete the construction of the new table record.
>    *
>    * @param pParse Parser context.
> - * @param pName1 First part of the name of the table or view.
> - * @param noErr Do nothing if table already exists.
>    */
>   void
> -sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
> +sqlite3StartTable(struct Parse *pParse)
>   {
>   	Table *pTable;
>   	char *zName = 0;	/* The name of the new table */
> @@ -403,8 +401,9 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>   	if (v == NULL)
>   		goto cleanup;
>   	sqlite3VdbeCountChanges(v);
> -
> -	zName = sqlite3NameFromToken(db, pName);
> +	struct Token name =
> +		((struct create_entity_def *) &pParse->create_table_def)->name;
> +	zName = sqlite3NameFromToken(db, &name);

2. Looks shorter, no?

  	if (v == NULL)
  		goto cleanup;
  	sqlite3VdbeCountChanges(v);
-	struct Token name =
-		((struct create_entity_def *) &pParse->create_table_def)->name;
+	struct Token name = pParse->create_table_def.base.name;
  	zName = sqlite3NameFromToken(db, &name);
  
  	if (zName == 0)

>   
>   	if (zName == 0)
>   		return;
> @@ -1788,12 +1795,14 @@ columnno_by_name(struct Parse *parse_context, const struct space *space,
>   }
>   
>   void
> -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> -		       struct Token *constraint, struct ExprList *child_cols,
> -		       struct Token *parent, struct ExprList *parent_cols,
> -		       bool is_deferred, int actions)
> +sql_create_foreign_key(struct Parse *parse_context)
>   {
>   	struct sqlite3 *db = parse_context->db;
> +	struct create_fk_def create_fk_def = parse_context->create_fk_def;
> +	struct create_constraint_def create_constr_def = create_fk_def.base;
> +	struct create_entity_def create_def = create_constr_def.base;
> +	struct alter_entity_def alter_def = create_def.base;

3. Please, do not copy these structs onto the stack. They are too big.
The same in all other places.

> +	assert(alter_def.entity_type == ENTITY_TYPE_FK);
>   	/*
>   	 * When this function is called second time during
>   	 * <CREATE TABLE ...> statement (i.e. at VDBE runtime),
> @@ -2211,19 +2227,29 @@ constraint_is_named(const char *name)
>   }
>   
>   void
> -sql_create_index(struct Parse *parse, struct Token *token,
> -		 struct SrcList *tbl_name, struct ExprList *col_list,
> -		 MAYBE_UNUSED struct Token *start, enum sort_order sort_order,
> -		 bool if_not_exist, enum sql_index_type idx_type) {
> +sql_create_index(struct Parse *parse) {
>   	/* The index to be created. */
>   	struct index *index = NULL;
>   	/* Name of the index. */
>   	char *name = NULL;
>   	struct sqlite3 *db = parse->db;
>   	assert(!db->init.busy);
> +	struct create_index_def create_idx_def = parse->create_index_def;
> +	struct create_entity_def *create_entity_def =
> +		(struct create_entity_def *) &create_idx_def;

4. Please, use 'base'. Code would be smaller and less erroneous than casts.
Or at least be consistent and use everywhere either cast or base.

> +	struct alter_entity_def alter_entity_def = create_entity_def->base;
> +	assert(alter_entity_def.entity_type == ENTITY_TYPE_INDEX);
> +	/*
> +	 * Get list of columns to be indexed. It will be NULL if
> +	 * this is a primary key or unique-constraint on the most
> +	 * recent column added to the table under construction.
> +	 */
> +	struct ExprList *col_list = create_idx_def.cols;
> +	struct SrcList *tbl_name = alter_entity_def.entity_name;
>   
>   	if (db->mallocFailed || parse->nErr > 0)
>   		goto exit_create_index;
> +	enum sql_index_type idx_type = create_idx_def.idx_type;
>   	if (idx_type == SQL_INDEX_TYPE_UNIQUE ||
>   	    idx_type == SQL_INDEX_TYPE_NON_UNIQUE) {
>   		Vdbe *v = sqlite3GetVdbe(parse);
> @@ -2299,11 +2324,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
>   		}
>   	} else {
>   		char *constraint_name = NULL;
> -		if (parse->constraintName.z != NULL)
> +		if (create_entity_def->name.n > 0)

5. In struct Token comment you wrote/copy-pasted, that
if z == NULL, n is undefined. So here n > 0 does
not mean that z != NULL. Or the comment was wrong?

>   			constraint_name =
>   				sqlite3NameFromToken(db,
> -						     &parse->constraintName);
> -
> +						     &create_entity_def->name);
>   	       /*
>   		* This naming is temporary. Now it's not
>   		* possible (since we implement UNIQUE
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 0bcf41594..e8b053361 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -168,7 +168,9 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). {
>   //
>   cmd ::= create_table create_table_args.
>   create_table ::= createkw TABLE ifnotexists(E) nm(Y). {
> -   sqlite3StartTable(pParse,&Y,E);
> +  alter_entity_def_init(&pParse->create_table_def, NULL, ENTITY_TYPE_TABLE);
> +  create_entity_def_init(&pParse->create_table_def, Y, E);

6. We never use 'init' suffix. Please, use 'create'.

> +  sqlite3StartTable(pParse);
>   }
>   createkw(A) ::= CREATE(A).  {disableLookaside(pParse);}
>   
> @@ -237,8 +239,15 @@ nm(A) ::= id(A). {
>   carglist ::= carglist cconsdef.
>   carglist ::= .
>   cconsdef ::= cconsname ccons.
> -cconsname ::= CONSTRAINT nm(X).           {pParse->constraintName = X;}
> -cconsname ::= .                           {pParse->constraintName.n = 0;}
> +cconsname ::= cconsname_start cconsname_parse .
> +cconsname_start ::= . {
> +  /* Prepare base members for re-usage. */
> +  memset(&pParse->create_index_def, 0, sizeof(struct create_index_def));
> +}
> +cconsname_parse ::= CONSTRAINT nm(X). {
> +  create_entity_def_init(&pParse->create_index_def, X, false);

7. Why in case of any constraint start, you reset only index_def? UNIQUE/PK
are not the only existing constraints. Also, memset(0) is not scalable
solution - you need a separate reset() function, or something like that in
case if in future parse_def.h structures will grow in size and complexity,
and 0 becomes not default value of some attribute.

By the way, 0 is already not default value of entity type. Memset above
makes create_index_def ha ENTITY_TYPE_TABLE type.

> +}
> +cconsname_parse ::= .
>   ccons ::= DEFAULT term(X).            {sqlite3AddDefaultValue(pParse,&X);}
>   ccons ::= DEFAULT LP expr(X) RP.      {sqlite3AddDefaultValue(pParse,&X);}
>   ccons ::= DEFAULT PLUS term(X).       {sqlite3AddDefaultValue(pParse,&X);}
> @@ -260,14 +269,29 @@ ccons ::= NULL onconf(R).        {
>           sql_column_add_nullable_action(pParse, R);
>   }
>   ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
> -ccons ::= PRIMARY KEY sortorder(Z) autoinc(I).
> -                                 {sqlite3AddPrimaryKey(pParse,0,I,Z);}
> -ccons ::= UNIQUE.                {sql_create_index(pParse,0,0,0,0,
> -                                                   SORT_ORDER_ASC, false,
> -                                                   SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);}
> -ccons ::= CHECK LP expr(X) RP.   {sql_add_check_constraint(pParse,&X);}
> -ccons ::= REFERENCES nm(T) eidlist_opt(TA) refargs(R).
> -                                 {sql_create_foreign_key(pParse, NULL, NULL, NULL, &T, TA, false, R);}
> +ccons ::= PRIMARY KEY sortorder(Z) autoinc(I). {
> +  pParse->create_table_def.has_autoinc = I;
> +  sqlite3AddPrimaryKey(pParse,0,Z);
> +}
> +ccons ::= UNIQUE. {
> +  pParse->create_index_def.idx_type = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE;
> +  entity_set_type(&pParse->create_index_def, ENTITY_TYPE_INDEX);

8. Why UNIQUE sets entity type index, but PRIMARY KEY does not?

> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> new file mode 100644
> index 000000000..7a61a27e0
> --- /dev/null
> +++ b/src/box/sql/parse_def.h
> @@ -0,0 +1,260 @@
> +#ifndef TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED
> +#define TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "box/fkey.h"
> +
> +/**
> + * This file contains auxiliary structures and functions which
> + * are used only during parsing routine (see parse.y).
> + * Their main purpose is to assemble common parts of altered
> + * entities (such as name, or IF EXISTS clause) and pass them
> + * as a one object to further functions.
> + *
> + * Hierarchy is following:
> + *
> + * Base structure is ALTER.
> + * ALTER is omitted only for CREATE TABLE since table is filled
> + * with meta-information just-in-time of parsing:
> + * for instance, as soon as field's name and type are recognized
> + * they are added to space definition.
> + *
> + * DROP is general for all existing objects and includes
> + * name of object itself, name of parent object (table),
> + * IF EXISTS clause and may contain on-drop behaviour
> + * (CASCADE/RESTRICT, but now it is always RESTRICT).
> + * Hence, it terms of grammar - it is a terminal symbol.
> + *
> + * RENAME can be applied only to table (at least now, since it is
> + * ANSI extension), so it is also terminal symbol.
> + *
> + * CREATE in turn can be expanded to nonterminal symbol
> + * CREATE CONSTRAINT or to terminal CREATE TABLE/INDEX/TRIGGER.
> + * CREATE CONSTRAINT unfolds to FOREIGN KEY or UNIQUE/PRIMARY KEY.
> + *
> + * For instance:
> + * ALTER TABLE t ADD CONSTRAINT c FOREIGN KEY REFERENCES t2(id);
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE CONSTRAINT -> CREATE FK
> + *
> + * CREATE TRIGGER tr1 ...
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE TRIGGER
> + *
> + * All terminal symbols are stored as a union within
> + * parsing context (struct Parse).
> + */
> +
> +/**
> + * Each token coming out of the lexer is an instance of
> + * this structure. Tokens are also used as part of an expression.
> + *
> + * Note if Token.z is NULL then Token.n is undefined.
> + */
> +struct Token {
> +	/** Text of the token. Not NULL-terminated! */
> +	const char *z;
> +	/** Number of characters in this token. */
> +	unsigned int n;
> +	bool isReserved;
> +};
> +
> +/**
> + * Possible SQL index types. Note that PK and UNIQUE constraints
> + * are implemented as indexes and have their own types:
> + * _CONSTRAINT_PK and _CONSTRAINT_UNIQUE.
> + */
> +enum sql_index_type {
> +	SQL_INDEX_TYPE_NON_UNIQUE = 0,
> +	SQL_INDEX_TYPE_UNIQUE,
> +	SQL_INDEX_TYPE_CONSTRAINT_UNIQUE,
> +	SQL_INDEX_TYPE_CONSTRAINT_PK,
> +	sql_index_type_MAX
> +};
> +
> +enum entity_type {
> +	ENTITY_TYPE_TABLE = 0,
> +	ENTITY_TYPE_INDEX,
> +	ENTITY_TYPE_TRIGGER,
> +	ENTITY_TYPE_CK,
> +	ENTITY_TYPE_FK,
> +	entity_type_MAX

9. These entity_types are useless. Even being used in asserts,
they do not prevent errors, allowing to create an instance of
create_entity_def, but use it as drop_entity_def, because
base.entity_type in both cases can be, for example,
ENTITY_TYPE_INDEX.

Please, use not object entity types, but def types:

     ALTER_DEF_CREATE_TABLE/DROP_TABLE/RENAME.../...INDEX.../...

Or split entity type and action, so as to have something like
this:

     def.entity = ALTER_DEF_TABLE;
     def.action = ALTER_DEF_RENAME;

By the way, please, rename ENTITY_TYPE_CK to ENTITY_TYPE_CHECK.
Three letters 'HEC' economy is not worth the name obfuscation.

> +};
> +
> +struct alter_entity_def {
> +	/** Type of topmost entity. */
> +	enum entity_type entity_type;
> +	/** As a rule it is a name of table to be altered. */
> +	struct SrcList *entity_name;
> +};
> +
> +struct rename_entity_def {
> +	struct alter_entity_def base;
> +	struct Token new_name;
> +};
> +
> +struct create_entity_def {
> +	struct alter_entity_def base;
> +	struct Token name;
> +	/** Statement comes with IF NOT EXISTS clause. */
> +	bool if_not_exist;
> +};
> +
> +struct create_table_def {
> +	struct create_entity_def base;
> +	struct Table *new_table;
> +	/**
> +	 * Number of FK constraints declared within
> +	 * CREATE TABLE statement.
> +	 */
> +	uint32_t fkey_count;
> +	/**
> +	 * Foreign key constraint appeared in CREATE TABLE stmt.
> +	 */
> +	struct rlist new_fkey;
> +	/** True, if table to be created has AUTOINCREMENT PK. */
> +	bool has_autoinc;
> +};
> +
> +struct drop_entity_def {
> +	struct alter_entity_def base;
> +	/** Name of index/trigger/constraint to be dropped. */
> +	struct Token name;
> +	/** Statement comes with IF EXISTS clause. */
> +	bool if_exist;
> +};
> +
> +struct create_trigger_def {
> +	struct create_entity_def base;
> +	/** One of TK_BEFORE, TK_AFTER, TK_INSTEAD. */
> +	int tr_tm;
> +	/** One of TK_INSERT, TK_UPDATE, TK_DELETE. */
> +	int op;
> +	/** Column list if this is an UPDATE trigger. */
> +	struct IdList *cols;
> +	/** When clause. */
> +	struct Expr *when;
> +};
> +
> +struct create_constraint_def {
> +	struct create_entity_def base;
> +	/** One of DEFERRED, IMMEDIATE. */
> +	bool is_deferred;
> +};
> +
> +struct create_ck_def {
> +	struct create_constraint_def base;
> +	/** AST representing check expression. */
> +	struct ExprSpan *expr;
> +};
> +
> +struct create_fk_def {
> +	struct create_constraint_def base;
> +	struct ExprList *child_cols;
> +	struct Token *parent_name;
> +	struct ExprList *parent_cols;
> +	/**
> +	 * Encoded actions for MATCH, ON DELETE and
> +	 * ON UPDATE clauses.
> +	 */
> +	int actions;
> +};
> +
> +struct create_index_def {
> +	struct create_constraint_def base;
> +	/** List of indexed columns. */
> +	struct ExprList *cols;
> +	/** One of _PRIMARY_KEY, _UNIQUE, _NON_UNIQUE. */
> +	enum sql_index_type idx_type;
> +	enum sort_order sort_order;
> +};
> +
> +/**
> + * In those functions which accept void * instead of certain type
> + * it was made on purpose: it allows to avoid explicit cast before
> + * passing parameter to function. Hence, we can invoke it like:
> + * entity_set_type(create_fk_def, ...); instead of
> + * entity_set_type((struct alter_entity_def *) create_fk_def, ...);
> + */
> +static inline void
> +entity_set_type(void *entity_def, enum entity_type type)
> +{
> +	struct alter_entity_def *alter_def =
> +		(struct alter_entity_def *) entity_def;
> +	alter_def->entity_type = type;
> +}

10. Please, do not use void * in either of these functions. Use
normal types. Also, entity_set_type() should not exist as a separate
function. Instead, each terminal structure above should have it own
create() function, which internally sets type.

> +
> +static inline void
> +entity_set_defer_mode(void *entity_def, bool is_deferred)
> +{
> +	struct create_constraint_def *constr_def =
> +		(struct create_constraint_def *) entity_def;
> +	constr_def->is_deferred = is_deferred;
> +}

11. Same. You unsafely cast void * to create_constraint_def *.
Also, you have a special setter for is_deferred, but have no
for if_exist. And nonetheless you set if_exist manually in at
least one place. Why?

> +
> +static inline void
> +alter_entity_def_init(void *entity_def, struct SrcList *entity_name,
> +		      enum entity_type type)

12. This function should not be called by user. It should be part of
terminal def create() functions. It looks really weird when you do
something like this:

     alter_entity_def_init(&pParse->create_fk_def)
     create_entity_def_init(&pParse->create_fk_def)
     entity_set_defer_mode(&pParse->create_fk_def)

So basically you called three!!! constructors. It is not scalable,
sorry.

> +{
> +	struct alter_entity_def *alter_def =
> +		(struct alter_entity_def *) entity_def;
> +	alter_def->entity_name = entity_name;
> +	alter_def->entity_type = type;
> +}
> +
> +static inline void
> +create_entity_def_init(void *entity_def, struct Token name, bool if_not_exist)

13. You if_not_exist passed through the constructor of create_entity, but
you do not do the same for is_exist and drop_entity. Why? I see the function
below, but parse.y:381 directly changes if_exist.

> +{
> +	struct create_entity_def *create_def =
> +		(struct create_entity_def *) entity_def;
> +	create_def->name = name;
> +	create_def->if_not_exist = if_not_exist;
> +}
> +
> +static inline void
> +drop_entity_def_init(struct drop_entity_def *drop_def, struct Token name,
> +		     bool if_exist)
> +{
> +	drop_def->name = name;
> +	drop_def->if_exist = if_exist;
> +}
> +
> +static inline void
> +create_fk_def_init(struct create_fk_def *fk_def, struct ExprList *child_cols,
> +		   struct Token *parent_name, struct ExprList *parent_cols,
> +		   int actions)
> +{
> +	assert(fk_def != NULL);
> +	fk_def->child_cols = child_cols;
> +	fk_def->parent_name = parent_name;
> +	fk_def->parent_cols = parent_cols;
> +	fk_def->actions = actions;
> +}
> +
> +#endif /* TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED */
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 824578e45..3343c2942 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -273,7 +273,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
>   {
>   	memset(parser, 0, sizeof(struct Parse));
>   	parser->db = db;
> -	rlist_create(&parser->new_fkey);
> +	rlist_create(&parser->create_table_def.new_fkey);

14. Please, introduce create_table_def_create() in parse_def.h and do
all necessary actions there.

>   	rlist_create(&parser->record_list);
>   	region_create(&parser->region, &cord()->slabc);
>   }
> @@ -287,7 +287,7 @@ sql_parser_destroy(Parse *parser)
>   	sqlite3DbFree(db, parser->aLabel);
>   	sql_expr_list_delete(db, parser->pConstExpr);
>   	struct fkey_parse *fk;
> -	rlist_foreach_entry(fk, &parser->new_fkey, link)
> +	rlist_foreach_entry(fk, &parser->create_table_def.new_fkey, link)
>   		sql_expr_list_delete(db, fk->selfref_cols);

15. create_table_def_destroy().

>   	if (db != NULL) {
>   		assert(db->lookaside.bDisable >=
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 6462467bc..e66f0d6db 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1616,9 +1616,9 @@ void
>   sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
>   			   struct Expr *expr, struct ExprList *expr_list)
>   {
> -	/* Fake SrcList for parser->pNewTable */
> +	/* Fake SrcList for parser->new_table */
>   	SrcList sSrc;
> -	/* Name context for parser->pNewTable */
> +	/* Name context for parser->new_table */

16. No such member in struct Parse.

>   	NameContext sNC;
>   
>   	assert(type == NC_IsCheck || type == NC_IdxExpr);