On Thu, Jan 4, 2018 at 12:01 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.mu...@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
