Thread: TupleDescCopy doesn't clear atthasdef, attnotnull, attidentity
Hi hackers, Andrew Gierth complained off-list that TupleDescCopy() doesn't clear atthasdef. Yeah, that's an oversight. The function is new in commit cc5f81366c36 and was written by me to support "flat" (pointer-free) tuple descriptors for use in DSM. Following the example of CreateTupleDescCopy() I think it should also clear attnotnull and attidentity. Please see attached. -- Thomas Munro http://www.enterprisedb.com
Attachment
On 11/29/2017 08:58 AM, Thomas Munro wrote: > Hi hackers, > > Andrew Gierth complained off-list that TupleDescCopy() doesn't clear > atthasdef. Yeah, that's an oversight. The function is new in commit > cc5f81366c36 and was written by me to support "flat" (pointer-free) > tuple descriptors for use in DSM. Following the example of > CreateTupleDescCopy() I think it should also clear attnotnull and > attidentity. Please see attached. This trivial patch is a clear oversight in the original patch. Thank you to Andrew for some explanation off-list to help me understand tuple descriptors better. Ready for committer. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Andrew Gierth complained off-list that TupleDescCopy() doesn't clear
> atthasdef. Yeah, that's an oversight. The function is new in commit
> cc5f81366c36 and was written by me to support "flat" (pointer-free)
> tuple descriptors for use in DSM. Following the example of
> CreateTupleDescCopy() I think it should also clear attnotnull and
> attidentity. Please see attached.
I've pushed this with some editorialization. I added some comments, and
noting that you have TupleDescCopy() copying the entire attribute array
in one memcpy, I propagated that same approach into CreateTupleDescCopy
and CreateTupleDescCopyConstr. This should make things a bit faster
since memcpy can spend more time in its maximum-stride loop.
The reason I note this explicitly is that I don't find it to be
entirely safe. If ATTRIBUTE_FIXED_PART_SIZE were less than
sizeof(FormData_pg_attribute) due to alignment padding at the end of
the struct, I think we would get some valgrind complaints about copying
uninitialized data, since there are code paths in which only the first
ATTRIBUTE_FIXED_PART_SIZE bytes of each array entry get filled in.
Now, currently I don't think there's any padding there anyway on any
platform we support. But if we're going to do things like this, I think
that we ought to explicitly make ATTRIBUTE_FIXED_PART_SIZE the same as
sizeof(FormData_pg_attribute). Hence, I propose the attached follow-on
patch.
regards, tom lane
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index f1f4423..67d3d3d 100644
*** a/src/backend/access/common/tupdesc.c
--- b/src/backend/access/common/tupdesc.c
*************** CreateTemplateTupleDesc(int natts, bool
*** 57,65 ****
* since we declare the array elements as FormData_pg_attribute for
* notational convenience. However, we only guarantee that the first
* ATTRIBUTE_FIXED_PART_SIZE bytes of each entry are valid; most code that
! * copies tupdesc entries around copies just that much. In principle that
! * could be less due to trailing padding, although with the current
! * definition of pg_attribute there probably isn't any padding.
*/
desc = (TupleDesc) palloc(offsetof(struct tupleDesc, attrs) +
natts * sizeof(FormData_pg_attribute));
--- 57,69 ----
* since we declare the array elements as FormData_pg_attribute for
* notational convenience. However, we only guarantee that the first
* ATTRIBUTE_FIXED_PART_SIZE bytes of each entry are valid; most code that
! * copies tupdesc entries around copies just that much. Currently, that
! * symbol is just defined as sizeof(FormData_pg_attribute) anyway, so this
! * distinction is somewhat academic. If there were any trailing padding
! * space in FormData_pg_attribute, it'd be possible to define
! * ATTRIBUTE_FIXED_PART_SIZE to omit it, but that might cause complaints
! * from valgrind about copying uninitialized padding bytes, so it seems
! * best to keep them the same.
*/
desc = (TupleDesc) palloc(offsetof(struct tupleDesc, attrs) +
natts * sizeof(FormData_pg_attribute));
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 6104254..739e784 100644
*** a/src/include/catalog/pg_attribute.h
--- b/src/include/catalog/pg_attribute.h
*************** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
*** 175,183 ****
* guaranteed-not-null part of a pg_attribute row. This is in fact as much
* of the row as gets copied into tuple descriptors, so don't expect you
* can access fields beyond attcollation except in a real tuple!
*/
#define ATTRIBUTE_FIXED_PART_SIZE \
! (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid))
/* ----------------
* Form_pg_attribute corresponds to a pointer to a tuple with
--- 175,188 ----
* guaranteed-not-null part of a pg_attribute row. This is in fact as much
* of the row as gets copied into tuple descriptors, so don't expect you
* can access fields beyond attcollation except in a real tuple!
+ *
+ * Since the introduction of CATALOG_VARLEN, this can just be defined as
+ * sizeof(FormData_pg_attribute). It's possible that that would include
+ * some trailing alignment padding, but that's harmless (and anyway there
+ * is no such padding given the current contents of pg_attribute).
*/
#define ATTRIBUTE_FIXED_PART_SIZE \
! sizeof(FormData_pg_attribute)
/* ----------------
* Form_pg_attribute corresponds to a pointer to a tuple with
On Thu, Jan 4, 2018 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: >> Andrew Gierth complained off-list that TupleDescCopy() doesn't clear >> atthasdef. Yeah, that's an oversight. The function is new in commit >> cc5f81366c36 and was written by me to support "flat" (pointer-free) >> tuple descriptors for use in DSM. Following the example of >> CreateTupleDescCopy() I think it should also clear attnotnull and >> attidentity. Please see attached. > > I've pushed this with some editorialization. I added some comments, and > noting that you have TupleDescCopy() copying the entire attribute array > in one memcpy, I propagated that same approach into CreateTupleDescCopy > and CreateTupleDescCopyConstr. This should make things a bit faster > since memcpy can spend more time in its maximum-stride loop. Thanks. > The reason I note this explicitly is that I don't find it to be > entirely safe. If ATTRIBUTE_FIXED_PART_SIZE were less than > sizeof(FormData_pg_attribute) due to alignment padding at the end of > the struct, I think we would get some valgrind complaints about copying > uninitialized data, since there are code paths in which only the first > ATTRIBUTE_FIXED_PART_SIZE bytes of each array entry get filled in. > Now, currently I don't think there's any padding there anyway on any > platform we support. But if we're going to do things like this, I think > that we ought to explicitly make ATTRIBUTE_FIXED_PART_SIZE the same as > sizeof(FormData_pg_attribute). Hence, I propose the attached follow-on > patch. That seems correct. If there is any system where sizeof(FormData_pg_attribute) != (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid)), won't load_relcache_init_file() get upset? Oh, I see it would just go to read_failed and then "do it the hard way". +1 -- Thomas Munro http://www.enterprisedb.com
On Thu, Jan 4, 2018 at 12:29 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > If there is any system where sizeof(FormData_pg_attribute) != > (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid)), won't > load_relcache_init_file() get upset? Oh, I see it would just go to > read_failed and then "do it the hard way". This doesn't make any sense, we're free to change that file format in a major release anyway. I withdraw that comment! -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> If there is any system where sizeof(FormData_pg_attribute) !=
> (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid)), won't
> load_relcache_init_file() get upset? Oh, I see it would just go to
> read_failed and then "do it the hard way".
Right, so we needn't include a catversion bump (or more likely,
a RELCACHE_INIT_FILEMAGIC change) in that patch.
regards, tom lane
I wrote:
> The reason I note this explicitly is that I don't find it to be
> entirely safe. If ATTRIBUTE_FIXED_PART_SIZE were less than
> sizeof(FormData_pg_attribute) due to alignment padding at the end of
> the struct, I think we would get some valgrind complaints about copying
> uninitialized data, since there are code paths in which only the first
> ATTRIBUTE_FIXED_PART_SIZE bytes of each array entry get filled in.
After further study I think this is a false alarm. valgrind doesn't
actually whine when you memcpy some undefined bytes: it just propagates
the undefined-ness of those bytes to the destination, and complains only
if the program uses the destination bytes to do something meaningful.
(Were this not so, copying structs containing embedded padding bytes
would always cause bogus warnings.)
There might still be value in redefining ATTRIBUTE_FIXED_PART_SIZE as
sizeof(FormData_pg_attribute), but I'm not sure, and am sort of inclined
to leave it alone.
regards, tom lane