Обсуждение: PG 11 JIT deform failure
Hi, JIT slot_compile_deform assumes there's at least 'natts' in TupleDesc, eg /* * Iterate over each attribute that needs to be deformed, build code to * deform it. */ for (attnum = 0; attnum < natts; attnum++) { Form_pg_attribute att = TupleDescAttr(desc, attnum); but a new TupleDesc has no attribute and the caller only tests TupleDesc is not null.
Вложения
didier <did447@gmail.com> writes: > JIT slot_compile_deform assumes there's at least 'natts' in TupleDesc, eg > /* > * Iterate over each attribute that needs to be deformed, build code to > * deform it. > */ > for (attnum = 0; attnum < natts; attnum++) > { > Form_pg_attribute att = TupleDescAttr(desc, attnum); > but a new TupleDesc has no attribute and the caller only tests > TupleDesc is not null. I looked at this, but I find it quite unconvincing. Under what circumstances would we not have a correctly filled-in tupdesc here? regards, tom lane
Extensions can do it, timescaledb in this case with: INSERT INTO ... RETURNING *; Or replacing the test in llvm_compile_expr with an Assert in slot_compile_deform ?
Hi, On June 13, 2019 11:08:15 AM PDT, didier <did447@gmail.com> wrote: >Extensions can do it, timescaledb in this case with: >INSERT INTO ... RETURNING *; > >Or replacing the test in llvm_compile_expr with an Assert in >slot_compile_deform ? In that case we ought to never generate a deform expression step - core code doesn't afair. That's only done I'd there'sactually something to deform. I'm fine with adding an assert tough Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi, I searched the mailing list but found nothing. Any reason why TupleDescAttr is a macro and not a static inline? Rather than adding an Assert attached POC replace TupleDescAttr macro by a static inline function with AssertArg. It: - Factorize Assert. - Trigger an Assert in JIT_deform if natts is wrong. - Currently In HEAD src/backend/access/common/tupdesc.c:TupleDescCopyEntry() compiler can optimize out AssertArg(PointerIsValid(...)), no idea if compiling with both cassert and -O2 make sense though). - Remove two UB in memcpy when natts is zero. Note: Comment line 1480 in ../contrib/tablefunc/tablefunc.c is wrong it's the fourth column. Regards Didier On Thu, Jun 13, 2019 at 8:35 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On June 13, 2019 11:08:15 AM PDT, didier <did447@gmail.com> wrote: > >Extensions can do it, timescaledb in this case with: > >INSERT INTO ... RETURNING *; > > > >Or replacing the test in llvm_compile_expr with an Assert in > >slot_compile_deform ? > > In that case we ought to never generate a deform expression step - core code doesn't afair. That's only done I'd there'sactually something to deform. I'm fine with adding an assert tough > > Andres > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity.
Вложения
Hi, I still haven't heard an explanation why you see a problem here. On 2019-06-27 15:54:28 +0200, didier wrote: > I searched the mailing list but found nothing. Any reason why > TupleDescAttr is a macro and not a static inline? It's present in branches that can't rely on static inlines being present. Obviously we can still change it in HEAD, because there we rely on static inlien functions working (althoug we might need to surround it with #ifndef FRONTEND, if tupdesc.h is included from other headers legitimately needed from frontend code). > Rather than adding an Assert attached POC replace TupleDescAttr macro > by a static inline function with AssertArg. > It: > - Factorize Assert. > > - Trigger an Assert in JIT_deform if natts is wrong. > - Currently In HEAD > src/backend/access/common/tupdesc.c:TupleDescCopyEntry() compiler can > optimize out AssertArg(PointerIsValid(...)), no idea > if compiling with both cassert and -O2 make sense though). It's not important. > - Remove two UB in memcpy when natts is zero. I don't think it matters, but I'm not actually sure this is actually UB. It's IIRC legal to form a pointer to one after the end of an array (but not dereference, obviously), and memcpy with a 0 length byte also is legal. > Note: > Comment line 1480 in ../contrib/tablefunc/tablefunc.c is wrong it's > the fourth column. Huh, this is of very long-standing vintage. Think it's been introduced in commit a265b7f70aa01a34ae30554186ee8c2089e035d8 Author: Bruce Momjian <bruce@momjian.us> Date: 2003-07-27 03:51:59 +0000 Greetings, Andres Freund