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.