Re: TupleDescCopy doesn't clear atthasdef, attnotnull, attidentity

On Thu, Jan 4, 2018 at 12:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)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