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