Re: Make attstattarget nullable

On 10.01.24 14:16, Alvaro Herrera wrote:
>> Here is an updated patch rebased over 3e2e0d5ad7.
>>
>> The 0001 patch stands on its own, but I also tacked on two additional WIP
>> patches that simplify some pg_attribute handling and make these kinds of
>> refactorings simpler in the future.  See description in the patches.
> 
> I didn't look at 0002 and 0003, since they're marked as WIP.  (But I did
> like the removal that happens in 0003, so I hope these two also make it
> to 17).

Here is an updated patch set.  I have addressed your comments on 0001. 
I looked again at 0002 and 0003 and I was quite happy with them, so I 
just removed the WIP label and also added a few more code comments, but 
otherwise didn't change anything.

> Seems reasonable.  Do we really need a catversion bump for this?

Yes, this changes the order of the fields in pg_attribute.

> I like that we now have SET STATISTICS DEFAULT rather than -1 to reset
> to default.  Do we want to document that setting explicitly to -1
> continues to have that behavior?  (I would add something like "Setting
> to a value of -1 is an obsolete spelling to get the same behavior."
> after the phrase that explains DEFAULT in the ALTER TABLE manpage.)

done

> I noticed that equalTupleDescs no longer compares attstattarget, and
> this is because the field is not in TupleDesc anymore.  I looked at the
> callers of equalTupleDescs and I think this is exactly what we want
> (precisely because attstattarget is no longer in TupleDesc.)

Yes, I had investigated that in some detail, and I think it's ok.  I 
think equalTupleDescs() is actually mostly useless and I plan to start a 
separate discussion on that.

>>> This changes the pg_attribute field attstattarget into a nullable field
>>> in the variable-length part of the row.
> 
> I don't think we use "the variable-length part of the row" as a term
> anywhere.  We only have the variable-length columns, and we made a bit
> of a mistake in using CATALOG_VARLEN to differentiate the part of the
> catalogs that are not mapped to the structs (because at the time those
> were in effect only the variable length fields).  I think this is
> largely not a problem, but let's be careful with how we word the related
> comments.  So:

Yeah, there are multiple ways to interpret this.  There are fields with 
varlena headers, but there are also fields that are not-fixed-length as 
far as struct access to catalog tuples is concerned, and the two not the 
same.

> I think the comment next to "#ifdef CATALOG_VARLEN" is now a bit
> misleading, because the field immediately below is effectively not
> varlena.  Maybe make it
> #ifdef CATALOG_VARLEN            /* nullable/varlena fields start here */

done

> In RemoveAttributeById, a comment says
> "Clear the other variable-length fields."
> but this is no longer fully correct.  Again maybe make it "... the other
> nullable or variable-length fields".

done

> In get_attstattarget() I think we should return 0 for dropped columns
> without reading attstattarget, which is useless anyway, and if it did
> happen to return non-null, it might cause us to do stuff, which would be
> a waste.

I ended up deciding to get rid of get_attstattarget() altogether and 
just do the fetching inline in examine_attribute().  Because the 
previous API and what you are discussing here is over-designed, since 
the only caller doesn't call it with dropped columns or system columns 
anyway.  This way these issues are contained in the ANALYZE code, not in 
a very general place like lsyscache.c.

> It's annoying that the new code in index_concurrently_swap() is more
> verbose than the code being replaced, but it seems OK to me, since it
> allows us to distinguish a null value in attstattarget from actual 0
> without complicating the get_attstattarget API (which I think we would
> have to do if we wanted to use it here.)

Yeah, this was annoying.  Originally, I had it even more complicated, 
because I was trying to check if the actual (non-null) values are the 
same.  But then I realized the new value is never set at this point.  I 
think what the code is actually about is clearer now.  And of course the 
0003 patch gets rid of it anyway.