Re: Make attstattarget nullable
On Sun, Mar 17, 2024 at 01:51:39PM +0100, Peter Eisentraut wrote:
> I have committed this patch series. Thanks.
My compiler is worried that "newtarget" might be getting used
uninitialized. AFAICT there's no actual risk here, so I think initializing
it to 0 is sufficient. I'll commit the attached patch shortly.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 5f49479832..1db3ef69d2 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -606,7 +606,7 @@ AlterStatistics(AlterStatsStmt *stmt)
bool repl_null[Natts_pg_statistic_ext];
bool repl_repl[Natts_pg_statistic_ext];
ObjectAddress address;
- int newtarget;
+ int newtarget = 0;
bool newtarget_default;
/* -1 was used in previous versions for the default setting */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ae6719329e..3ed0618b4e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8711,7 +8711,7 @@ ATExecDropExpression(Relation rel, const char *colName, bool missing_ok, LOCKMOD
static ObjectAddress
ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newValue, LOCKMODE lockmode)
{
- int newtarget;
+ int newtarget = 0;
bool newtarget_default;
Relation attrelation;
HeapTuple tuple,
Re: Make attstattarget nullable
On 14.03.24 15:46, Tomas Vondra wrote: 2) The newtarget handling in AlterStatistics seems rather confusing. Why does it get set to -1 just to ignore the value later? For a while I was 99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field to -1. Maybe ditching the first if block and directly checking stmt->stxstattarget before setting repl_val/repl_null would be better? But we also need to continue accepting -1 for default on input. The current code achieves that, the proposed variant would not. OK, I did not realize that. But then maybe this should be explained in a comment before the new "if" block, because people won't realize why it needs to be this way. In the new version, I tried to write this more explicitly, and updated tablecmds.c to match. WFM. It still seems a bit hard to read, but I don't know how to do it better. I guess it's how it has to be to deal with multiple default values in a backwards-compatible way. Good thing is it's localized in two places. I have committed this patch series. Thanks.
Re: Make attstattarget nullable
On 3/14/24 11:13, Peter Eisentraut wrote:
> On 12.03.24 14:32, Tomas Vondra wrote:
>> On 3/12/24 13:47, Peter Eisentraut wrote:
>>> On 06.03.24 22:34, Tomas Vondra wrote:
0001
1) I think this bit in ALTER STATISTICS docs is wrong:
- >>> class="parameter">new_target
+ SET STATISTICS { >>> class="parameter">integer | DEFAULT }
because it means we now have list entries for name, ..., new_name,
new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
That's a bit weird.
>>>
>>> Ok, how would you change it? List out the full clauses of the other
>>> variants under Parameters as well?
>>
>> I'd go with a parameter, essentially exactly as it used to be, except
>> for adding the DEFAULT option. So the list would define new_target, and
>> mention DEFAULT as a special value.
>
> Ok, done that way (I think).
>
Seems OK to me.
2) The newtarget handling in AlterStatistics seems rather confusing.
Why
does it get set to -1 just to ignore the value later? For a while I was
99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
to -1. Maybe ditching the first if block and directly checking
stmt->stxstattarget before setting repl_val/repl_null would be better?
>>>
>>> But we also need to continue accepting -1 for default on input. The
>>> current code achieves that, the proposed variant would not.
>>
>> OK, I did not realize that. But then maybe this should be explained in a
>> comment before the new "if" block, because people won't realize why it
>> needs to be this way.
>
> In the new version, I tried to write this more explicitly, and updated
> tablecmds.c to match.
WFM. It still seems a bit hard to read, but I don't know how to do it
better. I guess it's how it has to be to deal with multiple default
values in a backwards-compatible way. Good thing is it's localized in
two places.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Make attstattarget nullable
On 12.03.24 14:32, Tomas Vondra wrote:
On 3/12/24 13:47, Peter Eisentraut wrote:
On 06.03.24 22:34, Tomas Vondra wrote:
0001
1) I think this bit in ALTER STATISTICS docs is wrong:
- new_target
+ SET STATISTICS { integer | DEFAULT }
because it means we now have list entries for name, ..., new_name,
new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
That's a bit weird.
Ok, how would you change it? List out the full clauses of the other
variants under Parameters as well?
I'd go with a parameter, essentially exactly as it used to be, except
for adding the DEFAULT option. So the list would define new_target, and
mention DEFAULT as a special value.
Ok, done that way (I think).
2) The newtarget handling in AlterStatistics seems rather confusing. Why
does it get set to -1 just to ignore the value later? For a while I was
99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
to -1. Maybe ditching the first if block and directly checking
stmt->stxstattarget before setting repl_val/repl_null would be better?
But we also need to continue accepting -1 for default on input. The
current code achieves that, the proposed variant would not.
OK, I did not realize that. But then maybe this should be explained in a
comment before the new "if" block, because people won't realize why it
needs to be this way.
In the new version, I tried to write this more explicitly, and updated
tablecmds.c to match.
From d98742439bfe82a20cffcdda6d5a05fdd06b46ea Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 14 Mar 2024 11:06:49 +0100
Subject: [PATCH v5 1/3] Make stxstattarget nullable
To match attstattarget change (commit 4f622503d6d).
Reviewed-by: Tomas Vondra
Discussion:
https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184...@eisentraut.org
---
doc/src/sgml/catalogs.sgml | 28 +++
doc/src/sgml/ref/alter_statistics.sgml | 11 +++---
src/backend/commands/statscmds.c| 46
src/backend/commands/tablecmds.c| 47 +
src/backend/parser/gram.y | 4 +--
src/backend/statistics/extended_stats.c | 4 ++-
src/bin/pg_dump/pg_dump.c | 10 +++---
src/bin/psql/describe.c | 3 +-
src/include/catalog/catversion.h| 2 +-
src/include/catalog/pg_statistic_ext.h | 6 ++--
src/include/nodes/parsenodes.h | 2 +-
11 files changed, 93 insertions(+), 70 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 387a14b1869..2f091ad09d1 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7657,6 +7657,19 @@ pg_statistic_ext
Columns
+
+
+ stxkeys int2vector
+ (references pg_attribute.attnum)
+
+
+ An array of attribute numbers, indicating which table columns are
+ covered by this statistics object;
+ for example a value of 1 3 would
+ mean that the first and the third table columns are covered
+
+
+
stxstattarget int2
@@ -7666,7 +7679,7 @@ pg_statistic_ext
Columns
of statistics accumulated for this statistics object by
ANALYZE.
A zero value indicates that no statistics should be collected.
- A negative value says to use the maximum of the statistics targets of
+ A null value says to use the maximum of the statistics targets of
the referenced columns, if set, or the system default statistics target.
Positive values of stxstattarget
determine the target number of most common values
@@ -7674,19 +7687,6 @@ pg_statistic_ext
Columns
-
-
- stxkeys int2vector
- (references pg_attribute.attnum)
-
-
- An array of attribute numbers, indicating which table columns are
- covered by this statistics object;
- for example a value of 1 3 would
- mean that the first and the third table columns are covered
-
-
-
stxkind char[]
diff --git a/doc/src/sgml/ref/alter_statistics.sgml
b/doc/src/sgml/ref/alter_statistics.sgml
index 73cc9e830de..c82a728a910 100644
--- a/doc/src/sgml/ref/alter_statistics.sgml
+++ b/doc/src/sgml/ref/alter_statistics.sgml
@@ -26,7 +26,7 @@
ALTER STATISTICS name OWNER TO {
new_owner | CURRENT_ROLE |
CURRENT_USER | SESSION_USER }
ALTER STATISTICS name RENAME TO
new_name
ALTER STATISTICS name SET SCHEMA
new_schema
-ALTER STATISTICS name SET
STATISTICS new_target
+ALTER STATISTICS name SET
STATISTICS { new_target | DEFAULT }
@@ -101,10 +101,11 @@ Parameters
The statistic-gathering target for this statistics object for
subsequent
ANALYZE
operations.
-The target can be set in the range 0 to 1; alternatively, set it
-to -1 to revert to using the maximum of the statistics target of the
-referenced
Re: Make attstattarget nullable
On 3/12/24 13:47, Peter Eisentraut wrote:
> On 06.03.24 22:34, Tomas Vondra wrote:
>> 0001
>>
>>
>> 1) I think this bit in ALTER STATISTICS docs is wrong:
>>
>> - > class="parameter">new_target
>> + SET STATISTICS { > class="parameter">integer | DEFAULT }
>>
>> because it means we now have list entries for name, ..., new_name,
>> new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
>> That's a bit weird.
>
> Ok, how would you change it? List out the full clauses of the other
> variants under Parameters as well?
I'd go with a parameter, essentially exactly as it used to be, except
for adding the DEFAULT option. So the list would define new_target, and
mention DEFAULT as a special value.
> We have similar inconsistencies on other ALTER reference pages, so I'm
> not sure what the preferred approach is.
>
Yeah, the other reference pages may have some inconsistencies too, but
let's not add more.
>> 2) The newtarget handling in AlterStatistics seems rather confusing. Why
>> does it get set to -1 just to ignore the value later? For a while I was
>> 99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
>> to -1. Maybe ditching the first if block and directly checking
>> stmt->stxstattarget before setting repl_val/repl_null would be better?
>
> But we also need to continue accepting -1 for default on input. The
> current code achieves that, the proposed variant would not.
>
OK, I did not realize that. But then maybe this should be explained in a
comment before the new "if" block, because people won't realize why it
needs to be this way.
> Note that this patch matches the equivalent patch for attstattarget
> (4f622503d6d), which uses the same logic. We could change it if we have
> a better idea, but then we should change both.
>
>> 0002
>>
>>
>> 1) I think InsertPgAttributeTuples comment probably needs to document
>> what the new tupdesc_extra parameter does.
>
> Yes, I'll update that comment.
>
OK.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Make attstattarget nullable
On 06.03.24 22:34, Tomas Vondra wrote:
0001
1) I think this bit in ALTER STATISTICS docs is wrong:
- new_target
+ SET STATISTICS { integer | DEFAULT }
because it means we now have list entries for name, ..., new_name,
new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
That's a bit weird.
Ok, how would you change it? List out the full clauses of the other
variants under Parameters as well?
We have similar inconsistencies on other ALTER reference pages, so I'm
not sure what the preferred approach is.
2) The newtarget handling in AlterStatistics seems rather confusing. Why
does it get set to -1 just to ignore the value later? For a while I was
99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
to -1. Maybe ditching the first if block and directly checking
stmt->stxstattarget before setting repl_val/repl_null would be better?
But we also need to continue accepting -1 for default on input. The
current code achieves that, the proposed variant would not.
Note that this patch matches the equivalent patch for attstattarget
(4f622503d6d), which uses the same logic. We could change it if we have
a better idea, but then we should change both.
0002
1) I think InsertPgAttributeTuples comment probably needs to document
what the new tupdesc_extra parameter does.
Yes, I'll update that comment.
Re: Make attstattarget nullable
Hi Peter,
I took a look at this patch today. I think it makes sense to do this,
and I haven't found any major issues with any of the three patches. A
couple minor comments:
0001
1) I think this bit in ALTER STATISTICS docs is wrong:
- new_target
+ SET STATISTICS { integer | DEFAULT }
because it means we now have list entries for name, ..., new_name,
new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
That's a bit weird.
2) The newtarget handling in AlterStatistics seems rather confusing. Why
does it get set to -1 just to ignore the value later? For a while I was
99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
to -1. Maybe ditching the first if block and directly checking
stmt->stxstattarget before setting repl_val/repl_null would be better?
3) I find it a bit tedious that making the stxstattarget field nullable
means we now have to translate NULL to -1 in so many places. I know why
it's that way, but it's ... not very convenient :-(
0002
1) I think InsertPgAttributeTuples comment probably needs to document
what the new tupdesc_extra parameter does.
0003
no comment, seems fine
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Make attstattarget nullable
On 12.01.24 12:16, Alvaro Herrera wrote: 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. I have committed this first patch. 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.) But I now remembered why I didn't do this. The extended statistics code needs to know whether the statistics target was set or left as default, because it will then apply its own sequence of logic to determine a final value. (Maybe there is a way to untangle this further, but it's not as obvious as it seems.) At which point I then realized that extended statistics have their own statistics target catalog field and command, and we really should change that to match the changes done to attstattarget. So here is another patch that does all that again for stxstattarget. It's meant to mirror the attstattarget changes exactly. 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. I agree that this naming was problematic. After some introverted bikeshedding, I changed it to FormExtraData_pg_attribute. Obviously, other solutions are possible. I also removed the typedef as you suggested. From 9199be09efcbca1b906b5c41e8524e68b1a6b64e Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 15 Jan 2024 16:44:55 +0100 Subject: [PATCH v4 1/3] Make stxstattarget nullable To match attstattarget change (commit 4f622503d6d). TODO: catversion --- doc/src/sgml/catalogs.sgml | 28 - doc/src/sgml/ref/alter_statistics.sgml | 13 ++-- src/backend/commands/statscmds.c| 21 --- src/backend/parser/gram.y | 4 ++-- src/backend/statistics/extended_stats.c | 4 +++- src/bin/pg_dump/pg_dump.c | 10 + src/bin/psql/describe.c | 3 ++- src/include/catalog/pg_statistic_ext.h | 6 +++--- src/include/nodes/parsenodes.h | 2 +- 9 files changed, 56 insertions(+), 35 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index c15d861e823..34458df6d4c 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -7633,6 +7633,19 @@ pg_statistic_ext Columns + + + stxkeys int2vector + (references pg_attribute.attnum) + + + An array of attribute numbers, indicating which table columns are + covered by this statistics object; + for example a value of 1 3 would + mean that the first and the third table columns are covered + + + stxstattarget int2 @@ -7642,7 +7655,7 @@ pg_statistic_ext Columns of statistics accumulated for this statistics object by ANALYZE. A zero value indicates that no statistics should be collected. - A negative value says to use the maximum of the statistics targets of + A null value says to use the maximum of the statistics targets of the referenced columns, if set, or the system default statistics target. Positive values of stxstattarget determine the target number of most common values @@ -7650,19 +7663,6 @@ pg_statistic_ext Columns - - - stxkeys int2vector - (references pg_attribute.attnum) - - - An array of attribute numbers, indicating which table columns are - covered by this statistics object; - for example a value of 1 3 would - mean that the first and the third table columns are covered - - - stxkind char[] diff --git a/doc/src/sgml/ref/alter_statistics.sgml b/doc/src/sgml/ref/alter_statistics.sgml index 73cc9e830de..c16caa0b61b 100644 --- a/doc/src/sgml/ref/alter_statistics.sgml +++ b/doc/src/sgml/ref/alter_statistics.sgml @@ -26,7 +26,7 @@ ALTER STATISTICS name OWNER TO
Re: Make attstattarget nullable
BTW I wanted to but didn't say so explicitly, so here goes: 0001 looks ready to go in. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Find a bug in a program, and fix it, and the program will work today. Show the program how to find and fix a bug, and the program will work forever" (Oliver Silfridge)
Re: Make attstattarget nullable
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/
Re: Make attstattarget nullable
On 10.01.24 14:16, Alvaro Herrera 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). Here is an updated patch set. I have addressed your comments on 0001. I looked again at 0002 and 0003 and I was quite happy with them, so I just removed the WIP label and also added a few more code comments, but otherwise didn't change anything. Seems reasonable. Do we really need a catversion bump for this? Yes, this changes the order of the fields in pg_attribute. 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.) done 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.) Yes, I had investigated that in some detail, and I think it's ok. I think equalTupleDescs() is actually mostly useless and I plan to start a separate discussion on that. 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: Yeah, there are multiple ways to interpret this. There are fields with varlena headers, but there are also fields that are not-fixed-length as far as struct access to catalog tuples is concerned, and the two not the same. 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 */ done 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". done 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. 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. And of course the 0003 patch gets rid of it anyway. From d937c26d8c471c999aa53c96dce86c68fad71a7a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 11 Jan 2024 10:09:02 +0100 Subject: [PATCH v3 1/3] Make attstattarget nullable This changes the pg_attribute field attstattarget into a nullable field in the variable-length part of the row. If no value is set by the user for attstattarget, it is now null instead of previously -1. This saves space in pg_attribute and tuple descriptors for most practical scenarios. (ATTRIBUTE_FIXED_PART_SIZE is reduced from 108 to 104.) Also, null is the semantically more correct value. The ANALYZE code internally continues to represent the default statistics target by -1, so that that code can avoid having to deal with null values. But that is now contained to the ANALYZE code. Only the DDL code deals with attstattarget possibly null. For system
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 HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Make attstattarget nullable
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.
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.
This changes the pg_attribute field attstattarget into a nullable field
in the variable-length part of the row. If no value is set by the user
for attstattarget, it is now null instead of previously -1. This saves
space in pg_attribute and tuple descriptors for most practical
scenarios. (ATTRIBUTE_FIXED_PART_SIZE is reduced from 108 to 104.)
Also, null is the semantically more correct value.
The ANALYZE code internally continues to represent the default
statistics target by -1, so that that code can avoid having to deal with
null values. But that is now contained to ANALYZE code. The DDL code
deals with attstattarget possibly null.
For system columns, the field is now always null but the effective value
0 (don't analyze) is assumed.
To set a column's statistics target to the default value, the new
command form ALTER TABLE ... SET STATISTICS DEFAULT can be used. (SET
STATISTICS -1 still works.)
[0]:
https://www.postgresql.org/message-id/flat/d07ffc2b-e0e8-77f7-38fb-be921dff71af%40enterprisedb.com
From f370eceec0cbb9b6bf76d3394e56a5df4280c906 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 23 Dec 2023 10:47:19 +0100
Subject: [PATCH v2 1/3] Make attstattarget nullable
This changes the pg_attribute field attstattarget into a nullable
field in the variable-length part of the row. If no value is set by
the user for attstattarget, it is now null instead of previously -1.
This saves space in pg_attribute and tuple descriptors for most
practical scenarios. (ATTRIBUTE_FIXED_PART_SIZE is reduced from 108
to 104.) Also, null is the semantically more correct value.
The ANALYZE code internally continues to represent the default
statistics target by -1, so that that code can avoid having to deal
with null values. But that is now contained to ANALYZE code. The DDL
code deals with attstattarget possibly null.
For system columns, the field is now always null but the effective
value 0 (don't analyze) is assumed.
To set a column's statistics target to the default value, the new
command form ALTER TABLE ... SET STATISTICS DEFAULT can be used. (SET
STATISTICS -1 still works.)
Discussion:
https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184...@eisentraut.org
TODO: move get_attstattarget() into analyze.c?
TODO: catversion
---
doc/src/sgml/ref/alter_table.sgml | 4 +-
src/backend/access/common/tupdesc.c| 4 --
src/backend/bootstrap/bootstrap.c | 1 -
src/backend/catalog/genbki.pl | 1 -
src/backend/catalog/heap.c | 14 +++
src/backend/catalog/index.c| 21 ---
src/backend/commands/analyze.c | 7 +++-
src/backend/commands/tablecmds.c | 44 +-
src/backend/parser/gram.y | 18 ++---
src/backend/utils/cache/lsyscache.c| 22 +--
src/bin/pg_dump/pg_dump.c | 7 +++-
src/include/catalog/pg_attribute.h | 16
src/include/commands/vacuum.h | 2 +-
src/test/regress/expected/create_index.out | 4 +-
14 files changed, 109 insertions(+), 56 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml
b/doc/src/sgml/ref/alter_table.sgml
index e1d207bc60..9d637157eb 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -50,7 +50,7 @@
ALTER [ COLUMN ] column_name
ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ (
sequence_options ) ]
ALTER [ COLUMN ] column_name
{ SET GENERATED { ALWAYS | BY DEFAULT } | SET
sequence_option | RESTART [ [ WITH ] restart ] } [...]
ALTER [ COLUMN ] column_name
DROP IDENTITY [ IF EXISTS ]
-ALTER [ COLUMN ] column_name
SET STATISTICS integer
+ALTER [ COLUMN ] column_name
SET STATISTICS { integer | DEFAULT
}
ALTER [ COLUMN ] column_name
SET ( attribute_option =
value [, ... ] )
ALTER [ COLUMN ] column_name
RESET ( attribute_option [, ... ] )
ALTER [ COLUMN ] column_name
SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT }
@@ -317,7 +317,7 @@ Description
sets the per-column statistics-gathering target for subsequent
ANALYZE operations.
The target can be set in the range 0 to 1; alternatively, set it
- to -1 to revert to using the system default statistics
+ to DEFAULT to revert to using the system default
statistics
target ().
For more information on the use of statistics by the
PostgreSQL
