[tarantool-patches] Re: [PATCH v3 08/10] sql: new _trigger space format with space_id

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

Thanks for the fixes! See 2 comments below.

Besides, I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> As we would like to lookup triggers by space_id in future
> on space deletion to delete associated _trigger tuples we
> need to introduce new field space_id as secondary key.
> 
> Part of #3273.
> ---
>   src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
>   src/box/lua/upgrade.lua                            |   4 +++
>   src/box/schema_def.h                               |   6 ++++
>   src/box/sql.c                                      |  21 +++++++-----
>   src/box/sql/sqliteInt.h                            |   2 ++
>   src/box/sql/trigger.c                              |   8 +++--
>   test/app-tap/tarantoolctl.test.lua                 |   2 +-
>   test/box-py/bootstrap.result                       |   5 +--
>   test/box/access_misc.result                        |   4 +--
>   test/box/access_sysview.result                     |   4 +--
>   test/box/alter.result                              |   1 +
>   test/sql/gh2141-delete-trigger-drop-table.result   |   4 +--
>   test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +--
>   test/sql/persistency.result                        |   8 ++---
>   test/sql/persistency.test.lua                      |   8 ++---
>   test/sql/upgrade.result                            |  37 ++++++++++++++++++---
>   test/sql/upgrade.test.lua                          |   9 ++++-
>   17 files changed, 91 insertions(+), 36 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index d271dca7bc6030f6a05c28c6385ed01a0a02f700..a8a00ec29e7106afbd06294cf6ffe291f90a2e10 100644
> GIT binary patch
> delta 1694
> zcmV;P24VT44X6!}9)B}5W;ZrBISNT`b97;DV`VxZVq`I4G%`73EjBVRG%YkYH!&?Z
> zW-vD`GG;kpI50CYF*i6k3RXjGZ)0mZAbWiZ3e~y`y3GbK0M4rK@!O>U00000D77#B
> z08l+K04hVU4oT1$Hvs@&JireJkQ`J#QC_IkDFq@CiO~a`bblj<f!dUr5pAcW$;}j#
> zB<X3(6|NP4kJNOv>$ELmxPvu%zAcout22V@@+qYh(*Vl=;{f7}eD(eMXFOG}!_0MA
> zQ{^T>M0(5}i{c1=M)gI&wbZ2YVhW35sXv~Nz7(<Rd}$mrUr!|VEv#js&b#y3ldmod
> zf)$}az4fgVw||}m@{d+sDw2{7Ccw4Sd~Mvyo20RNsDF1}Gm^p2$O5>Qnnjr8NZ}$@
> zUp0bRm$XW>z1(Z5NvqXk=KEc=B-c_isUoTJ2ZUR*dM!1-%Q<nNXZjQlS2A&)SMqXl
> zOeK)R!6gtY@J}Yw2}73sotGj=gKaLA(km`OP_%4Waet%=mK`eqEDZ`TuDnT<gKMez
> z;fvXoH(B&`@Q0+vqyMpMyhEBRZ}K~<*I;NrFFOX;Qj^t7JhY#e-&r}AB%m%=u5VUt
> z7F<isvOiA(>T>m+wMGHamSAJ@%rPRMSLyuCDh+}quFt5>9Ozi}J*gB7TuV)jUmVAj
> z2)>>LVt?@L8+U#JVVzsC{o!|=Y2<e<TcW@8dHxkunF2qbUHrvqu8&q@2;jZ(iCcSI
> z%|a}$&aTT5{8ZxS)sfDj{+W^oG0DGKg&A-yHA&ox@lK915rbnvcwj(U)G7-cD|%E^
> zo)kPNs?Ls%4(bHT1gZpVW<3(r8*wc)FARw%qJKkmUU%X`a3DC7O^ywYkWE*^)qJ>?
> znnE=<TM4##py>dk;V@$}Q!}%H#W9P52AK>p7-TNQScutTXsm^)P+>w-D$EQFg<4-^
> zuBGP71PT+P@}*DK<;=+c?lS69k%%@L3D;6{V-bU73-g&u)G?peF(Oltn9gNDsvHwj
> zJ%849-1#T^@Xx3Mfl+<O`}Kz(&rXh@R2&hmr6#OnP?x)ZMYRXQwbZO33B$bmSF1b^
> zuBB!P`)XAuo_FfH9L~e26;A2It-MlqI+;o)4kttjMB$ilr%GYSQem5pkP1O65S{zO
> z_V_^c=y=%d*mTU~s4+OMrKWwUNT<C~^?#T<sLNH&)}hU_k(88lrPjEXnlSYFepl9I
> zjmEXq1Yl_$+PoS2$&#P7f($T=<nN<;W4t(S8JNXXNzSSr+Bg_Gw^aD7^~JT+ED^3)
> z<co_tFIa#bYWVn{myFJlFMi&|a!Khhi<d~u0*j37=cTOkFT=Cap^fXgmYO>`V1LBw
> zxYGG%KQC`?fW}mmT8Fk87Xe^M0)a@V;aFr43CsWhfB+}}@dXDd=ZXdrfUr1<!!VA4
> z7=}Xtj6xGfKp-dx2{b^pU^HXwVBKS4p%&1X|4*aC=rB5r4iKkj+pXF1yo`v%b|Iu9
> zdofA^fdyFzLvnw7cVbd<_StLc@_#R^s5l<^p4W;`8)7qS9>v{|TchhZB_P^);=ymZ
> zIl7E^Uvlc<%`Fj-7xknjySbbQ4f!DO#f(gG#AT(F2pzK&f)RTwwyKFODV_57>)62c
> zcU8a_L{GIZbY%Pz#>pe6#^}gK1ydGiF#eR)Gjp;Gt+E+g;bR)c>^vjr(tmsM89goz
> z1A&B6?YN%{OOg{CnHyC3+<)`5__2GC1PwVg^uSGQF#eR)Gjp;GO~6}!wtV)haNl!i
> zV3weV8pz9g_D>aT=wIklyeHtL&G749Jsmxug;UC;VbMR~u&zFKv>o>O!QR-KmFgwy
> z50A^=RP+dFcJSHQk?!c?`F}z%LBKyYQbqk3bpDiyd|?vZ^C0TiDxb<tl1w`#F_kZ@
> zj|Ta;4hk`H!92Ly7#DV+#&F9_ZH8^mDu2j1dRS2>UXm?dxrK1*3vmvJgznO%+@MU9
> z&FbjovR)z~e@Aib@JDa0jX`Rx;muN>Oq+mTOc!e{6IVo}y#OjS&VNwbdcc{Hozr7r
> zK=WtO9rs}jIa>?olPm@&{Ada=x%f*h`1(<QBQE@pu!7r`tUXO;d_=|Ip@}+^Y_!rV
> zxw&QFHhIOe`ahl%;E{JMQbFlmJc$@rD}kiHR_%KIk*T>JWoBLv1#B$^hO@mwpbE8P
> oJi~Ha`5lx^HFfBzk{A9#Sl5<&4hy7?2*p>)dXg|iIn@xY?G8vRL;wH)
> 
> delta 1688
> zcmV;J250%G4WbQ@9)CA7F*0H`F*r5~Np5p=VQyn(Iv{3aVKy^mG-EAfIAUZiG-EX~
> zEn+h@W-VnnF)%n{F)}n_H#iDbLu_wjYdRo%F*+bHeF_TIx(m9^1|tB@X~^8mr2qf`
> z001bpFZ}>e)i411LZl8!&{$0YfIW;-52I0K5Pk_XKqcbbL4TXX{Q$^sCLS<!$HYcN
> zrlck%?V2P)zDy=66lajMGo8{c399v6>?$}HOBd`M$VQ%~lv2_F$pGX4H4)bLYoM_F
> zUb8aS<L%2!got#Q`vt)ecSh7jfUT39@}PG?aMT^oM?Zr1Rl@Y0k+2ssd)L)8md?BL
> z+KaGG6N(L)L4V!#uJXFRg?VSAEfnbpR|;V3Bw_oy@)m8a&eFd-zY)o}GpYb=o#YTD
> zI8eBl(f>l6^=P9+oQt(}l9N^)X1@PrGh*u`lPJ=cyP)f;QClZ@Z*){1>T_(q9F%FL
> zljE6A_=ID!QT@tvJ?z<_R2T!6{hf~>w#B#>I;WKup?@S7vOr0qa)qY~fI5W&d5bCs
> zwodX{59U|iVo}%e2l2wA|FG-3S+-Z+Vpq~s1KEB)q8QjZ$yOh)Y(F2nlAc<OU_Gv0
> zU5(N#uyvASe{KZpan+S{Mgb9*U|+G!GoqmXbna?Y1_6>+XGCKT^!s&9C<OzyPEy|o
> z!_WgU?0;MM;<E4Cc^PzdcFp#)zUyogzjGQA{o~K`u86`E0Q&smFm8KwHu^%~?tRPa
> z+Tp6ky5OqpdJJ**fuC1LI<xfGjy%31?`pI(z}88M*EQo^3~AuSAs9ed#D1)}Q6CjP
> zDSA+}I6-%Kc6L-IR3%g-VzrtYF;gOJourKwNq>bvsZc4D&MD^v;)FwULt`^UgPJ+b
> ze6V$rLo+p;DaLt_=@66Q5JM9)6SJYkZ;OHkm<%u&U@p8^c;Q}TYAscuu^}n66jP!=
> zsSAv)lXRIdV?$GZ{K<N(5&7SpHd`nX5ho+T)=BOQUM#vUuYI7N{k)zL;Q)E*oEG%u
> z8Gq>Y8LHpTJMf2hM)XJN`+oQ9XLUR?IV$3W`b4mGlCgfV9(V7GXb%KiC&^;v-MbrY
> z@xazej<N1WcU)cpDIE{T2IZwrsMMQQAZ|R94G4^5`n=R<!Z?4EbMXoT78>JrfK>QF
> zeeBpCwTFjl2TBJ`4h~0*4}G~|>m;iSMSm)*4XVT3u^v~qN@ZNCMp9Ce6-vX_NtWjG
> z{jaRY+YDPL37}3Y<5DvAiz6>-g%~)4;_oABVLTXa8qT0MiZxng-06kRuJ4wV_SiZ}
> z60B8AgmIAP#R}Y^Mvn)A$>=BX1<=1*uPCV7;wJI7a1oLHe57^$BderS#+_W)I)6zX
> z957}yT<Ki1pN};)L814AQW-bXB0#K2fkUX_SY!|h%m4s@04M+v1_vp}iUtyZusDjt
> zFpPm1hGPJXLIX!YASeh4G(fdrG$SlvT|;7_7Qm4EO(KKHATo#yVAEsm)_AK7Gb$3>
> zC6gNKVl4^83lbp=-T(O3#iSJ3$A7HF$X~Uh;uz#)u2r8lgl6Wrk2@AyOZRCSK~7uM
> z8$U_)nFHdHKZX;08u1@rJKYX)NY>k3oEKG_B~q0v=#;`(!Pa9YNfM{b`_(mY`A-$}
> z1<~_Xf*rb*hH+Bl7BE_-QC}nl4hTGi<FFVh0*M&LHgHaNmCP9qI(koV9)BdwfIA?N
> zZ;bSFGLlGSl5>kVQF_`iE$e&_$E6`_1|3L&1_U0$aafELfe;}4pUpXYH+svoBAh#P
> z@dgrmFJqZ*`1%)yE8f3x#FG1UA^0m+L$`uR>aa-u*&eP2ar7N_#ALTfgH?K#gg?kF
> ze^a3!qGf?*Vh-wjTpt$XLw|CdekL=j`lI~c`SthmqPlZpGpu>+RNndo^%05{*RnB2
> zFO7q%8RNhP&=_jtJYg6`dMQfI(FYbO;yfkH?`|Oo{6Yv#k>qY&%Nvw|f>|B8+~_3|
> zhVLkk4}WCVS{_LK8r}@$lW7Cs#dNOLFma;t)C)|ZaU8W>2Amn$Ie$I|1{9wb-EsNF
> zklWfhpY&pIktb6C&Bb?W!Of5Q9&w?5gxQ?FWDwKz$4GP=9`2|!!?sp>C6rqRrY5g=
> zslUN<5+2!(#TS%Lizk_JmHd$KZ`E$DKmVEmQD)}$P@7gzU^wj80=lYQ(=+Yk+TKB%
> itF48e>+~O}byP9oun~gm$Q^t|sV4~omjl%ht?dd`FccL4

1. Please, do not send binary files.

> diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result
> index dcdb689..882bdd0 100644
> --- a/test/sql/upgrade.result
> +++ b/test/sql/upgrade.result
> @@ -79,10 +82,33 @@ box.space._space.index['name']:get('T_OUT')
>   - [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
>     [{'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
>   ...
> -box.space._trigger:get('T1T')
> +t1t = box.space._trigger:get('T1T')
> +---
> +...
> +t2t = box.space._trigger:get('T2T')
> +---
> +...
> +t1t.name
> +---
> +- T1T
> +...
> +t1t.opts
>   ---
> -- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
> -      END;'}]
> +- {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
> +    END;'}
> +...
> +t2t.name
> +---
> +- T2T
> +...
> +t2t.opts
> +---
> +- {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2);
> +    END;'}
> +...
> +assert(t1t.id == t2t.id)
> +---
> +- true

2. Please, add a test that t1t == box.space.T.id. Your current test would pass
even if t1t.id == t2t.id == -100.

>   ...
>   box.sql.execute("INSERT INTO T VALUES(1);")
>   ---