On 12.01.24 12:16, Alvaro Herrera wrote:
>>> 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.
>
> Sounds good.
I have committed this first patch.
> Maybe instead of having examine_attribute hand a -1 target to the
> analyze functions, we could just put default_statistics_target there.
> Analyze functions would never receive negative values, and we could
> remove that from the analyze functions. Maybe make
> VacAttrStats->attstattarget unsigned while at it. (This could be a
> separate patch.)
But I now remembered why I didn't do this. The extended statistics code
needs to know whether the statistics target was set or left as default,
because it will then apply its own sequence of logic to determine a
final value. (Maybe there is a way to untangle this further, but it's
not as obvious as it seems.)
At which point I then realized that extended statistics have their own
statistics target catalog field and command, and we really should change
that to match the changes done to attstattarget. So here is another
patch that does all that again for stxstattarget. It's meant to mirror
the attstattarget changes exactly.
>> And of course the 0003 patch gets rid of it anyway.
>
> I again didn't look at 0002 and 0003 very closely, but from 10,000 feet
> it looks mostly reasonable -- but I think naming the struct
> FormData_pg_attribute_extra is not a great idea, as it looks like there
> would have to be a catalog named pg_attribute_extra -- and I don't think
> I would make the "non-Data" pointer-to-struct typedef either.
I agree that this naming was problematic. After some introverted
bikeshedding, I changed it to FormExtraData_pg_attribute. Obviously,
other solutions are possible. I also removed the typedef as you suggested.