[patches] [autoinc 2/2] sql: regenerate opcodes after fixing autoinc

n.pettik korablev at tarantool.org
Thu Feb 15 03:06:14 MSK 2018


Some tests are failed on your branch:
sql/update-with-nested-select.test.lua                          [ fail ]

Also, squash two commits into one: there is no need for
separe commit which does nothing but updates opcodes.

>+	int space_id = -1;	/* space id of table */
>+	space_id = (int) SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
>+

Why do you declare variable as int and then make cast?
Make it to be uint32_t. Moreover, I would defer computing space id
untill its real usage (after several hundred lines).

>+/* Opcode: NextValue P1 P2 * * *

Choose better name for this opcode, like NextSequenceValue or
NextSequenceId or NextAutoincValue or etc.

>+ * Synopsis: r[P2] = next val from seq r[P1]

IMO, it should be sort of
"next values from space's sequence, where space id is r[p1]".

>+ * Get next value for space from register P1 and write it into
>+ * register P2. 

What next value for space? I guess, it should be
"Get next value from sequence attached to space with given space id."

>+ * P1 - contains space_id of necessary space with sequence
>+ * P2 - register number for output

We don't describe opcode arguments in this way.

>+ Opcode semantics assumes that space P1 has attached
>+ * sequence, otherwise assertion fault will be caused.

Sounds too brutally. I suggest to just end VDBE execution with
appropriate return code.

>+	int space_id = pOp->p1;

I would add set of assert to test pOp->p1 type and value.

>+	struct space * autoinc_space = space_by_id(space_id);

Move asterisk to variable name: struct space *space;

>+	struct sequence * space_seq = autoinc_space->sequence;

Same.

>+ pOut->n = 0;

I guess, it's too much. You set type of mem, so by definition it
can't have size of string different from 0.

>+	break;
>+}

Put empty string between opcodes.






More information about the Tarantool-patches mailing list