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.
Attachments:
jit_expr.patchtext/x-patch; charset=US-ASCII; name=jit_expr.patchDownload
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 0da318218f..3831ed2f5a 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -336,7 +336,7 @@ llvm_compile_expr(ExprState *state)
* function specific to tupledesc and the exact number of
* to-be-extracted attributes.
*/
- if (desc && (context->base.flags & PGJIT_DEFORM))
+ if (desc && desc->natts >= op->d.fetch.last_var && (context->base.flags & PGJIT_DEFORM))
{
LLVMValueRef params[1];
LLVMValueRef l_jit_deform;
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's actually 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
Show quoted text
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's actually 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.
Attachments:
POC_inline_TupleDescAttr.patchtext/x-patch; charset=US-ASCII; name=POC_inline_TupleDescAttr.patchDownload
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index a48a6cd757..23ef9a6a75 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -86,9 +86,6 @@ getmissingattr(TupleDesc tupleDesc,
{
Form_pg_attribute att;
- Assert(attnum <= tupleDesc->natts);
- Assert(attnum > 0);
-
att = TupleDescAttr(tupleDesc, attnum - 1);
if (att->atthasmissing)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 6bc4e4c036..850771acf5 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -115,7 +115,8 @@ CreateTupleDescCopy(TupleDesc tupdesc)
desc = CreateTemplateTupleDesc(tupdesc->natts);
/* Flat-copy the attribute array */
- memcpy(TupleDescAttr(desc, 0),
+ if (desc->natts)
+ memcpy(TupleDescAttr(desc, 0),
TupleDescAttr(tupdesc, 0),
desc->natts * sizeof(FormData_pg_attribute));
@@ -156,7 +157,8 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
desc = CreateTemplateTupleDesc(tupdesc->natts);
/* Flat-copy the attribute array */
- memcpy(TupleDescAttr(desc, 0),
+ if (desc->natts)
+ memcpy(TupleDescAttr(desc, 0),
TupleDescAttr(tupdesc, 0),
desc->natts * sizeof(FormData_pg_attribute));
@@ -274,16 +276,6 @@ TupleDescCopyEntry(TupleDesc dst, AttrNumber dstAttno,
Form_pg_attribute dstAtt = TupleDescAttr(dst, dstAttno - 1);
Form_pg_attribute srcAtt = TupleDescAttr(src, srcAttno - 1);
- /*
- * sanity checks
- */
- AssertArg(PointerIsValid(src));
- AssertArg(PointerIsValid(dst));
- AssertArg(srcAttno >= 1);
- AssertArg(srcAttno <= src->natts);
- AssertArg(dstAttno >= 1);
- AssertArg(dstAttno <= dst->natts);
-
memcpy(dstAtt, srcAtt, ATTRIBUTE_FIXED_PART_SIZE);
/*
@@ -611,13 +603,6 @@ TupleDescInitEntry(TupleDesc desc,
Form_pg_type typeForm;
Form_pg_attribute att;
- /*
- * sanity checks
- */
- AssertArg(PointerIsValid(desc));
- AssertArg(attributeNumber >= 1);
- AssertArg(attributeNumber <= desc->natts);
-
/*
* initialize the attribute fields
*/
@@ -682,11 +667,6 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
{
Form_pg_attribute att;
- /* sanity checks */
- AssertArg(PointerIsValid(desc));
- AssertArg(attributeNumber >= 1);
- AssertArg(attributeNumber <= desc->natts);
-
/* initialize the attribute fields */
att = TupleDescAttr(desc, attributeNumber - 1);
att->attrelid = 0; /* dummy value */
@@ -770,13 +750,6 @@ TupleDescInitEntryCollation(TupleDesc desc,
AttrNumber attributeNumber,
Oid collationid)
{
- /*
- * sanity checks
- */
- AssertArg(PointerIsValid(desc));
- AssertArg(attributeNumber >= 1);
- AssertArg(attributeNumber <= desc->natts);
-
TupleDescAttr(desc, attributeNumber - 1)->attcollation = collationid;
}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index d768b9b061..2fbd2ef207 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3842,7 +3842,6 @@ heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
}
else
{
- Assert(attrnum <= tupdesc->natts);
att = TupleDescAttr(tupdesc, attrnum - 1);
return datumIsEqual(value1, value2, att->attbyval, att->attlen);
}
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 77a48b039d..f34334b2a2 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2600,7 +2600,6 @@ expandTupleDesc(TupleDesc tupdesc, Alias *eref, int count, int offset,
}
}
- Assert(count <= tupdesc->natts);
for (varattno = 0; varattno < count; varattno++)
{
Form_pg_attribute attr = TupleDescAttr(tupdesc, varattno);
@@ -2837,8 +2836,6 @@ get_rte_attribute_type(RangeTblEntry *rte, AttrNumber attnum,
/* Composite data type, e.g. a table's row type */
Form_pg_attribute att_tup;
- Assert(tupdesc);
- Assert(attnum <= tupdesc->natts);
att_tup = TupleDescAttr(tupdesc, attnum - 1);
/*
@@ -3050,8 +3047,6 @@ get_rte_attribute_is_dropped(RangeTblEntry *rte, AttrNumber attnum)
/* Composite data type, e.g. a table's row type */
Form_pg_attribute att_tup;
- Assert(tupdesc);
- Assert(attnum - atts_done <= tupdesc->natts);
att_tup = TupleDescAttr(tupdesc,
attnum - atts_done - 1);
return att_tup->attisdropped;
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 00def27881..8554e42803 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1918,8 +1918,6 @@ CatCacheFreeKeys(TupleDesc tupdesc, int nkeys, int *attnos, Datum *keys)
Form_pg_attribute att;
/* system attribute are not supported in caches */
- Assert(attnum > 0);
-
att = TupleDescAttr(tupdesc, attnum - 1);
if (!att->attbyval)
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index a06800555c..4b09c26915 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -89,7 +89,17 @@ typedef struct TupleDescData
typedef struct TupleDescData *TupleDesc;
/* Accessor for the i'th attribute of tupdesc. */
+#if 1
+static inline FormData_pg_attribute *TupleDescAttr(TupleDesc tupdesc, int i)
+{
+ AssertArg(PointerIsValid(tupdesc));
+ AssertArg(i >= 0);
+ AssertArg(i < tupdesc->natts);
+ return &tupdesc->attrs[i];
+}
+#else
#define TupleDescAttr(tupdesc, i) (&(tupdesc)->attrs[(i)])
+#endif
extern TupleDesc CreateTemplateTupleDesc(int natts);
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