Re: Make attstattarget nullable - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: Make attstattarget nullable
Date
Msg-id 202401121116.t5hjmzszlzzw@alvherre.pgsql
Whole thread Raw
In response to Re: Make attstattarget nullable  (Peter Eisentraut <peter@eisentraut.org>)
Responses Re: Make attstattarget nullable
Re: Make attstattarget nullable
List pgsql-hackers
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/



pgsql-hackers by date:

Previous
From: Alexander Lakhin
Date:
Subject: Re: Test slots invalidations in 035_standby_logical_decoding.pl only if dead rows are removed
Next
From: Alvaro Herrera
Date:
Subject: Re: Make attstattarget nullable