Re: Make attstattarget nullable

On 2023-Dec-23, Peter Eisentraut 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).

> On 05.12.23 13:52, Peter Eisentraut wrote:
> > In [0] it was discussed that we could make attstattarget a nullable
> > column, instead of always storing an explicit -1 default value for most
> > columns.  This patch implements this.

Seems reasonable. Do we really need a catversion bump for this?

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.)

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.)

> > 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:

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 */

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".

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.

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.)

I don't have anything else on this patch at this point.

Thanks

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/