On 2024-Jan-11, Peter Eisentraut wrote:
> On 10.01.24 14:16, Alvaro Herrera wrote:
> > Seems reasonable. Do we really need a catversion bump for this?
>
> Yes, this changes the order of the fields in pg_attribute.
Ah, right.
> > 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.
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.)
> > 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.
Yeah, it's neat and the comment is clear enough.
> 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.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
