Re: preliminary: logical column order

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Bruce and I were chatting about that on the phone today. I think it
> might be useful for TupleDescs to doubly index their contained
> attribute rows

Ok, I implemented this. I made it so that the properly sorted
attribute array is constructed lazily -- my reasoning was that (a)
relatively few locations in the code actually use the sorted attribute
array, so there is no point allocating and initializing it every time
a TupleDesc is used (b) the array could potentially be 1,500 pointers
long -- not huge, but not tiny either, so it is worth a little effort
to avoid unnecessarily alloc'ing and sorting it. Access to the sorted
array is (only) done via a new function, GetAttrByLogicalPosition()

> Easy to fix given above proposal ... although actually I am not sure
> why this would occur. printtup and friends should always get a
> constructed TupDesc that has no notion of deleted or renumbered
> columns.

It seems to happen because:

- SendRowDescription() in printtup.c notes that "the TupleDesc has
been manufactured by ExecTypeFromTL() or some similar function;
it does not contain a full set of fields."

- ExecTypeFromTL() allocates an empty TupleDesc, and then fills in
its individual entries with data from a TargetList -- the
Form_pg_attribute for the attributes involved is never consulted,
so the default attlogpos inserted by TupleDescInitEntry() is
used: attlogpos == attnum

I'm unsure of the best way to fix this so that the TupleDesc that is
handed to printtup & friends contains the information we require. Any
suggestions?

A new version of the patch is attached. Changes:

- replace sorting code with a lazily-constructed sorted array of
pointers to attribute data in TupleDesc
- include Manfred's suggested fix for the alter table regression
test
- (unrelated) add a comment to rewrite/rewriteDefine.c noting the
intent of the code and known problems
- (unrelated) merge my other patch for refactoring
CreateTupleDescCopy() into this patch: I needed to modify this
function in this patch, so I didn't want to deal with patching
complexities.

-Neil