Re: [Tarantool-patches] [PATCH] sql: fix fk-constraint error on update of a self-referenced table

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

Hi! Thanks for the patch!

On 07/02/2020 17:19, Chris Sosnin wrote:
> Currently, update operation of a self-referenced table is not handled
> correctly. Without this fix, we will succeed only if the row refers to
> itself, otherwise, the nFkConstraint will be incremented. As it is
> mentioned in comments, we shouldn't do the lookup of the table on deletion,
> as long as the tuple will be modified, but it is still necessary to inspect
> it on insertion.

As someone, who didn't work with SQL code generation by myself for
quite a while, I didn't understand almost everything. What is nFkConstraint?
Why its increment is bad? Why should not we check FK on deletion, and how
a tuple can be modified after deletion?

> Closes #4661
> ---
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4661-fk-self-ref-table
> issue:https://github.com/tarantool/tarantool/issues/4661
> 
>  src/box/sql/fk_constraint.c    | 51 ++++++++++++++++++++++++++++------
>  test/sql/foreign-keys.result   | 44 +++++++++++++++++++++++++++++
>  test/sql/foreign-keys.test.lua | 12 ++++++++
>  3 files changed, 98 insertions(+), 9 deletions(-)
> 
> diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
> index 482220a95..8a329a00d 100644
> --- a/src/box/sql/fk_constraint.c
> +++ b/src/box/sql/fk_constraint.c
> @@ -225,7 +226,8 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
>  	 * we are about to increment the constraint-counter (i.e.
>  	 * this is an INSERT operation), then check if the row
>  	 * being inserted matches itself. If so, do not increment
> -	 * the constraint-counter.
> +	 * the constraint-counter. Otherwise, if this is an insertion
> +	 * during update, we must inspect the table.
>  	 *

Here is the full comment:

	 * If the parent table is the same as the child table, and
	 * we are about to increment the constraint-counter (i.e.
	 * this is an INSERT operation), then check if the row
	 * being inserted matches itself. If so, do not increment
	 * the constraint-counter. Otherwise, if this is an insertion
	 * during update, we must inspect the table.

So your amendment means that we must inspect a table only if
the inserted row didn't match itself, and it is a part of an update.

What if it didn't match itself, and it is not a part of an update?
The FK is still correctly checked. Is the comment wrong?

Also, please, keep comments inside 66 line width. Here and in other
new places.

Talking of the rest of the patch - I tried to understand anything, but
failed. FK code looked like a mess for me, and now it is not better.
Can't wrap my mind around these countless ifs. I don't understand why
should it be so complex. Please, send it to Nikita first. Perhaps he
will understand.