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/