Обсуждение: 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