[tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes

[Kirill Yukhin <kyukhin@tarantool.org>]

Hello Nikita,

My answers are inlined.
Updated patch in the bottom. Branch force-pushed.

On 21 авг 23:38, n.pettik wrote:
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index dc5146f..821c7a9 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -4625,13 +4625,35 @@ case OP_RenameTable: {
> > 	db->init.busy = 1;
> > 	init.rc = SQLITE_OK;
> > 	sql_init_callback(&init, zNewTableName, space_id, 0, zSqlStmt);
> > -	db->init.busy = 0;
> > 	rc = init.rc;
> > -	if (rc) {
> > +	if (rc != SQLITE_OK) {
> > 		sqlite3CommitInternalChanges();
> > +		db->init.busy = 0;
> > 		goto abort_due_to_error;
> > 	}
> > 
> > +	/* Space was altered, refetch the pointer. */
> > +	space = space_by_id(space_id);
> > +	for (uint32_t i = 0; i < space->index_count; ++i) {
> > +		struct index_def *def = space->index[i]->def;
> > +		if (def->opts.sql == NULL)
> > +			continue;
> > +		char *sql_stmt;
> > +		rc = sql_index_update_table_name(def, zNewTableName, &sql_stmt);
> 
> Where do you free memory for sql_stmt? table_rename allocates new
> string using sqlite3DbMalloc AFAIK.
Yes, it is. This is a clear leak. Fixed.
 
> > +		if (rc != SQLITE_OK)
> 
> rc != 0 (or better: if (sql_index_update_table_name() != 0)).
> 
> > +			goto abort_due_to_error;
> > +		space = space_by_id(space_id);
> > +		sql_init_callback(&init, zNewTableName, space_id,
> > +				  space->index[i]->def->iid, sql_stmt);
> > +		rc = init.rc;
> > +		if (rc != SQLITE_OK) {
> 
> rc != 0 (or better: if (init.rc != 0)
Fixed.

Here's iterative diff:
1 file changed, 4 insertions(+), 3 deletions(-)
src/box/sql/vdbe.c | 7 ++++---

modified   src/box/sql/vdbe.c
@@ -4663,14 +4663,15 @@ case OP_RenameTable: {
                         continue;
                 char *sql_stmt;
                 rc = sql_index_update_table_name(def, zNewTableName, &sql_stmt);
-                if (rc != SQLITE_OK)
+                if (rc != 0)
                         goto abort_due_to_error;
                 space = space_by_id(space_id);
                 sql_init_callback(&init, zNewTableName, space_id,
                                   space->index[i]->def->iid, sql_stmt);
-                rc = init.rc;
-                if (rc != SQLITE_OK) {
+                sqlite3DbFree(db, sql_stmt);
+                if (init.rc != 0) {
                         sqlite3CommitInternalChanges();
+                        rc = init.rc;
                         db->init.busy = 0;
                         goto abort_due_to_error;
                 }

> 
> The rest seems to be ok.
> 

Thanks! I'll check updated patch into 2.0 branch.

--
Regards, Kirill Yukhin