Make attstattarget nullable

Started by Peter Eisentrautabout 2 years ago14 messages
#1Peter Eisentraut
peter@eisentraut.org
1 attachment(s)

In [0]/messages/by-id/d07ffc2b-e0e8-77f7-38fb-be921dff71af@enterprisedb.com 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]: /messages/by-id/d07ffc2b-e0e8-77f7-38fb-be921dff71af@enterprisedb.com
/messages/by-id/d07ffc2b-e0e8-77f7-38fb-be921dff71af@enterprisedb.com

Attachments:

v1-0001-Make-attstattarget-nullable.patchtext/plain; charset=UTF-8; name=v1-0001-Make-attstattarget-nullable.patchDownload
From 174d51a66c80a3284ad97b347a286a7b65a2a440 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 5 Dec 2023 13:43:30 +0100
Subject: [PATCH v1] 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.)

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                 | 25 ++++++------
 src/backend/catalog/index.c                | 23 ++++++++---
 src/backend/commands/analyze.c             |  7 +++-
 src/backend/commands/tablecmds.c           | 44 +++++++++++++++++-----
 src/backend/parser/gram.y                  | 18 ++++++---
 src/backend/utils/cache/lsyscache.c        | 17 +++++++--
 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, 113 insertions(+), 60 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 ] <replaceable class="parameter">column_name</replaceable> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> { SET GENERATED { ALWAYS | BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESTART [ [ WITH ] <replaceable class="parameter">restart</replaceable> ] } [...]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> DROP IDENTITY [ IF EXISTS ]
-    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
+    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS { <replaceable class="parameter">integer</replaceable> | DEFAULT }
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT }
@@ -317,7 +317,7 @@ <title>Description</title>
       sets the per-column statistics-gathering target for subsequent
       <link linkend="sql-analyze"><command>ANALYZE</command></link> operations.
       The target can be set in the range 0 to 10000; alternatively, set it
-      to -1 to revert to using the system default statistics
+      to <literal>DEFAULT</literal> to revert to using the system default statistics
       target (<xref linkend="guc-default-statistics-target"/>).
       For more information on the use of statistics by the
       <productname>PostgreSQL</productname> query planner, refer to
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 8826519e5e..054ccff1e2 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -453,8 +453,6 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 			return false;
 		if (attr1->atttypid != attr2->atttypid)
 			return false;
-		if (attr1->attstattarget != attr2->attstattarget)
-			return false;
 		if (attr1->attlen != attr2->attlen)
 			return false;
 		if (attr1->attndims != attr2->attndims)
@@ -639,7 +637,6 @@ TupleDescInitEntry(TupleDesc desc,
 	else if (attributeName != NameStr(att->attname))
 		namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
@@ -702,7 +699,6 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
 	Assert(attributeName != NULL);
 	namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index e01dca9b7c..c7546da51e 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -552,7 +552,6 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
 	if (OidIsValid(attrtypes[attnum]->attcollation))
 		attrtypes[attnum]->attcollation = C_COLLATION_OID;
 
-	attrtypes[attnum]->attstattarget = -1;
 	attrtypes[attnum]->attcacheoff = -1;
 	attrtypes[attnum]->atttypmod = -1;
 	attrtypes[attnum]->attislocal = true;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 380bc23c82..13cd2fee14 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -840,7 +840,6 @@ sub gen_pg_attribute
 				my %row;
 				$row{attnum} = $attnum;
 				$row{attrelid} = $table->{relation_oid};
-				$row{attstattarget} = '0';
 
 				morph_row_for_pgattr(\%row, $schema, $attr, 1);
 				print_bki_insert(\%row, $schema);
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7224d96695..6e08d9b496 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -749,14 +749,16 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attisdropped - 1] = BoolGetDatum(attrs->attisdropped);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
-		slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attrs->attstattarget);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attoptions && attoptions[natts] != (Datum) 0)
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
 		else
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 
-		/* start out with empty permissions and empty options */
+		/*
+		 * The remaining fields are not set for new columns.
+		 */
+		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
@@ -818,9 +820,6 @@ AddNewAttributeTuples(Oid new_rel_oid,
 
 	indstate = CatalogOpenIndexes(rel);
 
-	/* set stats detail level to a sane default */
-	for (int i = 0; i < natts; i++)
-		tupdesc->attrs[i].attstattarget = -1;
 	InsertPgAttributeTuples(rel, tupdesc, new_rel_oid, NULL, indstate);
 
 	/* add dependencies on their datatypes and collations */
@@ -1647,6 +1646,9 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	HeapTuple	tuple;
 	Form_pg_attribute attStruct;
 	char		newattname[NAMEDATALEN];
+	Datum		valuesAtt[Natts_pg_attribute] = {0};
+	bool		nullsAtt[Natts_pg_attribute] = {0};
+	bool		replacesAtt[Natts_pg_attribute] = {0};
 
 	/*
 	 * Grab an exclusive lock on the target table, which we will NOT release
@@ -1683,7 +1685,8 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	attStruct->attnotnull = false;
 
 	/* We don't want to keep stats for it anymore */
-	attStruct->attstattarget = 0;
+	nullsAtt[Anum_pg_attribute_attstattarget - 1] = true;
+	replacesAtt[Anum_pg_attribute_attstattarget - 1] = true;
 
 	/* Unset this so no one tries to look up the generation expression */
 	attStruct->attgenerated = '\0';
@@ -1698,10 +1701,6 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	/* clear the missing value if any */
 	if (attStruct->atthasmissing)
 	{
-		Datum		valuesAtt[Natts_pg_attribute] = {0};
-		bool		nullsAtt[Natts_pg_attribute] = {0};
-		bool		replacesAtt[Natts_pg_attribute] = {0};
-
 		/* update the tuple - set atthasmissing and attmissingval */
 		valuesAtt[Anum_pg_attribute_atthasmissing - 1] =
 			BoolGetDatum(false);
@@ -1709,11 +1708,11 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 		valuesAtt[Anum_pg_attribute_attmissingval - 1] = (Datum) 0;
 		nullsAtt[Anum_pg_attribute_attmissingval - 1] = true;
 		replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
-
-		tuple = heap_modify_tuple(tuple, RelationGetDescr(attr_rel),
-								  valuesAtt, nullsAtt, replacesAtt);
 	}
 
+	tuple = heap_modify_tuple(tuple, RelationGetDescr(attr_rel),
+							  valuesAtt, nullsAtt, replacesAtt);
+
 	CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
 
 	/*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b8c7945322..60fa4e5c4a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -325,7 +325,6 @@ ConstructTupleDescriptor(Relation heapRelation,
 
 		MemSet(to, 0, ATTRIBUTE_FIXED_PART_SIZE);
 		to->attnum = i + 1;
-		to->attstattarget = -1;
 		to->attcacheoff = -1;
 		to->attislocal = true;
 		to->attcollation = (i < numkeyatts) ? collationIds[i] : InvalidOid;
@@ -1780,31 +1779,45 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
 		{
 			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
+			HeapTuple	tp;
+			Datum		dat1,
+						dat2;
+			bool		isnull1,
+						isnull2;
 			Datum		repl_val[Natts_pg_attribute];
 			bool		repl_null[Natts_pg_attribute];
 			bool		repl_repl[Natts_pg_attribute];
-			int			attstattarget;
 			HeapTuple	newTuple;
 
 			/* Ignore dropped columns */
 			if (att->attisdropped)
 				continue;
 
+			dat1 = heap_getattr(attrTuple, Anum_pg_attribute_attstattarget, RelationGetDescr(pg_attribute), &isnull1);
+
 			/*
 			 * Get attstattarget from the old index and refresh the new value.
 			 */
-			attstattarget = get_attstattarget(oldIndexId, att->attnum);
+			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
+			if (!HeapTupleIsValid(tp))
+				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+					 att->attnum, oldIndexId);
+			dat2 = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull2);
+			ReleaseSysCache(tp);
 
 			/* no need for a refresh if both match */
-			if (attstattarget == att->attstattarget)
+			if ((isnull1 && isnull2) || (!isnull1 && !isnull2 && DatumGetInt16(dat1) == DatumGetInt16(dat2)))
 				continue;
 
 			memset(repl_val, 0, sizeof(repl_val));
 			memset(repl_null, false, sizeof(repl_null));
 			memset(repl_repl, false, sizeof(repl_repl));
 
+			if (isnull2)
+				repl_null[Anum_pg_attribute_attstattarget - 1] = true;
+			else
+				repl_val[Anum_pg_attribute_attstattarget - 1] = dat2;
 			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attstattarget);
 
 			newTuple = heap_modify_tuple(attrTuple,
 										 RelationGetDescr(pg_attribute),
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index e264ffdcf2..116ac23fa4 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1001,6 +1001,7 @@ static VacAttrStats *
 examine_attribute(Relation onerel, int attnum, Node *index_expr)
 {
 	Form_pg_attribute attr = TupleDescAttr(onerel->rd_att, attnum - 1);
+	int			attstattarget;
 	HeapTuple	typtuple;
 	VacAttrStats *stats;
 	int			i;
@@ -1010,15 +1011,17 @@ examine_attribute(Relation onerel, int attnum, Node *index_expr)
 	if (attr->attisdropped)
 		return NULL;
 
+	attstattarget = get_attstattarget(RelationGetRelid(onerel), attnum);
+
 	/* Don't analyze column if user has specified not to */
-	if (attr->attstattarget == 0)
+	if (attstattarget == 0)
 		return NULL;
 
 	/*
 	 * Create the VacAttrStats struct.
 	 */
 	stats = (VacAttrStats *) palloc0(sizeof(VacAttrStats));
-	stats->attstattarget = attr->attstattarget;
+	stats->attstattarget = attstattarget;
 
 	/*
 	 * When analyzing an expression index, believe the expression tree's type
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6b0a20010e..77bb17c479 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7124,7 +7124,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.attrelid = myrelid;
 	namestrcpy(&(attribute.attname), colDef->colname);
 	attribute.atttypid = typeOid;
-	attribute.attstattarget = -1;
 	attribute.attlen = tform->typlen;
 	attribute.attnum = newattnum;
 	if (list_length(colDef->typeName->arrayBounds) > PG_INT16_MAX)
@@ -8453,10 +8452,14 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 {
 	int			newtarget;
 	Relation	attrelation;
-	HeapTuple	tuple;
+	HeapTuple	tuple,
+				newtuple;
 	Form_pg_attribute attrtuple;
 	AttrNumber	attnum;
 	ObjectAddress address;
+	Datum		repl_val[Natts_pg_attribute];
+	bool		repl_null[Natts_pg_attribute];
+	bool		repl_repl[Natts_pg_attribute];
 
 	/*
 	 * We allow referencing columns by numbers only for indexes, since table
@@ -8469,8 +8472,18 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot refer to non-index column by number")));
 
-	Assert(IsA(newValue, Integer));
-	newtarget = intVal(newValue);
+	if (newValue)
+	{
+		Assert(IsA(newValue, Integer));
+		newtarget = intVal(newValue);
+	}
+	else
+	{
+		/*
+		 * -1 was used in previous versions to represent the default setting
+		 */
+		newtarget = -1;
+	}
 
 	/*
 	 * Limit target to a sane range
@@ -8495,7 +8508,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 
 	if (colName)
 	{
-		tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
+		tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8505,7 +8518,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 	}
 	else
 	{
-		tuple = SearchSysCacheCopyAttNum(RelationGetRelid(rel), colNum);
+		tuple = SearchSysCacheAttNum(RelationGetRelid(rel), colNum);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8539,16 +8552,27 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 					 errhint("Alter statistics on table column instead.")));
 	}
 
-	attrtuple->attstattarget = newtarget;
-
-	CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
+	/* Build new tuple. */
+	memset(repl_null, false, sizeof(repl_null));
+	memset(repl_repl, false, sizeof(repl_repl));
+	if (newtarget != -1)
+		repl_val[Anum_pg_attribute_attstattarget - 1] = newtarget;
+	else
+		repl_null[Anum_pg_attribute_attstattarget - 1] = true;
+	repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
+	newtuple = heap_modify_tuple(tuple, RelationGetDescr(attrelation),
+								 repl_val, repl_null, repl_repl);
+	CatalogTupleUpdate(attrelation, &tuple->t_self, newtuple);
 
 	InvokeObjectPostAlterHook(RelationRelationId,
 							  RelationGetRelid(rel),
 							  attrtuple->attnum);
 	ObjectAddressSubSet(address, RelationRelationId,
 						RelationGetRelid(rel), attnum);
-	heap_freetuple(tuple);
+
+	heap_freetuple(newtuple);
+
+	ReleaseSysCache(tuple);
 
 	table_close(attrelation, RowExclusiveLock);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d631ac89a9..b86bc0c298 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -337,6 +337,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <list>	alter_table_cmds alter_type_cmds
 %type <list>    alter_identity_column_option_list
 %type <defelt>  alter_identity_column_option
+%type <node>	set_statistics_value
 
 %type <list>	createdb_opt_list createdb_opt_items copy_opt_list
 				transaction_mode_list
@@ -2436,18 +2437,18 @@ alter_table_cmd:
 					n->missing_ok = true;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column ColId SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS */
+			| ALTER opt_column ColId SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStatistics;
 					n->name = $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column Iconst SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS */
+			| ALTER opt_column Iconst SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
@@ -2459,7 +2460,7 @@ alter_table_cmd:
 
 					n->subtype = AT_SetStatistics;
 					n->num = (int16) $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET ( column_parameter = value [, ... ] ) */
@@ -3060,6 +3061,11 @@ alter_identity_column_option:
 				}
 		;
 
+set_statistics_value:
+			SignedIconst					{ $$ = (Node *) makeInteger($1); }
+			| DEFAULT						{ $$ = NULL; }
+		;
+
 PartitionBoundSpec:
 			/* a HASH partition */
 			FOR VALUES WITH '(' hash_partbound ')'
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index fc6d267e44..238d25d81d 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -878,23 +878,34 @@ get_attnum(Oid relid, const char *attname)
  *		Given the relation id and the attribute number,
  *		return the "attstattarget" field from the attribute relation.
  *
+ *		Returns	-1 if attstattarget is null.
+ *
+ *		Always returns 0 for system columns.
+ *
  *		Errors if not found.
  */
 int
 get_attstattarget(Oid relid, AttrNumber attnum)
 {
 	HeapTuple	tp;
-	Form_pg_attribute att_tup;
+	Datum		dat;
+	bool		isnull;
 	int			result;
 
+	if (attnum < 0)
+		return 0;
+
 	tp = SearchSysCache2(ATTNUM,
 						 ObjectIdGetDatum(relid),
 						 Int16GetDatum(attnum));
 	if (!HeapTupleIsValid(tp))
 		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
 			 attnum, relid);
-	att_tup = (Form_pg_attribute) GETSTRUCT(tp);
-	result = att_tup->attstattarget;
+	dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+	if (isnull)
+		result = -1;
+	else
+		result = DatumGetInt16(dat);
 	ReleaseSysCache(tp);
 	return result;
 }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8c0b5486b9..bc3651a73a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8682,7 +8682,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 						 tbinfo->dobj.name);
 			tbinfo->attnames[j] = pg_strdup(PQgetvalue(res, r, i_attname));
 			tbinfo->atttypnames[j] = pg_strdup(PQgetvalue(res, r, i_atttypname));
-			tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
+			if (PQgetisnull(res, r, i_attstattarget))
+				tbinfo->attstattarget[j] = -1;
+			else
+				tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
 			tbinfo->attstorage[j] = *(PQgetvalue(res, r, i_attstorage));
 			tbinfo->typstorage[j] = *(PQgetvalue(res, r, i_typstorage));
 			tbinfo->attidentity[j] = *(PQgetvalue(res, r, i_attidentity));
@@ -16261,7 +16264,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 			/*
 			 * Dump per-column statistics information. We only issue an ALTER
 			 * TABLE statement if the attstattarget entry for this column is
-			 * non-negative (i.e. it's not the default value)
+			 * not the default value.
 			 */
 			if (tbinfo->attstattarget[j] >= 0)
 				appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET STATISTICS %d;\n",
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 672a5a16ff..337a2d5bf9 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -158,22 +158,22 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	/* Number of times inherited from direct parent relation(s) */
 	int16		attinhcount BKI_DEFAULT(0);
 
+	/* attribute's collation, if any */
+	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
+
+#ifdef CATALOG_VARLEN			/* variable-length fields start here */
+	/* NOTE: The following fields are not present in tuple descriptors. */
+
 	/*
 	 * attstattarget is the target number of statistics datapoints to collect
 	 * during VACUUM ANALYZE of this column.  A zero here indicates that we do
-	 * not wish to collect any stats about this column. A "-1" here indicates
+	 * not wish to collect any stats about this column. A NULL here indicates
 	 * that no value has been explicitly set for this column, so ANALYZE
 	 * should use the default setting.
 	 *
 	 * int16 is sufficient for the current max value (MAX_STATISTICS_TARGET).
 	 */
-	int16		attstattarget BKI_DEFAULT(-1);
-
-	/* attribute's collation, if any */
-	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
-
-#ifdef CATALOG_VARLEN			/* variable-length fields start here */
-	/* NOTE: The following fields are not present in tuple descriptors. */
+	int16		attstattarget BKI_DEFAULT(_null_) BKI_FORCE_NULL;
 
 	/* Column-level access permissions */
 	aclitem		attacl[1] BKI_DEFAULT(_null_);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4af02940c5..ea096ee8a8 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -121,7 +121,7 @@ typedef struct VacAttrStats
 	 * than the underlying column/expression.  Therefore, use these fields for
 	 * information about the datatype being fed to the typanalyze function.
 	 */
-	int			attstattarget;
+	int			attstattarget;	/* -1 to use default */
 	Oid			attrtypid;		/* type of data being analyzed */
 	int32		attrtypmod;		/* typmod of data being analyzed */
 	Form_pg_type attrtype;		/* copy of pg_type row for attrtypid */
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index acfd9d1f4f..b63182d5e6 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2706,8 +2706,8 @@ SELECT attrelid::regclass, attnum, attstattarget
          attrelid          | attnum | attstattarget 
 ---------------------------+--------+---------------
  concur_exprs_index_expr   |      1 |           100
- concur_exprs_index_pred   |      1 |            -1
- concur_exprs_index_pred_2 |      1 |            -1
+ concur_exprs_index_pred   |      1 |              
+ concur_exprs_index_pred_2 |      1 |              
 (3 rows)
 
 DROP TABLE concur_exprs_tab;

base-commit: b8ba7344e9eb9bb505a92900b2a59257ef718135
-- 
2.43.0

#2Peter Eisentraut
peter@eisentraut.org
In reply to: Peter Eisentraut (#1)
3 attachment(s)
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.

Show quoted text

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]:
/messages/by-id/d07ffc2b-e0e8-77f7-38fb-be921dff71af@enterprisedb.com

Attachments:

v2-0001-Make-attstattarget-nullable.patchtext/plain; charset=UTF-8; name=v2-0001-Make-attstattarget-nullable.patchDownload
From f370eceec0cbb9b6bf76d3394e56a5df4280c906 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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-f2a9f1184c76@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 ] <replaceable class="parameter">column_name</replaceable> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> { SET GENERATED { ALWAYS | BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESTART [ [ WITH ] <replaceable class="parameter">restart</replaceable> ] } [...]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> DROP IDENTITY [ IF EXISTS ]
-    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
+    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS { <replaceable class="parameter">integer</replaceable> | DEFAULT }
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT }
@@ -317,7 +317,7 @@ <title>Description</title>
       sets the per-column statistics-gathering target for subsequent
       <link linkend="sql-analyze"><command>ANALYZE</command></link> operations.
       The target can be set in the range 0 to 10000; alternatively, set it
-      to -1 to revert to using the system default statistics
+      to <literal>DEFAULT</literal> to revert to using the system default statistics
       target (<xref linkend="guc-default-statistics-target"/>).
       For more information on the use of statistics by the
       <productname>PostgreSQL</productname> query planner, refer to
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 8826519e5e..054ccff1e2 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -453,8 +453,6 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 			return false;
 		if (attr1->atttypid != attr2->atttypid)
 			return false;
-		if (attr1->attstattarget != attr2->attstattarget)
-			return false;
 		if (attr1->attlen != attr2->attlen)
 			return false;
 		if (attr1->attndims != attr2->attndims)
@@ -639,7 +637,6 @@ TupleDescInitEntry(TupleDesc desc,
 	else if (attributeName != NameStr(att->attname))
 		namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
@@ -702,7 +699,6 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
 	Assert(attributeName != NULL);
 	namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index e01dca9b7c..c7546da51e 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -552,7 +552,6 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
 	if (OidIsValid(attrtypes[attnum]->attcollation))
 		attrtypes[attnum]->attcollation = C_COLLATION_OID;
 
-	attrtypes[attnum]->attstattarget = -1;
 	attrtypes[attnum]->attcacheoff = -1;
 	attrtypes[attnum]->atttypmod = -1;
 	attrtypes[attnum]->attislocal = true;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 380bc23c82..13cd2fee14 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -840,7 +840,6 @@ sub gen_pg_attribute
 				my %row;
 				$row{attnum} = $attnum;
 				$row{attrelid} = $table->{relation_oid};
-				$row{attstattarget} = '0';
 
 				morph_row_for_pgattr(\%row, $schema, $attr, 1);
 				print_bki_insert(\%row, $schema);
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b93894889d..52b4485c4b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -749,14 +749,16 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attisdropped - 1] = BoolGetDatum(attrs->attisdropped);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
-		slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attrs->attstattarget);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attoptions && attoptions[natts] != (Datum) 0)
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
 		else
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 
-		/* start out with empty permissions and empty options */
+		/*
+		 * The remaining fields are not set for new columns.
+		 */
+		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
@@ -818,9 +820,6 @@ AddNewAttributeTuples(Oid new_rel_oid,
 
 	indstate = CatalogOpenIndexes(rel);
 
-	/* set stats detail level to a sane default */
-	for (int i = 0; i < natts; i++)
-		tupdesc->attrs[i].attstattarget = -1;
 	InsertPgAttributeTuples(rel, tupdesc, new_rel_oid, NULL, indstate);
 
 	/* add dependencies on their datatypes and collations */
@@ -1685,9 +1684,6 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	/* Remove any not-null constraint the column may have */
 	attStruct->attnotnull = false;
 
-	/* We don't want to keep stats for it anymore */
-	attStruct->attstattarget = 0;
-
 	/* Unset this so no one tries to look up the generation expression */
 	attStruct->attgenerated = '\0';
 
@@ -1707,6 +1703,8 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	 * Clear the other variable-length fields.  This saves some space in
 	 * pg_attribute and removes no longer useful information.
 	 */
+	nullsAtt[Anum_pg_attribute_attstattarget - 1] = true;
+	replacesAtt[Anum_pg_attribute_attstattarget - 1] = true;
 	nullsAtt[Anum_pg_attribute_attacl - 1] = true;
 	replacesAtt[Anum_pg_attribute_attacl - 1] = true;
 	nullsAtt[Anum_pg_attribute_attoptions - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7b186c0220..b2759df311 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -325,7 +325,6 @@ ConstructTupleDescriptor(Relation heapRelation,
 
 		MemSet(to, 0, ATTRIBUTE_FIXED_PART_SIZE);
 		to->attnum = i + 1;
-		to->attstattarget = -1;
 		to->attcacheoff = -1;
 		to->attislocal = true;
 		to->attcollation = (i < numkeyatts) ? collationIds[i] : InvalidOid;
@@ -1780,10 +1779,12 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
 		{
 			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
+			HeapTuple	tp;
+			Datum		dat;
+			bool		isnull;
 			Datum		repl_val[Natts_pg_attribute];
 			bool		repl_null[Natts_pg_attribute];
 			bool		repl_repl[Natts_pg_attribute];
-			int			attstattarget;
 			HeapTuple	newTuple;
 
 			/* Ignore dropped columns */
@@ -1793,10 +1794,18 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 			/*
 			 * Get attstattarget from the old index and refresh the new value.
 			 */
-			attstattarget = get_attstattarget(oldIndexId, att->attnum);
+			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
+			if (!HeapTupleIsValid(tp))
+				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+					 att->attnum, oldIndexId);
+			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+			ReleaseSysCache(tp);
 
-			/* no need for a refresh if both match */
-			if (attstattarget == att->attstattarget)
+			/*
+			 * No need for a refresh if old index value is null.  (All new
+			 * index values are null at this point.)
+			 */
+			if (isnull)
 				continue;
 
 			memset(repl_val, 0, sizeof(repl_val));
@@ -1804,7 +1813,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 			memset(repl_repl, false, sizeof(repl_repl));
 
 			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attstattarget);
+			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
 
 			newTuple = heap_modify_tuple(attrTuple,
 										 RelationGetDescr(pg_attribute),
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 1f4a951681..fd2202cbb8 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1004,6 +1004,7 @@ static VacAttrStats *
 examine_attribute(Relation onerel, int attnum, Node *index_expr)
 {
 	Form_pg_attribute attr = TupleDescAttr(onerel->rd_att, attnum - 1);
+	int			attstattarget;
 	HeapTuple	typtuple;
 	VacAttrStats *stats;
 	int			i;
@@ -1013,15 +1014,17 @@ examine_attribute(Relation onerel, int attnum, Node *index_expr)
 	if (attr->attisdropped)
 		return NULL;
 
+	attstattarget = get_attstattarget(RelationGetRelid(onerel), attnum);
+
 	/* Don't analyze column if user has specified not to */
-	if (attr->attstattarget == 0)
+	if (attstattarget == 0)
 		return NULL;
 
 	/*
 	 * Create the VacAttrStats struct.
 	 */
 	stats = (VacAttrStats *) palloc0(sizeof(VacAttrStats));
-	stats->attstattarget = attr->attstattarget;
+	stats->attstattarget = attstattarget;
 
 	/*
 	 * When analyzing an expression index, believe the expression tree's type
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6b0a20010e..77bb17c479 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7124,7 +7124,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.attrelid = myrelid;
 	namestrcpy(&(attribute.attname), colDef->colname);
 	attribute.atttypid = typeOid;
-	attribute.attstattarget = -1;
 	attribute.attlen = tform->typlen;
 	attribute.attnum = newattnum;
 	if (list_length(colDef->typeName->arrayBounds) > PG_INT16_MAX)
@@ -8453,10 +8452,14 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 {
 	int			newtarget;
 	Relation	attrelation;
-	HeapTuple	tuple;
+	HeapTuple	tuple,
+				newtuple;
 	Form_pg_attribute attrtuple;
 	AttrNumber	attnum;
 	ObjectAddress address;
+	Datum		repl_val[Natts_pg_attribute];
+	bool		repl_null[Natts_pg_attribute];
+	bool		repl_repl[Natts_pg_attribute];
 
 	/*
 	 * We allow referencing columns by numbers only for indexes, since table
@@ -8469,8 +8472,18 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot refer to non-index column by number")));
 
-	Assert(IsA(newValue, Integer));
-	newtarget = intVal(newValue);
+	if (newValue)
+	{
+		Assert(IsA(newValue, Integer));
+		newtarget = intVal(newValue);
+	}
+	else
+	{
+		/*
+		 * -1 was used in previous versions to represent the default setting
+		 */
+		newtarget = -1;
+	}
 
 	/*
 	 * Limit target to a sane range
@@ -8495,7 +8508,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 
 	if (colName)
 	{
-		tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
+		tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8505,7 +8518,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 	}
 	else
 	{
-		tuple = SearchSysCacheCopyAttNum(RelationGetRelid(rel), colNum);
+		tuple = SearchSysCacheAttNum(RelationGetRelid(rel), colNum);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8539,16 +8552,27 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 					 errhint("Alter statistics on table column instead.")));
 	}
 
-	attrtuple->attstattarget = newtarget;
-
-	CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
+	/* Build new tuple. */
+	memset(repl_null, false, sizeof(repl_null));
+	memset(repl_repl, false, sizeof(repl_repl));
+	if (newtarget != -1)
+		repl_val[Anum_pg_attribute_attstattarget - 1] = newtarget;
+	else
+		repl_null[Anum_pg_attribute_attstattarget - 1] = true;
+	repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
+	newtuple = heap_modify_tuple(tuple, RelationGetDescr(attrelation),
+								 repl_val, repl_null, repl_repl);
+	CatalogTupleUpdate(attrelation, &tuple->t_self, newtuple);
 
 	InvokeObjectPostAlterHook(RelationRelationId,
 							  RelationGetRelid(rel),
 							  attrtuple->attnum);
 	ObjectAddressSubSet(address, RelationRelationId,
 						RelationGetRelid(rel), attnum);
-	heap_freetuple(tuple);
+
+	heap_freetuple(newtuple);
+
+	ReleaseSysCache(tuple);
 
 	table_close(attrelation, RowExclusiveLock);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 63f172e175..b6f23e26e7 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -337,6 +337,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <list>	alter_table_cmds alter_type_cmds
 %type <list>    alter_identity_column_option_list
 %type <defelt>  alter_identity_column_option
+%type <node>	set_statistics_value
 
 %type <list>	createdb_opt_list createdb_opt_items copy_opt_list
 				transaction_mode_list
@@ -2436,18 +2437,18 @@ alter_table_cmd:
 					n->missing_ok = true;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column ColId SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS */
+			| ALTER opt_column ColId SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStatistics;
 					n->name = $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column Iconst SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS */
+			| ALTER opt_column Iconst SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
@@ -2459,7 +2460,7 @@ alter_table_cmd:
 
 					n->subtype = AT_SetStatistics;
 					n->num = (int16) $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET ( column_parameter = value [, ... ] ) */
@@ -3060,6 +3061,11 @@ alter_identity_column_option:
 				}
 		;
 
+set_statistics_value:
+			SignedIconst					{ $$ = (Node *) makeInteger($1); }
+			| DEFAULT						{ $$ = NULL; }
+		;
+
 PartitionBoundSpec:
 			/* a HASH partition */
 			FOR VALUES WITH '(' hash_partbound ')'
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index fc6d267e44..41f877a305 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -878,23 +878,39 @@ get_attnum(Oid relid, const char *attname)
  *		Given the relation id and the attribute number,
  *		return the "attstattarget" field from the attribute relation.
  *
+ *		Returns -1 if attstattarget is null, except 0 for dropped columns.
+ *
+ *		Always returns 0 for system columns.
+ *
  *		Errors if not found.
  */
 int
 get_attstattarget(Oid relid, AttrNumber attnum)
 {
 	HeapTuple	tp;
-	Form_pg_attribute att_tup;
+	Datum		dat;
+	bool		isnull;
 	int			result;
 
+	if (attnum < 0)
+		return 0;
+
 	tp = SearchSysCache2(ATTNUM,
 						 ObjectIdGetDatum(relid),
 						 Int16GetDatum(attnum));
 	if (!HeapTupleIsValid(tp))
 		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
 			 attnum, relid);
-	att_tup = (Form_pg_attribute) GETSTRUCT(tp);
-	result = att_tup->attstattarget;
+	dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+	if (isnull)
+	{
+		if (((Form_pg_attribute) GETSTRUCT(tp))->attisdropped)
+			result = 0;
+		else
+			result = -1;
+	}
+	else
+		result = DatumGetInt16(dat);
 	ReleaseSysCache(tp);
 	return result;
 }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8c0b5486b9..bc3651a73a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8682,7 +8682,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 						 tbinfo->dobj.name);
 			tbinfo->attnames[j] = pg_strdup(PQgetvalue(res, r, i_attname));
 			tbinfo->atttypnames[j] = pg_strdup(PQgetvalue(res, r, i_atttypname));
-			tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
+			if (PQgetisnull(res, r, i_attstattarget))
+				tbinfo->attstattarget[j] = -1;
+			else
+				tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
 			tbinfo->attstorage[j] = *(PQgetvalue(res, r, i_attstorage));
 			tbinfo->typstorage[j] = *(PQgetvalue(res, r, i_typstorage));
 			tbinfo->attidentity[j] = *(PQgetvalue(res, r, i_attidentity));
@@ -16261,7 +16264,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 			/*
 			 * Dump per-column statistics information. We only issue an ALTER
 			 * TABLE statement if the attstattarget entry for this column is
-			 * non-negative (i.e. it's not the default value)
+			 * not the default value.
 			 */
 			if (tbinfo->attstattarget[j] >= 0)
 				appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET STATISTICS %d;\n",
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 672a5a16ff..337a2d5bf9 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -158,22 +158,22 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	/* Number of times inherited from direct parent relation(s) */
 	int16		attinhcount BKI_DEFAULT(0);
 
+	/* attribute's collation, if any */
+	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
+
+#ifdef CATALOG_VARLEN			/* variable-length fields start here */
+	/* NOTE: The following fields are not present in tuple descriptors. */
+
 	/*
 	 * attstattarget is the target number of statistics datapoints to collect
 	 * during VACUUM ANALYZE of this column.  A zero here indicates that we do
-	 * not wish to collect any stats about this column. A "-1" here indicates
+	 * not wish to collect any stats about this column. A NULL here indicates
 	 * that no value has been explicitly set for this column, so ANALYZE
 	 * should use the default setting.
 	 *
 	 * int16 is sufficient for the current max value (MAX_STATISTICS_TARGET).
 	 */
-	int16		attstattarget BKI_DEFAULT(-1);
-
-	/* attribute's collation, if any */
-	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
-
-#ifdef CATALOG_VARLEN			/* variable-length fields start here */
-	/* NOTE: The following fields are not present in tuple descriptors. */
+	int16		attstattarget BKI_DEFAULT(_null_) BKI_FORCE_NULL;
 
 	/* Column-level access permissions */
 	aclitem		attacl[1] BKI_DEFAULT(_null_);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4af02940c5..ea096ee8a8 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -121,7 +121,7 @@ typedef struct VacAttrStats
 	 * than the underlying column/expression.  Therefore, use these fields for
 	 * information about the datatype being fed to the typanalyze function.
 	 */
-	int			attstattarget;
+	int			attstattarget;	/* -1 to use default */
 	Oid			attrtypid;		/* type of data being analyzed */
 	int32		attrtypmod;		/* typmod of data being analyzed */
 	Form_pg_type attrtype;		/* copy of pg_type row for attrtypid */
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 446cfa678b..79fa117cb5 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2707,8 +2707,8 @@ SELECT attrelid::regclass, attnum, attstattarget
          attrelid          | attnum | attstattarget 
 ---------------------------+--------+---------------
  concur_exprs_index_expr   |      1 |           100
- concur_exprs_index_pred   |      1 |            -1
- concur_exprs_index_pred_2 |      1 |            -1
+ concur_exprs_index_pred   |      1 |              
+ concur_exprs_index_pred_2 |      1 |              
 (3 rows)
 
 DROP TABLE concur_exprs_tab;

base-commit: 3e2e0d5ad7fcb89d18a71cbfc885ef184e1b6f2e
-- 
2.43.0

v2-0002-WIP-Generalize-handling-of-nullable-pg_attribute-.patchtext/plain; charset=UTF-8; name=v2-0002-WIP-Generalize-handling-of-nullable-pg_attribute-.patchDownload
From 5f1f4bd9518a9c10c74de775125ea3549ba38d8c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 11:41:17 +0100
Subject: [PATCH v2 2/3] WIP: Generalize handling of nullable pg_attribute
 columns in DDL

DDL code uses tuple descriptors to pass around pg_attribute values
during table and index creation.  But tuple descriptors don't include
the variable-length/nullable columns of pg_attribute, so they have to
be handled separately.  Right now, the attoptions field is handled in
a one-off way with a separate argument passed to
InsertPgAttributeTuples().  The other affected fields of pg_attribute
are right now not needed at relation creation time.

The goal of this patch is to generalize this to allow handling
additional variable-length/nullable columns of pg_attribute in a
similar manner.  For that, create a new struct
Form_pg_attribute_extra, which is to be passed around in parallel to
the tuple descriptor and optionally supplies the additional columns.
Right now, this struct only contains one field for attoptions, so no
functionality is actually changed by this.
---
 src/backend/catalog/heap.c         | 12 +++++++++---
 src/backend/catalog/index.c        | 16 +++++++++++++++-
 src/include/catalog/heap.h         |  2 +-
 src/include/catalog/pg_attribute.h |  7 +++++++
 4 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 52b4485c4b..40b23fbead 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -697,7 +697,7 @@ void
 InsertPgAttributeTuples(Relation pg_attribute_rel,
 						TupleDesc tupdesc,
 						Oid new_rel_oid,
-						const Datum *attoptions,
+						const FormData_pg_attribute_extra tupdesc_extra[],
 						CatalogIndexState indstate)
 {
 	TupleTableSlot **slot;
@@ -719,6 +719,7 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 	while (natts < tupdesc->natts)
 	{
 		Form_pg_attribute attrs = TupleDescAttr(tupdesc, natts);
+		const FormData_pg_attribute_extra *attrs_extra = tupdesc_extra ? &tupdesc_extra[natts] : NULL;
 
 		ExecClearTuple(slot[slotCount]);
 
@@ -750,10 +751,15 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
-		if (attoptions && attoptions[natts] != (Datum) 0)
-			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
+		if (attrs_extra)
+		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
+		}
 		else
+		{
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
+		}
 
 		/*
 		 * The remaining fields are not set for new columns.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b2759df311..eaeae568a3 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -517,6 +517,20 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
 	TupleDesc	indexTupDesc;
+	FormData_pg_attribute_extra *attrs_extra = NULL;
+
+	if (attopts)
+	{
+		attrs_extra = palloc0_array(FormData_pg_attribute_extra, indexRelation->rd_att->natts);
+
+		for (int i = 0; i < indexRelation->rd_att->natts; i++)
+		{
+			if (attopts[i])
+				attrs_extra[i].attoptions.value = attopts[i];
+			else
+				attrs_extra[i].attoptions.isnull = true;
+		}
+	}
 
 	/*
 	 * open the attribute relation and its indexes
@@ -530,7 +544,7 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	 */
 	indexTupDesc = RelationGetDescr(indexRelation);
 
-	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attopts, indstate);
+	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attrs_extra, indstate);
 
 	CatalogCloseIndexes(indstate);
 
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 51f7b12aa3..dbd65c4d5e 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -98,7 +98,7 @@ extern List *heap_truncate_find_FKs(List *relationIds);
 extern void InsertPgAttributeTuples(Relation pg_attribute_rel,
 									TupleDesc tupdesc,
 									Oid new_rel_oid,
-									const Datum *attoptions,
+									const FormData_pg_attribute_extra tupdesc_extra[],
 									CatalogIndexState indstate);
 
 extern void InsertPgClassTuple(Relation pg_class_desc,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 337a2d5bf9..7336d7f2fd 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -208,6 +208,13 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
  */
 typedef FormData_pg_attribute *Form_pg_attribute;
 
+typedef struct FormData_pg_attribute_extra
+{
+	NullableDatum attoptions;
+} FormData_pg_attribute_extra;
+
+typedef FormData_pg_attribute_extra *Form_pg_attribute_extra;
+
 DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, pg_attribute, btree(attrelid oid_ops, attname name_ops));
 DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, pg_attribute, btree(attrelid oid_ops, attnum int2_ops));
 
-- 
2.43.0

v2-0003-WIP-Add-attstattarget-to-Form_pg_attribute_extra.patchtext/plain; charset=UTF-8; name=v2-0003-WIP-Add-attstattarget-to-Form_pg_attribute_extra.patchDownload
From eb616c2156163a3ca1d74f122377987b6c358764 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 12:14:42 +0100
Subject: [PATCH v2 3/3] WIP: Add attstattarget to Form_pg_attribute_extra

This allows setting attstattarget when a relation is created.

We make use of this by having index_concurrently_create_copy() copy
over the attstattarget values when the new index is created, instead
of having index_concurrently_swap() fix it up later.
---
 src/backend/catalog/heap.c         |  5 +-
 src/backend/catalog/index.c        | 97 +++++++++---------------------
 src/backend/catalog/toasting.c     |  2 +-
 src/backend/commands/indexcmds.c   |  2 +-
 src/include/catalog/index.h        |  1 +
 src/include/catalog/pg_attribute.h |  1 +
 6 files changed, 36 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 40b23fbead..f61c510b8d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -753,18 +753,21 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attrs_extra)
 		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.isnull;
+
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
 		}
 		else
 		{
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 		}
 
 		/*
 		 * The remaining fields are not set for new columns.
 		 */
-		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index eaeae568a3..e2ee775df1 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -112,7 +112,7 @@ static TupleDesc ConstructTupleDescriptor(Relation heapRelation,
 										  const Oid *opclassIds);
 static void InitializeAttributeOids(Relation indexRelation,
 									int numatts, Oid indexoid);
-static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts);
+static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets);
 static void UpdateIndexRelation(Oid indexoid, Oid heapoid,
 								Oid parentIndexId,
 								const IndexInfo *indexInfo,
@@ -512,7 +512,7 @@ InitializeAttributeOids(Relation indexRelation,
  * ----------------------------------------------------------------
  */
 static void
-AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
+AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets)
 {
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
@@ -529,6 +529,11 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 				attrs_extra[i].attoptions.value = attopts[i];
 			else
 				attrs_extra[i].attoptions.isnull = true;
+
+			if (stattargets)
+				attrs_extra[i].attstattarget = stattargets[i];
+			else
+				attrs_extra[i].attstattarget.isnull = true;
 		}
 	}
 
@@ -735,6 +740,7 @@ index_create(Relation heapRelation,
 			 const Oid *opclassIds,
 			 const Datum *opclassOptions,
 			 const int16 *coloptions,
+			 const NullableDatum *stattargets,
 			 Datum reloptions,
 			 bits16 flags,
 			 bits16 constr_flags,
@@ -1029,7 +1035,7 @@ index_create(Relation heapRelation,
 	/*
 	 * append ATTRIBUTE tuples for the index
 	 */
-	AppendAttributeTuples(indexRelation, opclassOptions);
+	AppendAttributeTuples(indexRelation, opclassOptions, stattargets);
 
 	/* ----------------
 	 *	  update pg_index
@@ -1308,6 +1314,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	Datum	   *opclassOptions;
 	oidvector  *indclass;
 	int2vector *indcoloptions;
+	NullableDatum *stattargets;
 	bool		isnull;
 	List	   *indexColNames = NIL;
 	List	   *indexExprs = NIL;
@@ -1412,6 +1419,23 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
 		opclassOptions[i] = get_attoptions(oldIndexId, i + 1);
 
+	/* Extra statistic targets for each attribute */
+	stattargets = palloc0_array(NullableDatum, newInfo->ii_NumIndexAttrs);
+	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
+	{
+		HeapTuple   tp;
+		Datum       dat;
+
+		tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(i + 1));
+		if (!HeapTupleIsValid(tp))
+			elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+				 i + 1, oldIndexId);
+		dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+		ReleaseSysCache(tp);
+		stattargets[i].value = dat;
+		stattargets[i].isnull = isnull;
+	}
+
 	/*
 	 * Now create the new index.
 	 *
@@ -1433,6 +1457,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							  indclass->values,
 							  opclassOptions,
 							  indcoloptions->values,
+							  stattargets,
 							  reloptionsDatum,
 							  INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
 							  0,
@@ -1775,72 +1800,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	/* Copy data of pg_statistic from the old index to the new one */
 	CopyStatistics(oldIndexId, newIndexId);
 
-	/* Copy pg_attribute.attstattarget for each index attribute */
-	{
-		HeapTuple	attrTuple;
-		Relation	pg_attribute;
-		SysScanDesc scan;
-		ScanKeyData key[1];
-
-		pg_attribute = table_open(AttributeRelationId, RowExclusiveLock);
-		ScanKeyInit(&key[0],
-					Anum_pg_attribute_attrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(newIndexId));
-		scan = systable_beginscan(pg_attribute, AttributeRelidNumIndexId,
-								  true, NULL, 1, key);
-
-		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
-		{
-			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
-			HeapTuple	tp;
-			Datum		dat;
-			bool		isnull;
-			Datum		repl_val[Natts_pg_attribute];
-			bool		repl_null[Natts_pg_attribute];
-			bool		repl_repl[Natts_pg_attribute];
-			HeapTuple	newTuple;
-
-			/* Ignore dropped columns */
-			if (att->attisdropped)
-				continue;
-
-			/*
-			 * Get attstattarget from the old index and refresh the new value.
-			 */
-			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
-			if (!HeapTupleIsValid(tp))
-				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-					 att->attnum, oldIndexId);
-			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
-			ReleaseSysCache(tp);
-
-			/*
-			 * No need for a refresh if old index value is null.  (All new
-			 * index values are null at this point.)
-			 */
-			if (isnull)
-				continue;
-
-			memset(repl_val, 0, sizeof(repl_val));
-			memset(repl_null, false, sizeof(repl_null));
-			memset(repl_repl, false, sizeof(repl_repl));
-
-			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
-
-			newTuple = heap_modify_tuple(attrTuple,
-										 RelationGetDescr(pg_attribute),
-										 repl_val, repl_null, repl_repl);
-			CatalogTupleUpdate(pg_attribute, &newTuple->t_self, newTuple);
-
-			heap_freetuple(newTuple);
-		}
-
-		systable_endscan(scan);
-		table_close(pg_attribute, RowExclusiveLock);
-	}
-
 	/* Close relations */
 	table_close(pg_class, RowExclusiveLock);
 	table_close(pg_index, RowExclusiveLock);
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 989f820791..35a0dbeeb0 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -326,7 +326,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 				 list_make2("chunk_id", "chunk_seq"),
 				 BTREE_AM_OID,
 				 rel->rd_rel->reltablespace,
-				 collationIds, opclassIds, NULL, coloptions, (Datum) 0,
+				 collationIds, opclassIds, NULL, coloptions, NULL, (Datum) 0,
 				 INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
 
 	table_close(toast_rel, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e56205abd8..dbd363589f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1193,7 +1193,7 @@ DefineIndex(Oid tableId,
 					 stmt->oldNumber, indexInfo, indexColNames,
 					 accessMethodId, tablespaceId,
 					 collationIds, opclassIds, opclassOptions,
-					 coloptions, reloptions,
+					 coloptions, NULL, reloptions,
 					 flags, constr_flags,
 					 allowSystemTableMods, !check_rights,
 					 &createdConstraintId);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 41a0b9aee1..cb9d380616 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -80,6 +80,7 @@ extern Oid	index_create(Relation heapRelation,
 						 const Oid *opclassIds,
 						 const Datum *opclassOptions,
 						 const int16 *coloptions,
+						 const NullableDatum *stattargets,
 						 Datum reloptions,
 						 bits16 flags,
 						 bits16 constr_flags,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 7336d7f2fd..98ab4ae538 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -210,6 +210,7 @@ typedef FormData_pg_attribute *Form_pg_attribute;
 
 typedef struct FormData_pg_attribute_extra
 {
+	NullableDatum attstattarget;
 	NullableDatum attoptions;
 } FormData_pg_attribute_extra;
 
-- 
2.43.0

#3Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Peter Eisentraut (#2)
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/

#4Peter Eisentraut
peter@eisentraut.org
In reply to: Alvaro Herrera (#3)
3 attachment(s)
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.

Attachments:

v3-0001-Make-attstattarget-nullable.patchtext/plain; charset=UTF-8; name=v3-0001-Make-attstattarget-nullable.patchDownload
From d937c26d8c471c999aa53c96dce86c68fad71a7a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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 columns, the field is now always null.  The ANALYZE code
skips system columns anyway.

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-f2a9f1184c76@eisentraut.org

TODO: catversion
---
 doc/src/sgml/ref/alter_table.sgml          | 10 +++--
 src/backend/access/common/tupdesc.c        |  4 --
 src/backend/bootstrap/bootstrap.c          |  1 -
 src/backend/catalog/genbki.pl              |  1 -
 src/backend/catalog/heap.c                 | 18 ++++-----
 src/backend/catalog/index.c                | 21 ++++++++---
 src/backend/commands/analyze.c             | 21 ++++++++++-
 src/backend/commands/tablecmds.c           | 44 +++++++++++++++++-----
 src/backend/parser/gram.y                  | 18 ++++++---
 src/backend/utils/cache/lsyscache.c        | 27 -------------
 src/bin/pg_dump/pg_dump.c                  |  7 +++-
 src/include/catalog/pg_attribute.h         | 16 ++++----
 src/include/commands/vacuum.h              |  2 +-
 src/include/utils/lsyscache.h              |  1 -
 src/test/regress/expected/create_index.out |  4 +-
 15 files changed, 110 insertions(+), 85 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index eaada230248..9670671107e 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -51,7 +51,7 @@
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> { SET GENERATED { ALWAYS | BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESTART [ [ WITH ] <replaceable class="parameter">restart</replaceable> ] } [...]
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> DROP IDENTITY [ IF EXISTS ]
-    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
+    ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS { <replaceable class="parameter">integer</replaceable> | DEFAULT }
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
     ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT }
@@ -328,9 +328,11 @@ <title>Description</title>
       This form
       sets the per-column statistics-gathering target for subsequent
       <link linkend="sql-analyze"><command>ANALYZE</command></link> operations.
-      The target can be set in the range 0 to 10000; alternatively, set it
-      to -1 to revert to using the system default statistics
-      target (<xref linkend="guc-default-statistics-target"/>).
+      The target can be set in the range 0 to 10000.  Set it
+      to <literal>DEFAULT</literal> to revert to using the system default
+      statistics target (<xref linkend="guc-default-statistics-target"/>).
+      (Setting to a value of -1 is an obsolete way spelling to get the same
+      outcome.)
       For more information on the use of statistics by the
       <productname>PostgreSQL</productname> query planner, refer to
       <xref linkend="planner-stats"/>.
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 6e7ad79834f..bc80caee117 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -453,8 +453,6 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 			return false;
 		if (attr1->atttypid != attr2->atttypid)
 			return false;
-		if (attr1->attstattarget != attr2->attstattarget)
-			return false;
 		if (attr1->attlen != attr2->attlen)
 			return false;
 		if (attr1->attndims != attr2->attndims)
@@ -639,7 +637,6 @@ TupleDescInitEntry(TupleDesc desc,
 	else if (attributeName != NameStr(att->attname))
 		namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
@@ -702,7 +699,6 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
 	Assert(attributeName != NULL);
 	namestrcpy(&(att->attname), attributeName);
 
-	att->attstattarget = -1;
 	att->attcacheoff = -1;
 	att->atttypmod = typmod;
 
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 55c53d01f87..141b25ddd7a 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -552,7 +552,6 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
 	if (OidIsValid(attrtypes[attnum]->attcollation))
 		attrtypes[attnum]->attcollation = C_COLLATION_OID;
 
-	attrtypes[attnum]->attstattarget = -1;
 	attrtypes[attnum]->attcacheoff = -1;
 	attrtypes[attnum]->atttypmod = -1;
 	attrtypes[attnum]->attislocal = true;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 531d7cd0d52..93553e8c3c4 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -840,7 +840,6 @@ sub gen_pg_attribute
 				my %row;
 				$row{attnum} = $attnum;
 				$row{attrelid} = $table->{relation_oid};
-				$row{attstattarget} = '0';
 
 				morph_row_for_pgattr(\%row, $schema, $attr, 1);
 				print_bki_insert(\%row, $schema);
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e80a90ef4c0..45a71081d42 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -749,14 +749,16 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attisdropped - 1] = BoolGetDatum(attrs->attisdropped);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
-		slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attrs->attstattarget);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attoptions && attoptions[natts] != (Datum) 0)
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
 		else
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 
-		/* start out with empty permissions and empty options */
+		/*
+		 * The remaining fields are not set for new columns.
+		 */
+		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
@@ -818,9 +820,6 @@ AddNewAttributeTuples(Oid new_rel_oid,
 
 	indstate = CatalogOpenIndexes(rel);
 
-	/* set stats detail level to a sane default */
-	for (int i = 0; i < natts; i++)
-		tupdesc->attrs[i].attstattarget = -1;
 	InsertPgAttributeTuples(rel, tupdesc, new_rel_oid, NULL, indstate);
 
 	/* add dependencies on their datatypes and collations */
@@ -1685,9 +1684,6 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	/* Remove any not-null constraint the column may have */
 	attStruct->attnotnull = false;
 
-	/* We don't want to keep stats for it anymore */
-	attStruct->attstattarget = 0;
-
 	/* Unset this so no one tries to look up the generation expression */
 	attStruct->attgenerated = '\0';
 
@@ -1704,9 +1700,11 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
 
 	/*
-	 * Clear the other variable-length fields.  This saves some space in
-	 * pg_attribute and removes no longer useful information.
+	 * Clear the other nullable fields.  This saves some space in pg_attribute
+	 * and removes no longer useful information.
 	 */
+	nullsAtt[Anum_pg_attribute_attstattarget - 1] = true;
+	replacesAtt[Anum_pg_attribute_attstattarget - 1] = true;
 	nullsAtt[Anum_pg_attribute_attacl - 1] = true;
 	replacesAtt[Anum_pg_attribute_attacl - 1] = true;
 	nullsAtt[Anum_pg_attribute_attoptions - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 88f7994b5a6..fbef3d5382d 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -325,7 +325,6 @@ ConstructTupleDescriptor(Relation heapRelation,
 
 		MemSet(to, 0, ATTRIBUTE_FIXED_PART_SIZE);
 		to->attnum = i + 1;
-		to->attstattarget = -1;
 		to->attcacheoff = -1;
 		to->attislocal = true;
 		to->attcollation = (i < numkeyatts) ? collationIds[i] : InvalidOid;
@@ -1780,10 +1779,12 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
 		{
 			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
+			HeapTuple	tp;
+			Datum		dat;
+			bool		isnull;
 			Datum		repl_val[Natts_pg_attribute];
 			bool		repl_null[Natts_pg_attribute];
 			bool		repl_repl[Natts_pg_attribute];
-			int			attstattarget;
 			HeapTuple	newTuple;
 
 			/* Ignore dropped columns */
@@ -1793,10 +1794,18 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 			/*
 			 * Get attstattarget from the old index and refresh the new value.
 			 */
-			attstattarget = get_attstattarget(oldIndexId, att->attnum);
+			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
+			if (!HeapTupleIsValid(tp))
+				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+					 att->attnum, oldIndexId);
+			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+			ReleaseSysCache(tp);
 
-			/* no need for a refresh if both match */
-			if (attstattarget == att->attstattarget)
+			/*
+			 * No need for a refresh if old index value is null.  (All new
+			 * index values are null at this point.)
+			 */
+			if (isnull)
 				continue;
 
 			memset(repl_val, 0, sizeof(repl_val));
@@ -1804,7 +1813,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 			memset(repl_repl, false, sizeof(repl_repl));
 
 			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = Int16GetDatum(attstattarget);
+			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
 
 			newTuple = heap_modify_tuple(attrTuple,
 										 RelationGetDescr(pg_attribute),
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index deef865ce6d..a03495d6c95 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1004,6 +1004,10 @@ static VacAttrStats *
 examine_attribute(Relation onerel, int attnum, Node *index_expr)
 {
 	Form_pg_attribute attr = TupleDescAttr(onerel->rd_att, attnum - 1);
+	int			attstattarget;
+	HeapTuple	atttuple;
+	Datum		dat;
+	bool		isnull;
 	HeapTuple	typtuple;
 	VacAttrStats *stats;
 	int			i;
@@ -1013,15 +1017,28 @@ examine_attribute(Relation onerel, int attnum, Node *index_expr)
 	if (attr->attisdropped)
 		return NULL;
 
+	/*
+	 * Get attstattarget value.  Set to -1 if null.  (Analyze functions expect
+	 * -1 to mean use default_statistics_target; see for example
+	 * std_typanalyze.)
+	 */
+	atttuple = SearchSysCache2(ATTNUM, ObjectIdGetDatum(RelationGetRelid(onerel)), Int16GetDatum(attnum));
+	if (!HeapTupleIsValid(atttuple))
+		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+			 attnum, RelationGetRelid(onerel));
+	dat = SysCacheGetAttr(ATTNUM, atttuple, Anum_pg_attribute_attstattarget, &isnull);
+	attstattarget = isnull ? -1 : DatumGetInt16(dat);
+	ReleaseSysCache(atttuple);
+
 	/* Don't analyze column if user has specified not to */
-	if (attr->attstattarget == 0)
+	if (attstattarget == 0)
 		return NULL;
 
 	/*
 	 * Create the VacAttrStats struct.
 	 */
 	stats = (VacAttrStats *) palloc0(sizeof(VacAttrStats));
-	stats->attstattarget = attr->attstattarget;
+	stats->attstattarget = attstattarget;
 
 	/*
 	 * When analyzing an expression index, believe the expression tree's type
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2822b2bb440..86a70599062 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7143,7 +7143,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.attrelid = myrelid;
 	namestrcpy(&(attribute.attname), colDef->colname);
 	attribute.atttypid = typeOid;
-	attribute.attstattarget = -1;
 	attribute.attlen = tform->typlen;
 	attribute.attnum = newattnum;
 	if (list_length(colDef->typeName->arrayBounds) > PG_INT16_MAX)
@@ -8588,10 +8587,14 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 {
 	int			newtarget;
 	Relation	attrelation;
-	HeapTuple	tuple;
+	HeapTuple	tuple,
+				newtuple;
 	Form_pg_attribute attrtuple;
 	AttrNumber	attnum;
 	ObjectAddress address;
+	Datum		repl_val[Natts_pg_attribute];
+	bool		repl_null[Natts_pg_attribute];
+	bool		repl_repl[Natts_pg_attribute];
 
 	/*
 	 * We allow referencing columns by numbers only for indexes, since table
@@ -8604,8 +8607,18 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot refer to non-index column by number")));
 
-	Assert(IsA(newValue, Integer));
-	newtarget = intVal(newValue);
+	if (newValue)
+	{
+		Assert(IsA(newValue, Integer));
+		newtarget = intVal(newValue);
+	}
+	else
+	{
+		/*
+		 * -1 was used in previous versions to represent the default setting
+		 */
+		newtarget = -1;
+	}
 
 	/*
 	 * Limit target to a sane range
@@ -8630,7 +8643,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 
 	if (colName)
 	{
-		tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
+		tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8640,7 +8653,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 	}
 	else
 	{
-		tuple = SearchSysCacheCopyAttNum(RelationGetRelid(rel), colNum);
+		tuple = SearchSysCacheAttNum(RelationGetRelid(rel), colNum);
 
 		if (!HeapTupleIsValid(tuple))
 			ereport(ERROR,
@@ -8674,16 +8687,27 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 					 errhint("Alter statistics on table column instead.")));
 	}
 
-	attrtuple->attstattarget = newtarget;
-
-	CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
+	/* Build new tuple. */
+	memset(repl_null, false, sizeof(repl_null));
+	memset(repl_repl, false, sizeof(repl_repl));
+	if (newtarget != -1)
+		repl_val[Anum_pg_attribute_attstattarget - 1] = newtarget;
+	else
+		repl_null[Anum_pg_attribute_attstattarget - 1] = true;
+	repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
+	newtuple = heap_modify_tuple(tuple, RelationGetDescr(attrelation),
+								 repl_val, repl_null, repl_repl);
+	CatalogTupleUpdate(attrelation, &tuple->t_self, newtuple);
 
 	InvokeObjectPostAlterHook(RelationRelationId,
 							  RelationGetRelid(rel),
 							  attrtuple->attnum);
 	ObjectAddressSubSet(address, RelationRelationId,
 						RelationGetRelid(rel), attnum);
-	heap_freetuple(tuple);
+
+	heap_freetuple(newtuple);
+
+	ReleaseSysCache(tuple);
 
 	table_close(attrelation, RowExclusiveLock);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6b88096e8e1..3460fea56ba 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -337,6 +337,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <list>	alter_table_cmds alter_type_cmds
 %type <list>    alter_identity_column_option_list
 %type <defelt>  alter_identity_column_option
+%type <node>	set_statistics_value
 
 %type <list>	createdb_opt_list createdb_opt_items copy_opt_list
 				transaction_mode_list
@@ -2446,18 +2447,18 @@ alter_table_cmd:
 					n->missing_ok = true;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column ColId SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STATISTICS */
+			| ALTER opt_column ColId SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStatistics;
 					n->name = $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
-			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS <SignedIconst> */
-			| ALTER opt_column Iconst SET STATISTICS SignedIconst
+			/* ALTER TABLE <name> ALTER [COLUMN] <colnum> SET STATISTICS */
+			| ALTER opt_column Iconst SET STATISTICS set_statistics_value
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
@@ -2469,7 +2470,7 @@ alter_table_cmd:
 
 					n->subtype = AT_SetStatistics;
 					n->num = (int16) $3;
-					n->def = (Node *) makeInteger($6);
+					n->def = $6;
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET ( column_parameter = value [, ... ] ) */
@@ -3070,6 +3071,11 @@ alter_identity_column_option:
 				}
 		;
 
+set_statistics_value:
+			SignedIconst					{ $$ = (Node *) makeInteger($1); }
+			| DEFAULT						{ $$ = NULL; }
+		;
+
 PartitionBoundSpec:
 			/* a HASH partition */
 			FOR VALUES WITH '(' hash_partbound ')'
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 8ec83561bfa..f730aa26c47 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -872,33 +872,6 @@ get_attnum(Oid relid, const char *attname)
 		return InvalidAttrNumber;
 }
 
-/*
- * get_attstattarget
- *
- *		Given the relation id and the attribute number,
- *		return the "attstattarget" field from the attribute relation.
- *
- *		Errors if not found.
- */
-int
-get_attstattarget(Oid relid, AttrNumber attnum)
-{
-	HeapTuple	tp;
-	Form_pg_attribute att_tup;
-	int			result;
-
-	tp = SearchSysCache2(ATTNUM,
-						 ObjectIdGetDatum(relid),
-						 Int16GetDatum(attnum));
-	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-			 attnum, relid);
-	att_tup = (Form_pg_attribute) GETSTRUCT(tp);
-	result = att_tup->attstattarget;
-	ReleaseSysCache(tp);
-	return result;
-}
-
 /*
  * get_attgenerated
  *
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 22d1e6cf922..d4a888f5f13 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8925,7 +8925,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 						 tbinfo->dobj.name);
 			tbinfo->attnames[j] = pg_strdup(PQgetvalue(res, r, i_attname));
 			tbinfo->atttypnames[j] = pg_strdup(PQgetvalue(res, r, i_atttypname));
-			tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
+			if (PQgetisnull(res, r, i_attstattarget))
+				tbinfo->attstattarget[j] = -1;
+			else
+				tbinfo->attstattarget[j] = atoi(PQgetvalue(res, r, i_attstattarget));
 			tbinfo->attstorage[j] = *(PQgetvalue(res, r, i_attstorage));
 			tbinfo->typstorage[j] = *(PQgetvalue(res, r, i_typstorage));
 			tbinfo->attidentity[j] = *(PQgetvalue(res, r, i_attidentity));
@@ -16507,7 +16510,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 			/*
 			 * Dump per-column statistics information. We only issue an ALTER
 			 * TABLE statement if the attstattarget entry for this column is
-			 * non-negative (i.e. it's not the default value)
+			 * not the default value.
 			 */
 			if (tbinfo->attstattarget[j] >= 0)
 				appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET STATISTICS %d;\n",
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 7f4d308e8dd..1a487441e50 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -158,22 +158,22 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	/* Number of times inherited from direct parent relation(s) */
 	int16		attinhcount BKI_DEFAULT(0);
 
+	/* attribute's collation, if any */
+	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
+
+#ifdef CATALOG_VARLEN			/* variable-length/nullable fields start here */
+	/* NOTE: The following fields are not present in tuple descriptors. */
+
 	/*
 	 * attstattarget is the target number of statistics datapoints to collect
 	 * during VACUUM ANALYZE of this column.  A zero here indicates that we do
-	 * not wish to collect any stats about this column. A "-1" here indicates
+	 * not wish to collect any stats about this column. A NULL here indicates
 	 * that no value has been explicitly set for this column, so ANALYZE
 	 * should use the default setting.
 	 *
 	 * int16 is sufficient for the current max value (MAX_STATISTICS_TARGET).
 	 */
-	int16		attstattarget BKI_DEFAULT(-1);
-
-	/* attribute's collation, if any */
-	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
-
-#ifdef CATALOG_VARLEN			/* variable-length fields start here */
-	/* NOTE: The following fields are not present in tuple descriptors. */
+	int16		attstattarget BKI_DEFAULT(_null_) BKI_FORCE_NULL;
 
 	/* Column-level access permissions */
 	aclitem		attacl[1] BKI_DEFAULT(_null_);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5cacefc7670..7f623b37fdc 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -121,7 +121,7 @@ typedef struct VacAttrStats
 	 * than the underlying column/expression.  Therefore, use these fields for
 	 * information about the datatype being fed to the typanalyze function.
 	 */
-	int			attstattarget;
+	int			attstattarget;	/* -1 to use default */
 	Oid			attrtypid;		/* type of data being analyzed */
 	int32		attrtypmod;		/* typmod of data being analyzed */
 	Form_pg_type attrtype;		/* copy of pg_type row for attrtypid */
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index be9ed70e841..e4a200b00ec 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -90,7 +90,6 @@ extern Oid	get_opfamily_proc(Oid opfamily, Oid lefttype, Oid righttype,
 							  int16 procnum);
 extern char *get_attname(Oid relid, AttrNumber attnum, bool missing_ok);
 extern AttrNumber get_attnum(Oid relid, const char *attname);
-extern int	get_attstattarget(Oid relid, AttrNumber attnum);
 extern char get_attgenerated(Oid relid, AttrNumber attnum);
 extern Oid	get_atttype(Oid relid, AttrNumber attnum);
 extern void get_atttypetypmodcoll(Oid relid, AttrNumber attnum,
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 446cfa678b7..79fa117cb54 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2707,8 +2707,8 @@ SELECT attrelid::regclass, attnum, attstattarget
          attrelid          | attnum | attstattarget 
 ---------------------------+--------+---------------
  concur_exprs_index_expr   |      1 |           100
- concur_exprs_index_pred   |      1 |            -1
- concur_exprs_index_pred_2 |      1 |            -1
+ concur_exprs_index_pred   |      1 |              
+ concur_exprs_index_pred_2 |      1 |              
 (3 rows)
 
 DROP TABLE concur_exprs_tab;

base-commit: 6f97ef05d62a9c4ed5c53e98ac8a44cf3e0a2780
-- 
2.43.0

v3-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchtext/plain; charset=UTF-8; name=v3-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchDownload
From 0cb841fd3d6ded93a54f499b328b49e63ad1541f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 11 Jan 2024 10:26:04 +0100
Subject: [PATCH v3 2/3] Generalize handling of nullable pg_attribute columns
 in DDL

DDL code uses tuple descriptors to pass around pg_attribute values
during table and index creation.  But tuple descriptors don't include
the variable-length/nullable columns of pg_attribute, so they have to
be handled separately.  Right now, the attoptions field is handled in
a one-off way with a separate argument passed to
InsertPgAttributeTuples().  The other affected fields of pg_attribute
are right now not needed at relation creation time.

The goal of this patch is to generalize this to allow handling
additional variable-length/nullable columns of pg_attribute in a
similar manner.  For that, create a new struct
Form_pg_attribute_extra, which is to be passed around in parallel to
the tuple descriptor and optionally supplies the additional columns.
Right now, this struct only contains one field for attoptions, so no
functionality is actually changed by this.

Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         | 12 +++++++++---
 src/backend/catalog/index.c        | 16 +++++++++++++++-
 src/include/catalog/heap.h         |  2 +-
 src/include/catalog/pg_attribute.h | 15 +++++++++++++++
 src/tools/pgindent/typedefs.list   |  2 ++
 5 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 45a71081d42..e0fa12a0a96 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -697,7 +697,7 @@ void
 InsertPgAttributeTuples(Relation pg_attribute_rel,
 						TupleDesc tupdesc,
 						Oid new_rel_oid,
-						const Datum *attoptions,
+						const FormData_pg_attribute_extra tupdesc_extra[],
 						CatalogIndexState indstate)
 {
 	TupleTableSlot **slot;
@@ -719,6 +719,7 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 	while (natts < tupdesc->natts)
 	{
 		Form_pg_attribute attrs = TupleDescAttr(tupdesc, natts);
+		const FormData_pg_attribute_extra *attrs_extra = tupdesc_extra ? &tupdesc_extra[natts] : NULL;
 
 		ExecClearTuple(slot[slotCount]);
 
@@ -750,10 +751,15 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
-		if (attoptions && attoptions[natts] != (Datum) 0)
-			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
+		if (attrs_extra)
+		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
+		}
 		else
+		{
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
+		}
 
 		/*
 		 * The remaining fields are not set for new columns.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index fbef3d5382d..d14e42ad8bb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -517,6 +517,20 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
 	TupleDesc	indexTupDesc;
+	FormData_pg_attribute_extra *attrs_extra = NULL;
+
+	if (attopts)
+	{
+		attrs_extra = palloc0_array(FormData_pg_attribute_extra, indexRelation->rd_att->natts);
+
+		for (int i = 0; i < indexRelation->rd_att->natts; i++)
+		{
+			if (attopts[i])
+				attrs_extra[i].attoptions.value = attopts[i];
+			else
+				attrs_extra[i].attoptions.isnull = true;
+		}
+	}
 
 	/*
 	 * open the attribute relation and its indexes
@@ -530,7 +544,7 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	 */
 	indexTupDesc = RelationGetDescr(indexRelation);
 
-	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attopts, indstate);
+	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attrs_extra, indstate);
 
 	CatalogCloseIndexes(indstate);
 
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 1d7f8380d90..1e694a40919 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -98,7 +98,7 @@ extern List *heap_truncate_find_FKs(List *relationIds);
 extern void InsertPgAttributeTuples(Relation pg_attribute_rel,
 									TupleDesc tupdesc,
 									Oid new_rel_oid,
-									const Datum *attoptions,
+									const FormData_pg_attribute_extra tupdesc_extra[],
 									CatalogIndexState indstate);
 
 extern void InsertPgClassTuple(Relation pg_class_desc,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 1a487441e50..6258da3f683 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -208,6 +208,21 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
  */
 typedef FormData_pg_attribute *Form_pg_attribute;
 
+/*
+ * FormData_pg_attribute_extra contains (some of) the fields that are not in
+ * FormData_pg_attribute because they are excluded by CATALOG_VARLEN.  It is
+ * meant to be used by DDL code so that the combination of
+ * FormData_pg_attribute (often via tuple descriptor) and
+ * FormData_pg_attribute_extra can be used to pass around all the information
+ * about an attribute.  Fields can be included here as needed.
+ */
+typedef struct FormData_pg_attribute_extra
+{
+	NullableDatum attoptions;
+} FormData_pg_attribute_extra;
+
+typedef FormData_pg_attribute_extra *Form_pg_attribute_extra;
+
 DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, pg_attribute, btree(attrelid oid_ops, attname name_ops));
 DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, pg_attribute, btree(attrelid oid_ops, attnum int2_ops));
 
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fd46b7bd1f..e811341ce06 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -792,6 +792,7 @@ FormData_pg_amop
 FormData_pg_amproc
 FormData_pg_attrdef
 FormData_pg_attribute
+FormData_pg_attribute_extra
 FormData_pg_auth_members
 FormData_pg_authid
 FormData_pg_cast
@@ -850,6 +851,7 @@ Form_pg_amop
 Form_pg_amproc
 Form_pg_attrdef
 Form_pg_attribute
+Form_pg_attribute_extra
 Form_pg_auth_members
 Form_pg_authid
 Form_pg_cast
-- 
2.43.0

v3-0003-Add-attstattarget-to-Form_pg_attribute_extra.patchtext/plain; charset=UTF-8; name=v3-0003-Add-attstattarget-to-Form_pg_attribute_extra.patchDownload
From 5874bc06ecf6d3cbf5aa13a2559d3a78e7d79bc0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 11 Jan 2024 10:30:13 +0100
Subject: [PATCH v3 3/3] Add attstattarget to Form_pg_attribute_extra

This allows setting attstattarget when a relation is created.

We make use of this by having index_concurrently_create_copy() copy
over the attstattarget values when the new index is created, instead
of having index_concurrently_swap() fix it up later.

Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         |  5 +-
 src/backend/catalog/index.c        | 97 +++++++++---------------------
 src/backend/catalog/toasting.c     |  2 +-
 src/backend/commands/indexcmds.c   |  2 +-
 src/include/catalog/index.h        |  1 +
 src/include/catalog/pg_attribute.h |  1 +
 6 files changed, 36 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e0fa12a0a96..d632fc00faf 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -753,18 +753,21 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attrs_extra)
 		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.isnull;
+
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
 		}
 		else
 		{
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 		}
 
 		/*
 		 * The remaining fields are not set for new columns.
 		 */
-		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d14e42ad8bb..c0bfd731379 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -112,7 +112,7 @@ static TupleDesc ConstructTupleDescriptor(Relation heapRelation,
 										  const Oid *opclassIds);
 static void InitializeAttributeOids(Relation indexRelation,
 									int numatts, Oid indexoid);
-static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts);
+static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets);
 static void UpdateIndexRelation(Oid indexoid, Oid heapoid,
 								Oid parentIndexId,
 								const IndexInfo *indexInfo,
@@ -512,7 +512,7 @@ InitializeAttributeOids(Relation indexRelation,
  * ----------------------------------------------------------------
  */
 static void
-AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
+AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets)
 {
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
@@ -529,6 +529,11 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 				attrs_extra[i].attoptions.value = attopts[i];
 			else
 				attrs_extra[i].attoptions.isnull = true;
+
+			if (stattargets)
+				attrs_extra[i].attstattarget = stattargets[i];
+			else
+				attrs_extra[i].attstattarget.isnull = true;
 		}
 	}
 
@@ -735,6 +740,7 @@ index_create(Relation heapRelation,
 			 const Oid *opclassIds,
 			 const Datum *opclassOptions,
 			 const int16 *coloptions,
+			 const NullableDatum *stattargets,
 			 Datum reloptions,
 			 bits16 flags,
 			 bits16 constr_flags,
@@ -1029,7 +1035,7 @@ index_create(Relation heapRelation,
 	/*
 	 * append ATTRIBUTE tuples for the index
 	 */
-	AppendAttributeTuples(indexRelation, opclassOptions);
+	AppendAttributeTuples(indexRelation, opclassOptions, stattargets);
 
 	/* ----------------
 	 *	  update pg_index
@@ -1308,6 +1314,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	Datum	   *opclassOptions;
 	oidvector  *indclass;
 	int2vector *indcoloptions;
+	NullableDatum *stattargets;
 	bool		isnull;
 	List	   *indexColNames = NIL;
 	List	   *indexExprs = NIL;
@@ -1412,6 +1419,23 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
 		opclassOptions[i] = get_attoptions(oldIndexId, i + 1);
 
+	/* Extra statistic targets for each attribute */
+	stattargets = palloc0_array(NullableDatum, newInfo->ii_NumIndexAttrs);
+	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
+	{
+		HeapTuple   tp;
+		Datum       dat;
+
+		tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(i + 1));
+		if (!HeapTupleIsValid(tp))
+			elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+				 i + 1, oldIndexId);
+		dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+		ReleaseSysCache(tp);
+		stattargets[i].value = dat;
+		stattargets[i].isnull = isnull;
+	}
+
 	/*
 	 * Now create the new index.
 	 *
@@ -1433,6 +1457,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							  indclass->values,
 							  opclassOptions,
 							  indcoloptions->values,
+							  stattargets,
 							  reloptionsDatum,
 							  INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
 							  0,
@@ -1775,72 +1800,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	/* Copy data of pg_statistic from the old index to the new one */
 	CopyStatistics(oldIndexId, newIndexId);
 
-	/* Copy pg_attribute.attstattarget for each index attribute */
-	{
-		HeapTuple	attrTuple;
-		Relation	pg_attribute;
-		SysScanDesc scan;
-		ScanKeyData key[1];
-
-		pg_attribute = table_open(AttributeRelationId, RowExclusiveLock);
-		ScanKeyInit(&key[0],
-					Anum_pg_attribute_attrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(newIndexId));
-		scan = systable_beginscan(pg_attribute, AttributeRelidNumIndexId,
-								  true, NULL, 1, key);
-
-		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
-		{
-			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
-			HeapTuple	tp;
-			Datum		dat;
-			bool		isnull;
-			Datum		repl_val[Natts_pg_attribute];
-			bool		repl_null[Natts_pg_attribute];
-			bool		repl_repl[Natts_pg_attribute];
-			HeapTuple	newTuple;
-
-			/* Ignore dropped columns */
-			if (att->attisdropped)
-				continue;
-
-			/*
-			 * Get attstattarget from the old index and refresh the new value.
-			 */
-			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
-			if (!HeapTupleIsValid(tp))
-				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-					 att->attnum, oldIndexId);
-			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
-			ReleaseSysCache(tp);
-
-			/*
-			 * No need for a refresh if old index value is null.  (All new
-			 * index values are null at this point.)
-			 */
-			if (isnull)
-				continue;
-
-			memset(repl_val, 0, sizeof(repl_val));
-			memset(repl_null, false, sizeof(repl_null));
-			memset(repl_repl, false, sizeof(repl_repl));
-
-			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
-
-			newTuple = heap_modify_tuple(attrTuple,
-										 RelationGetDescr(pg_attribute),
-										 repl_val, repl_null, repl_repl);
-			CatalogTupleUpdate(pg_attribute, &newTuple->t_self, newTuple);
-
-			heap_freetuple(newTuple);
-		}
-
-		systable_endscan(scan);
-		table_close(pg_attribute, RowExclusiveLock);
-	}
-
 	/* Close relations */
 	table_close(pg_class, RowExclusiveLock);
 	table_close(pg_index, RowExclusiveLock);
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 05d945b34b7..bbdf74ebed3 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -326,7 +326,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 				 list_make2("chunk_id", "chunk_seq"),
 				 BTREE_AM_OID,
 				 rel->rd_rel->reltablespace,
-				 collationIds, opclassIds, NULL, coloptions, (Datum) 0,
+				 collationIds, opclassIds, NULL, coloptions, NULL, (Datum) 0,
 				 INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
 
 	table_close(toast_rel, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 340248a3f29..c2416c11d61 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1193,7 +1193,7 @@ DefineIndex(Oid tableId,
 					 stmt->oldNumber, indexInfo, indexColNames,
 					 accessMethodId, tablespaceId,
 					 collationIds, opclassIds, opclassOptions,
-					 coloptions, reloptions,
+					 coloptions, NULL, reloptions,
 					 flags, constr_flags,
 					 allowSystemTableMods, !check_rights,
 					 &createdConstraintId);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 99dab5940bc..7d434f8e653 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -80,6 +80,7 @@ extern Oid	index_create(Relation heapRelation,
 						 const Oid *opclassIds,
 						 const Datum *opclassOptions,
 						 const int16 *coloptions,
+						 const NullableDatum *stattargets,
 						 Datum reloptions,
 						 bits16 flags,
 						 bits16 constr_flags,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 6258da3f683..c26058f8ab7 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -218,6 +218,7 @@ typedef FormData_pg_attribute *Form_pg_attribute;
  */
 typedef struct FormData_pg_attribute_extra
 {
+	NullableDatum attstattarget;
 	NullableDatum attoptions;
 } FormData_pg_attribute_extra;
 
-- 
2.43.0

#5Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Peter Eisentraut (#4)
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/

#6Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Alvaro Herrera (#5)
Re: Make attstattarget nullable

BTW I wanted to but didn't say so explicitly, so here goes: 0001 looks
ready to go in.

--
Álvaro Herrera Breisgau, 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)

#7Peter Eisentraut
peter@eisentraut.org
In reply to: Alvaro Herrera (#5)
3 attachment(s)
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.

Attachments:

v4-0001-Make-stxstattarget-nullable.patchtext/plain; charset=UTF-8; name=v4-0001-Make-stxstattarget-nullable.patchDownload
From 9199be09efcbca1b906b5c41e8524e68b1a6b64e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
       </para></entry>
      </row>
 
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>stxkeys</structfield> <type>int2vector</type>
+       (references <link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
+      </para>
+      <para>
+       An array of attribute numbers, indicating which table columns are
+       covered by this statistics object;
+       for example a value of <literal>1 3</literal> would
+       mean that the first and the third table columns are covered
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>stxstattarget</structfield> <type>int2</type>
@@ -7642,7 +7655,7 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
        of statistics accumulated for this statistics object by
        <link linkend="sql-analyze"><command>ANALYZE</command></link>.
        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 <structfield>stxstattarget</structfield>
        determine the target number of <quote>most common values</quote>
@@ -7650,19 +7663,6 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
       </para></entry>
      </row>
 
-     <row>
-      <entry role="catalog_table_entry"><para role="column_definition">
-       <structfield>stxkeys</structfield> <type>int2vector</type>
-       (references <link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
-      </para>
-      <para>
-       An array of attribute numbers, indicating which table columns are
-       covered by this statistics object;
-       for example a value of <literal>1 3</literal> would
-       mean that the first and the third table columns are covered
-      </para></entry>
-     </row>
-
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>stxkind</structfield> <type>char[]</type>
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 <replaceable class="parameter">name</replaceable> OWNER TO { <replaceable class="parameter">new_owner</replaceable> | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
 ALTER STATISTICS <replaceable class="parameter">name</replaceable> RENAME TO <replaceable class="parameter">new_name</replaceable>
 ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET SCHEMA <replaceable class="parameter">new_schema</replaceable>
-ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTICS <replaceable class="parameter">new_target</replaceable>
+ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTICS { <replaceable class="parameter">integer</replaceable> | DEFAULT }
 </synopsis>
  </refsynopsisdiv>
 
@@ -96,15 +96,16 @@ <title>Parameters</title>
      </varlistentry>
 
      <varlistentry>
-      <term><replaceable class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable class="parameter">integer</replaceable> | DEFAULT }</literal></term>
       <listitem>
        <para>
         The statistic-gathering target for this statistics object for subsequent
         <link linkend="sql-analyze"><command>ANALYZE</command></link> operations.
-        The target can be set in the range 0 to 10000; alternatively, set it
-        to -1 to revert to using the maximum of the statistics target of the
-        referenced columns, if set, or the system default statistics
-        target (<xref linkend="guc-default-statistics-target"/>).
+        The target can be set in the range 0 to 10000.  Set it to
+        <literal>DEFAULT</literal> to revert to using the system default
+        statistics target (<xref linkend="guc-default-statistics-target"/>).
+        (Setting to a value of -1 is an obsolete way spelling to get the same
+        outcome.)
         For more information on the use of statistics by the
         <productname>PostgreSQL</productname> query planner, refer to
         <xref linkend="planner-stats"/>.
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index b1a9c74bd63..b2e509afef0 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -498,9 +498,9 @@ CreateStatistics(CreateStatsStmt *stmt)
 	values[Anum_pg_statistic_ext_stxrelid - 1] = ObjectIdGetDatum(relid);
 	values[Anum_pg_statistic_ext_stxname - 1] = NameGetDatum(&stxname);
 	values[Anum_pg_statistic_ext_stxnamespace - 1] = ObjectIdGetDatum(namespaceId);
-	values[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(-1);
 	values[Anum_pg_statistic_ext_stxowner - 1] = ObjectIdGetDatum(stxowner);
 	values[Anum_pg_statistic_ext_stxkeys - 1] = PointerGetDatum(stxkeys);
+	nulls[Anum_pg_statistic_ext_stxstattarget - 1] = true;
 	values[Anum_pg_statistic_ext_stxkind - 1] = PointerGetDatum(stxkind);
 
 	values[Anum_pg_statistic_ext_stxexprs - 1] = exprsDatum;
@@ -609,7 +609,19 @@ AlterStatistics(AlterStatsStmt *stmt)
 	bool		repl_null[Natts_pg_statistic_ext];
 	bool		repl_repl[Natts_pg_statistic_ext];
 	ObjectAddress address;
-	int			newtarget = stmt->stxstattarget;
+	int			newtarget;
+
+	if (stmt->stxstattarget)
+	{
+		newtarget = intVal(stmt->stxstattarget);
+	}
+	else
+	{
+		/*
+		 * -1 was used in previous versions to represent the default setting
+		 */
+		newtarget = -1;
+	}
 
 	/* Limit statistics target to a sane range */
 	if (newtarget < -1)
@@ -676,7 +688,10 @@ AlterStatistics(AlterStatsStmt *stmt)
 
 	/* replace the stxstattarget column */
 	repl_repl[Anum_pg_statistic_ext_stxstattarget - 1] = true;
-	repl_val[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(newtarget);
+	if (newtarget != -1)
+		repl_val[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(newtarget);
+	else
+		repl_null[Anum_pg_statistic_ext_stxstattarget - 1] = true;
 
 	newtup = heap_modify_tuple(oldtup, RelationGetDescr(rel),
 							   repl_val, repl_null, repl_repl);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3460fea56ba..e0e30ce1716 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4597,7 +4597,7 @@ stats_param:	ColId
  *****************************************************************************/
 
 AlterStatsStmt:
-			ALTER STATISTICS any_name SET STATISTICS SignedIconst
+			ALTER STATISTICS any_name SET STATISTICS set_statistics_value
 				{
 					AlterStatsStmt *n = makeNode(AlterStatsStmt);
 
@@ -4606,7 +4606,7 @@ AlterStatsStmt:
 					n->stxstattarget = $6;
 					$$ = (Node *) n;
 				}
-			| ALTER STATISTICS IF_P EXISTS any_name SET STATISTICS SignedIconst
+			| ALTER STATISTICS IF_P EXISTS any_name SET STATISTICS set_statistics_value
 				{
 					AlterStatsStmt *n = makeNode(AlterStatsStmt);
 
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index c5461514d8f..33f1ad98d49 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -457,13 +457,15 @@ fetch_statentries_for_relation(Relation pg_statext, Oid relid)
 		entry->statOid = staForm->oid;
 		entry->schema = get_namespace_name(staForm->stxnamespace);
 		entry->name = pstrdup(NameStr(staForm->stxname));
-		entry->stattarget = staForm->stxstattarget;
 		for (i = 0; i < staForm->stxkeys.dim1; i++)
 		{
 			entry->columns = bms_add_member(entry->columns,
 											staForm->stxkeys.values[i]);
 		}
 
+		datum = SysCacheGetAttr(STATEXTOID, htup, Anum_pg_statistic_ext_stxstattarget, &isnull);
+		entry->stattarget = isnull ? -1 : DatumGetInt16(datum);
+
 		/* decode the stxkind char array into a list of chars */
 		datum = SysCacheGetAttrNotNull(STATEXTOID, htup,
 									   Anum_pg_statistic_ext_stxkind);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bc20a025ce4..b151eac6654 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7542,7 +7542,7 @@ getExtendedStatistics(Archive *fout)
 
 	if (fout->remoteVersion < 130000)
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
-							 "stxnamespace, stxowner, stxrelid, (-1) AS stxstattarget "
+							 "stxnamespace, stxowner, stxrelid, NULL AS stxstattarget "
 							 "FROM pg_catalog.pg_statistic_ext");
 	else
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
@@ -7575,7 +7575,10 @@ getExtendedStatistics(Archive *fout)
 		statsextinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_stxowner));
 		statsextinfo[i].stattable =
 			findTableByOid(atooid(PQgetvalue(res, i, i_stxrelid)));
-		statsextinfo[i].stattarget = atoi(PQgetvalue(res, i, i_stattarget));
+		if (PQgetisnull(res, i, i_stattarget))
+			statsextinfo[i].stattarget = -1;
+		else
+			statsextinfo[i].stattarget = atoi(PQgetvalue(res, i, i_stattarget));
 
 		/* Decide whether we want to dump it */
 		selectDumpableStatisticsObject(&(statsextinfo[i]), fout);
@@ -17010,8 +17013,7 @@ dumpStatisticsExt(Archive *fout, const StatsExtInfo *statsextinfo)
 
 	/*
 	 * We only issue an ALTER STATISTICS statement if the stxstattarget entry
-	 * for this statistics object is non-negative (i.e. it's not the default
-	 * value).
+	 * for this statistics object is not the default value.
 	 */
 	if (statsextinfo->stattarget >= 0)
 	{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 37f95163201..7be1ffba1ee 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2792,7 +2792,8 @@ describeOneTableDetails(const char *schemaname,
 									  PQgetvalue(result, i, 1));
 
 					/* Show the stats target if it's not default */
-					if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)
+					if (!PQgetisnull(result, i, 8) &&
+						strcmp(PQgetvalue(result, i, 8), "-1") != 0)
 						appendPQExpBuffer(&buf, "; STATISTICS %s",
 										  PQgetvalue(result, i, 8));
 
diff --git a/src/include/catalog/pg_statistic_ext.h b/src/include/catalog/pg_statistic_ext.h
index 104b7db1f95..1ef58b33e3e 100644
--- a/src/include/catalog/pg_statistic_ext.h
+++ b/src/include/catalog/pg_statistic_ext.h
@@ -43,15 +43,15 @@ CATALOG(pg_statistic_ext,3381,StatisticExtRelationId)
 														 * object's namespace */
 
 	Oid			stxowner BKI_LOOKUP(pg_authid); /* statistics object's owner */
-	int16		stxstattarget BKI_DEFAULT(-1);	/* statistics target */
 
 	/*
-	 * variable-length fields start here, but we allow direct access to
-	 * stxkeys
+	 * variable-length/nullable fields start here, but we allow direct access
+	 * to stxkeys
 	 */
 	int2vector	stxkeys BKI_FORCE_NOT_NULL; /* array of column keys */
 
 #ifdef CATALOG_VARLEN
+	int16		stxstattarget BKI_DEFAULT(_null_) BKI_FORCE_NULL;	/* statistics target */
 	char		stxkind[1] BKI_FORCE_NOT_NULL;	/* statistics kinds requested
 												 * to build */
 	pg_node_tree stxexprs;		/* A list of expression trees for stats
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b3181f34aee..59ea2f0a9f0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3267,7 +3267,7 @@ typedef struct AlterStatsStmt
 {
 	NodeTag		type;
 	List	   *defnames;		/* qualified name (list of String) */
-	int			stxstattarget;	/* statistics target */
+	Node	   *stxstattarget;	/* statistics target */
 	bool		missing_ok;		/* skip error if statistics object is missing */
 } AlterStatsStmt;
 

base-commit: 31acee4b66f9f88ad5c19c1276252688bdaa95c9
-- 
2.43.0

v4-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchtext/plain; charset=UTF-8; name=v4-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchDownload
From 552e5cc5d5e78134dcfcea242037f6ef36b4c36a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 16:44:55 +0100
Subject: [PATCH v4 2/3] Generalize handling of nullable pg_attribute columns
 in DDL

DDL code uses tuple descriptors to pass around pg_attribute values
during table and index creation.  But tuple descriptors don't include
the variable-length/nullable columns of pg_attribute, so they have to
be handled separately.  Right now, the attoptions field is handled in
a one-off way with a separate argument passed to
InsertPgAttributeTuples().  The other affected fields of pg_attribute
are right now not needed at relation creation time.

The goal of this patch is to generalize this to allow handling
additional variable-length/nullable columns of pg_attribute in a
similar manner.  For that, create a new struct
FormExtraData_pg_attribute, which is to be passed around in parallel
to the tuple descriptor and optionally supplies the additional
columns.  Right now, this struct only contains one field for
attoptions, so no functionality is actually changed by this.

Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         | 12 +++++++++---
 src/backend/catalog/index.c        | 16 +++++++++++++++-
 src/include/catalog/heap.h         |  2 +-
 src/include/catalog/pg_attribute.h | 13 +++++++++++++
 src/tools/pgindent/typedefs.list   |  1 +
 5 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 45a71081d42..70e14d09263 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -697,7 +697,7 @@ void
 InsertPgAttributeTuples(Relation pg_attribute_rel,
 						TupleDesc tupdesc,
 						Oid new_rel_oid,
-						const Datum *attoptions,
+						const FormExtraData_pg_attribute tupdesc_extra[],
 						CatalogIndexState indstate)
 {
 	TupleTableSlot **slot;
@@ -719,6 +719,7 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 	while (natts < tupdesc->natts)
 	{
 		Form_pg_attribute attrs = TupleDescAttr(tupdesc, natts);
+		const FormExtraData_pg_attribute *attrs_extra = tupdesc_extra ? &tupdesc_extra[natts] : NULL;
 
 		ExecClearTuple(slot[slotCount]);
 
@@ -750,10 +751,15 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
-		if (attoptions && attoptions[natts] != (Datum) 0)
-			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
+		if (attrs_extra)
+		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
+		}
 		else
+		{
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
+		}
 
 		/*
 		 * The remaining fields are not set for new columns.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index fbef3d5382d..f899d15876f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -517,6 +517,20 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
 	TupleDesc	indexTupDesc;
+	FormExtraData_pg_attribute *attrs_extra = NULL;
+
+	if (attopts)
+	{
+		attrs_extra = palloc0_array(FormExtraData_pg_attribute, indexRelation->rd_att->natts);
+
+		for (int i = 0; i < indexRelation->rd_att->natts; i++)
+		{
+			if (attopts[i])
+				attrs_extra[i].attoptions.value = attopts[i];
+			else
+				attrs_extra[i].attoptions.isnull = true;
+		}
+	}
 
 	/*
 	 * open the attribute relation and its indexes
@@ -530,7 +544,7 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	 */
 	indexTupDesc = RelationGetDescr(indexRelation);
 
-	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attopts, indstate);
+	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attrs_extra, indstate);
 
 	CatalogCloseIndexes(indstate);
 
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 1d7f8380d90..21e31f9c974 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -98,7 +98,7 @@ extern List *heap_truncate_find_FKs(List *relationIds);
 extern void InsertPgAttributeTuples(Relation pg_attribute_rel,
 									TupleDesc tupdesc,
 									Oid new_rel_oid,
-									const Datum *attoptions,
+									const FormExtraData_pg_attribute tupdesc_extra[],
 									CatalogIndexState indstate);
 
 extern void InsertPgClassTuple(Relation pg_class_desc,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index e2aadb94141..27b0077e25b 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -208,6 +208,19 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
  */
 typedef FormData_pg_attribute *Form_pg_attribute;
 
+/*
+ * FormExtraData_pg_attribute contains (some of) the fields that are not in
+ * FormData_pg_attribute because they are excluded by CATALOG_VARLEN.  It is
+ * meant to be used by DDL code so that the combination of
+ * FormData_pg_attribute (often via tuple descriptor) and
+ * FormExtraData_pg_attribute can be used to pass around all the information
+ * about an attribute.  Fields can be included here as needed.
+ */
+typedef struct FormExtraData_pg_attribute
+{
+	NullableDatum attoptions;
+} FormExtraData_pg_attribute;
+
 DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, pg_attribute, btree(attrelid oid_ops, attname name_ops));
 DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, pg_attribute, btree(attrelid oid_ops, attnum int2_ops));
 
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index f582eb59e7d..d6d065375e9 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -844,6 +844,7 @@ FormData_pg_ts_parser
 FormData_pg_ts_template
 FormData_pg_type
 FormData_pg_user_mapping
+FormExtraData_pg_attribute
 Form_pg_aggregate
 Form_pg_am
 Form_pg_amop
-- 
2.43.0

v4-0003-Add-attstattarget-to-FormExtraData_pg_attribute.patchtext/plain; charset=UTF-8; name=v4-0003-Add-attstattarget-to-FormExtraData_pg_attribute.patchDownload
From e12fa90ddc05f5ffc21f3240d3cb22beaa6969ac Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 16:44:56 +0100
Subject: [PATCH v4 3/3] Add attstattarget to FormExtraData_pg_attribute

This allows setting attstattarget when a relation is created.

We make use of this by having index_concurrently_create_copy() copy
over the attstattarget values when the new index is created, instead
of having index_concurrently_swap() fix it up later.

Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         |  5 +-
 src/backend/catalog/index.c        | 97 +++++++++---------------------
 src/backend/catalog/toasting.c     |  2 +-
 src/backend/commands/indexcmds.c   |  2 +-
 src/include/catalog/index.h        |  1 +
 src/include/catalog/pg_attribute.h |  1 +
 6 files changed, 36 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 70e14d09263..182db28a54d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -753,18 +753,21 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attrs_extra)
 		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.isnull;
+
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
 		}
 		else
 		{
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 		}
 
 		/*
 		 * The remaining fields are not set for new columns.
 		 */
-		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f899d15876f..91ce3a21b29 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -112,7 +112,7 @@ static TupleDesc ConstructTupleDescriptor(Relation heapRelation,
 										  const Oid *opclassIds);
 static void InitializeAttributeOids(Relation indexRelation,
 									int numatts, Oid indexoid);
-static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts);
+static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets);
 static void UpdateIndexRelation(Oid indexoid, Oid heapoid,
 								Oid parentIndexId,
 								const IndexInfo *indexInfo,
@@ -512,7 +512,7 @@ InitializeAttributeOids(Relation indexRelation,
  * ----------------------------------------------------------------
  */
 static void
-AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
+AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets)
 {
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
@@ -529,6 +529,11 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 				attrs_extra[i].attoptions.value = attopts[i];
 			else
 				attrs_extra[i].attoptions.isnull = true;
+
+			if (stattargets)
+				attrs_extra[i].attstattarget = stattargets[i];
+			else
+				attrs_extra[i].attstattarget.isnull = true;
 		}
 	}
 
@@ -735,6 +740,7 @@ index_create(Relation heapRelation,
 			 const Oid *opclassIds,
 			 const Datum *opclassOptions,
 			 const int16 *coloptions,
+			 const NullableDatum *stattargets,
 			 Datum reloptions,
 			 bits16 flags,
 			 bits16 constr_flags,
@@ -1029,7 +1035,7 @@ index_create(Relation heapRelation,
 	/*
 	 * append ATTRIBUTE tuples for the index
 	 */
-	AppendAttributeTuples(indexRelation, opclassOptions);
+	AppendAttributeTuples(indexRelation, opclassOptions, stattargets);
 
 	/* ----------------
 	 *	  update pg_index
@@ -1308,6 +1314,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	Datum	   *opclassOptions;
 	oidvector  *indclass;
 	int2vector *indcoloptions;
+	NullableDatum *stattargets;
 	bool		isnull;
 	List	   *indexColNames = NIL;
 	List	   *indexExprs = NIL;
@@ -1412,6 +1419,23 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
 		opclassOptions[i] = get_attoptions(oldIndexId, i + 1);
 
+	/* Extra statistic targets for each attribute */
+	stattargets = palloc0_array(NullableDatum, newInfo->ii_NumIndexAttrs);
+	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
+	{
+		HeapTuple   tp;
+		Datum       dat;
+
+		tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(i + 1));
+		if (!HeapTupleIsValid(tp))
+			elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+				 i + 1, oldIndexId);
+		dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+		ReleaseSysCache(tp);
+		stattargets[i].value = dat;
+		stattargets[i].isnull = isnull;
+	}
+
 	/*
 	 * Now create the new index.
 	 *
@@ -1433,6 +1457,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							  indclass->values,
 							  opclassOptions,
 							  indcoloptions->values,
+							  stattargets,
 							  reloptionsDatum,
 							  INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
 							  0,
@@ -1775,72 +1800,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	/* Copy data of pg_statistic from the old index to the new one */
 	CopyStatistics(oldIndexId, newIndexId);
 
-	/* Copy pg_attribute.attstattarget for each index attribute */
-	{
-		HeapTuple	attrTuple;
-		Relation	pg_attribute;
-		SysScanDesc scan;
-		ScanKeyData key[1];
-
-		pg_attribute = table_open(AttributeRelationId, RowExclusiveLock);
-		ScanKeyInit(&key[0],
-					Anum_pg_attribute_attrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(newIndexId));
-		scan = systable_beginscan(pg_attribute, AttributeRelidNumIndexId,
-								  true, NULL, 1, key);
-
-		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
-		{
-			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
-			HeapTuple	tp;
-			Datum		dat;
-			bool		isnull;
-			Datum		repl_val[Natts_pg_attribute];
-			bool		repl_null[Natts_pg_attribute];
-			bool		repl_repl[Natts_pg_attribute];
-			HeapTuple	newTuple;
-
-			/* Ignore dropped columns */
-			if (att->attisdropped)
-				continue;
-
-			/*
-			 * Get attstattarget from the old index and refresh the new value.
-			 */
-			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
-			if (!HeapTupleIsValid(tp))
-				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-					 att->attnum, oldIndexId);
-			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
-			ReleaseSysCache(tp);
-
-			/*
-			 * No need for a refresh if old index value is null.  (All new
-			 * index values are null at this point.)
-			 */
-			if (isnull)
-				continue;
-
-			memset(repl_val, 0, sizeof(repl_val));
-			memset(repl_null, false, sizeof(repl_null));
-			memset(repl_repl, false, sizeof(repl_repl));
-
-			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
-
-			newTuple = heap_modify_tuple(attrTuple,
-										 RelationGetDescr(pg_attribute),
-										 repl_val, repl_null, repl_repl);
-			CatalogTupleUpdate(pg_attribute, &newTuple->t_self, newTuple);
-
-			heap_freetuple(newTuple);
-		}
-
-		systable_endscan(scan);
-		table_close(pg_attribute, RowExclusiveLock);
-	}
-
 	/* Close relations */
 	table_close(pg_class, RowExclusiveLock);
 	table_close(pg_index, RowExclusiveLock);
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 05d945b34b7..bbdf74ebed3 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -326,7 +326,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 				 list_make2("chunk_id", "chunk_seq"),
 				 BTREE_AM_OID,
 				 rel->rd_rel->reltablespace,
-				 collationIds, opclassIds, NULL, coloptions, (Datum) 0,
+				 collationIds, opclassIds, NULL, coloptions, NULL, (Datum) 0,
 				 INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
 
 	table_close(toast_rel, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 340248a3f29..c2416c11d61 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1193,7 +1193,7 @@ DefineIndex(Oid tableId,
 					 stmt->oldNumber, indexInfo, indexColNames,
 					 accessMethodId, tablespaceId,
 					 collationIds, opclassIds, opclassOptions,
-					 coloptions, reloptions,
+					 coloptions, NULL, reloptions,
 					 flags, constr_flags,
 					 allowSystemTableMods, !check_rights,
 					 &createdConstraintId);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 99dab5940bc..7d434f8e653 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -80,6 +80,7 @@ extern Oid	index_create(Relation heapRelation,
 						 const Oid *opclassIds,
 						 const Datum *opclassOptions,
 						 const int16 *coloptions,
+						 const NullableDatum *stattargets,
 						 Datum reloptions,
 						 bits16 flags,
 						 bits16 constr_flags,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 27b0077e25b..b28725f4dbe 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -218,6 +218,7 @@ typedef FormData_pg_attribute *Form_pg_attribute;
  */
 typedef struct FormExtraData_pg_attribute
 {
+	NullableDatum attstattarget;
 	NullableDatum attoptions;
 } FormExtraData_pg_attribute;
 
-- 
2.43.0

#8Tomas Vondra
tomas.vondra@enterprisedb.com
In reply to: Peter Eisentraut (#7)
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:

-      <term><replaceable class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable
class="parameter">integer</replaceable> | DEFAULT }</literal></term>

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

#9Peter Eisentraut
peter@eisentraut.org
In reply to: Tomas Vondra (#8)
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:

-      <term><replaceable class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable
class="parameter">integer</replaceable> | DEFAULT }</literal></term>

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.

#10Tomas Vondra
tomas.vondra@enterprisedb.com
In reply to: Peter Eisentraut (#9)
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:

-      <term><replaceable
class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable
class="parameter">integer</replaceable> | DEFAULT }</literal></term>

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

#11Peter Eisentraut
peter@eisentraut.org
In reply to: Tomas Vondra (#10)
3 attachment(s)
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:

-      <term><replaceable
class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable
class="parameter">integer</replaceable> | DEFAULT }</literal></term>

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.

Attachments:

v5-0001-Make-stxstattarget-nullable.patchtext/plain; charset=UTF-8; name=v5-0001-Make-stxstattarget-nullable.patchDownload
From d98742439bfe82a20cffcdda6d5a05fdd06b46ea Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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 <tomas.vondra@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@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 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
       </para></entry>
      </row>
 
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>stxkeys</structfield> <type>int2vector</type>
+       (references <link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
+      </para>
+      <para>
+       An array of attribute numbers, indicating which table columns are
+       covered by this statistics object;
+       for example a value of <literal>1 3</literal> would
+       mean that the first and the third table columns are covered
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>stxstattarget</structfield> <type>int2</type>
@@ -7666,7 +7679,7 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
        of statistics accumulated for this statistics object by
        <link linkend="sql-analyze"><command>ANALYZE</command></link>.
        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 <structfield>stxstattarget</structfield>
        determine the target number of <quote>most common values</quote>
@@ -7674,19 +7687,6 @@ <title><structname>pg_statistic_ext</structname> Columns</title>
       </para></entry>
      </row>
 
-     <row>
-      <entry role="catalog_table_entry"><para role="column_definition">
-       <structfield>stxkeys</structfield> <type>int2vector</type>
-       (references <link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
-      </para>
-      <para>
-       An array of attribute numbers, indicating which table columns are
-       covered by this statistics object;
-       for example a value of <literal>1 3</literal> would
-       mean that the first and the third table columns are covered
-      </para></entry>
-     </row>
-
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>stxkind</structfield> <type>char[]</type>
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 <replaceable class="parameter">name</replaceable> OWNER TO { <replaceable class="parameter">new_owner</replaceable> | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
 ALTER STATISTICS <replaceable class="parameter">name</replaceable> RENAME TO <replaceable class="parameter">new_name</replaceable>
 ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET SCHEMA <replaceable class="parameter">new_schema</replaceable>
-ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTICS <replaceable class="parameter">new_target</replaceable>
+ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTICS { <replaceable class="parameter">new_target</replaceable> | DEFAULT }
 </synopsis>
  </refsynopsisdiv>
 
@@ -101,10 +101,11 @@ <title>Parameters</title>
        <para>
         The statistic-gathering target for this statistics object for subsequent
         <link linkend="sql-analyze"><command>ANALYZE</command></link> operations.
-        The target can be set in the range 0 to 10000; alternatively, set it
-        to -1 to revert to using the maximum of the statistics target of the
-        referenced columns, if set, or the system default statistics
-        target (<xref linkend="guc-default-statistics-target"/>).
+        The target can be set in the range 0 to 10000.  Set it to
+        <literal>DEFAULT</literal> to revert to using the system default
+        statistics target (<xref linkend="guc-default-statistics-target"/>).
+        (Setting to a value of -1 is an obsolete way spelling to get the same
+        outcome.)
         For more information on the use of statistics by the
         <productname>PostgreSQL</productname> query planner, refer to
         <xref linkend="planner-stats"/>.
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index a855f750c75..5f49479832d 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -495,9 +495,9 @@ CreateStatistics(CreateStatsStmt *stmt)
 	values[Anum_pg_statistic_ext_stxrelid - 1] = ObjectIdGetDatum(relid);
 	values[Anum_pg_statistic_ext_stxname - 1] = NameGetDatum(&stxname);
 	values[Anum_pg_statistic_ext_stxnamespace - 1] = ObjectIdGetDatum(namespaceId);
-	values[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(-1);
 	values[Anum_pg_statistic_ext_stxowner - 1] = ObjectIdGetDatum(stxowner);
 	values[Anum_pg_statistic_ext_stxkeys - 1] = PointerGetDatum(stxkeys);
+	nulls[Anum_pg_statistic_ext_stxstattarget - 1] = true;
 	values[Anum_pg_statistic_ext_stxkind - 1] = PointerGetDatum(stxkind);
 
 	values[Anum_pg_statistic_ext_stxexprs - 1] = exprsDatum;
@@ -606,23 +606,36 @@ AlterStatistics(AlterStatsStmt *stmt)
 	bool		repl_null[Natts_pg_statistic_ext];
 	bool		repl_repl[Natts_pg_statistic_ext];
 	ObjectAddress address;
-	int			newtarget = stmt->stxstattarget;
+	int			newtarget;
+	bool		newtarget_default;
 
-	/* Limit statistics target to a sane range */
-	if (newtarget < -1)
+	/* -1 was used in previous versions for the default setting */
+	if (stmt->stxstattarget && intVal(stmt->stxstattarget) != -1)
 	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("statistics target %d is too low",
-						newtarget)));
+		newtarget = intVal(stmt->stxstattarget);
+		newtarget_default = false;
 	}
-	else if (newtarget > MAX_STATISTICS_TARGET)
+	else
+		newtarget_default = true;
+
+	if (!newtarget_default)
 	{
-		newtarget = MAX_STATISTICS_TARGET;
-		ereport(WARNING,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("lowering statistics target to %d",
-						newtarget)));
+		/* Limit statistics target to a sane range */
+		if (newtarget < 0)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("statistics target %d is too low",
+							newtarget)));
+		}
+		else if (newtarget > MAX_STATISTICS_TARGET)
+		{
+			newtarget = MAX_STATISTICS_TARGET;
+			ereport(WARNING,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("lowering statistics target to %d",
+							newtarget)));
+		}
 	}
 
 	/* lookup OID of the statistics object */
@@ -673,7 +686,10 @@ AlterStatistics(AlterStatsStmt *stmt)
 
 	/* replace the stxstattarget column */
 	repl_repl[Anum_pg_statistic_ext_stxstattarget - 1] = true;
-	repl_val[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(newtarget);
+	if (!newtarget_default)
+		repl_val[Anum_pg_statistic_ext_stxstattarget - 1] = Int16GetDatum(newtarget);
+	else
+		repl_null[Anum_pg_statistic_ext_stxstattarget - 1] = true;
 
 	newtup = heap_modify_tuple(oldtup, RelationGetDescr(rel),
 							   repl_val, repl_null, repl_repl);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2470265561a..ae6719329e7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8712,6 +8712,7 @@ static ObjectAddress
 ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newValue, LOCKMODE lockmode)
 {
 	int			newtarget;
+	bool		newtarget_default;
 	Relation	attrelation;
 	HeapTuple	tuple,
 				newtuple;
@@ -8733,35 +8734,35 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot refer to non-index column by number")));
 
-	if (newValue)
+	/* -1 was used in previous versions for the default setting */
+	if (newValue && intVal(newValue) != -1)
 	{
 		newtarget = intVal(newValue);
+		newtarget_default = false;
 	}
 	else
+		newtarget_default = true;
+
+	if (!newtarget_default)
 	{
 		/*
-		 * -1 was used in previous versions to represent the default setting
+		 * Limit target to a sane range
 		 */
-		newtarget = -1;
-	}
-
-	/*
-	 * Limit target to a sane range
-	 */
-	if (newtarget < -1)
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("statistics target %d is too low",
-						newtarget)));
-	}
-	else if (newtarget > MAX_STATISTICS_TARGET)
-	{
-		newtarget = MAX_STATISTICS_TARGET;
-		ereport(WARNING,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("lowering statistics target to %d",
-						newtarget)));
+		if (newtarget < 0)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("statistics target %d is too low",
+							newtarget)));
+		}
+		else if (newtarget > MAX_STATISTICS_TARGET)
+		{
+			newtarget = MAX_STATISTICS_TARGET;
+			ereport(WARNING,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("lowering statistics target to %d",
+							newtarget)));
+		}
 	}
 
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
@@ -8815,7 +8816,7 @@ ATExecSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 	/* Build new tuple. */
 	memset(repl_null, false, sizeof(repl_null));
 	memset(repl_repl, false, sizeof(repl_repl));
-	if (newtarget != -1)
+	if (!newtarget_default)
 		repl_val[Anum_pg_attribute_attstattarget - 1] = newtarget;
 	else
 		repl_null[Anum_pg_attribute_attstattarget - 1] = true;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c6e2f679fd5..3ad99fffe1d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4610,7 +4610,7 @@ stats_param:	ColId
  *****************************************************************************/
 
 AlterStatsStmt:
-			ALTER STATISTICS any_name SET STATISTICS SignedIconst
+			ALTER STATISTICS any_name SET STATISTICS set_statistics_value
 				{
 					AlterStatsStmt *n = makeNode(AlterStatsStmt);
 
@@ -4619,7 +4619,7 @@ AlterStatsStmt:
 					n->stxstattarget = $6;
 					$$ = (Node *) n;
 				}
-			| ALTER STATISTICS IF_P EXISTS any_name SET STATISTICS SignedIconst
+			| ALTER STATISTICS IF_P EXISTS any_name SET STATISTICS set_statistics_value
 				{
 					AlterStatsStmt *n = makeNode(AlterStatsStmt);
 
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index f21bdc2ab9a..99fdf208dba 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -454,13 +454,15 @@ fetch_statentries_for_relation(Relation pg_statext, Oid relid)
 		entry->statOid = staForm->oid;
 		entry->schema = get_namespace_name(staForm->stxnamespace);
 		entry->name = pstrdup(NameStr(staForm->stxname));
-		entry->stattarget = staForm->stxstattarget;
 		for (i = 0; i < staForm->stxkeys.dim1; i++)
 		{
 			entry->columns = bms_add_member(entry->columns,
 											staForm->stxkeys.values[i]);
 		}
 
+		datum = SysCacheGetAttr(STATEXTOID, htup, Anum_pg_statistic_ext_stxstattarget, &isnull);
+		entry->stattarget = isnull ? -1 : DatumGetInt16(datum);
+
 		/* decode the stxkind char array into a list of chars */
 		datum = SysCacheGetAttrNotNull(STATEXTOID, htup,
 									   Anum_pg_statistic_ext_stxkind);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 171e5916965..a5149ca823c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7580,7 +7580,7 @@ getExtendedStatistics(Archive *fout)
 
 	if (fout->remoteVersion < 130000)
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
-							 "stxnamespace, stxowner, stxrelid, (-1) AS stxstattarget "
+							 "stxnamespace, stxowner, stxrelid, NULL AS stxstattarget "
 							 "FROM pg_catalog.pg_statistic_ext");
 	else
 		appendPQExpBufferStr(query, "SELECT tableoid, oid, stxname, "
@@ -7613,7 +7613,10 @@ getExtendedStatistics(Archive *fout)
 		statsextinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_stxowner));
 		statsextinfo[i].stattable =
 			findTableByOid(atooid(PQgetvalue(res, i, i_stxrelid)));
-		statsextinfo[i].stattarget = atoi(PQgetvalue(res, i, i_stattarget));
+		if (PQgetisnull(res, i, i_stattarget))
+			statsextinfo[i].stattarget = -1;
+		else
+			statsextinfo[i].stattarget = atoi(PQgetvalue(res, i, i_stattarget));
 
 		/* Decide whether we want to dump it */
 		selectDumpableStatisticsObject(&(statsextinfo[i]), fout);
@@ -17062,8 +17065,7 @@ dumpStatisticsExt(Archive *fout, const StatsExtInfo *statsextinfo)
 
 	/*
 	 * We only issue an ALTER STATISTICS statement if the stxstattarget entry
-	 * for this statistics object is non-negative (i.e. it's not the default
-	 * value).
+	 * for this statistics object is not the default value.
 	 */
 	if (statsextinfo->stattarget >= 0)
 	{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 1ab80eb7cac..6433497bcd2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2804,7 +2804,8 @@ describeOneTableDetails(const char *schemaname,
 									  PQgetvalue(result, i, 1));
 
 					/* Show the stats target if it's not default */
-					if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)
+					if (!PQgetisnull(result, i, 8) &&
+						strcmp(PQgetvalue(result, i, 8), "-1") != 0)
 						appendPQExpBuffer(&buf, "; STATISTICS %s",
 										  PQgetvalue(result, i, 8));
 
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 429989efd91..9cf6dae3d90 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202403132
+#define CATALOG_VERSION_NO	202403141
 
 #endif
diff --git a/src/include/catalog/pg_statistic_ext.h b/src/include/catalog/pg_statistic_ext.h
index 85064086ec5..50a5c021edf 100644
--- a/src/include/catalog/pg_statistic_ext.h
+++ b/src/include/catalog/pg_statistic_ext.h
@@ -43,15 +43,15 @@ CATALOG(pg_statistic_ext,3381,StatisticExtRelationId)
 														 * object's namespace */
 
 	Oid			stxowner BKI_LOOKUP(pg_authid); /* statistics object's owner */
-	int16		stxstattarget BKI_DEFAULT(-1);	/* statistics target */
 
 	/*
-	 * variable-length fields start here, but we allow direct access to
-	 * stxkeys
+	 * variable-length/nullable fields start here, but we allow direct access
+	 * to stxkeys
 	 */
 	int2vector	stxkeys BKI_FORCE_NOT_NULL; /* array of column keys */
 
 #ifdef CATALOG_VARLEN
+	int16		stxstattarget BKI_DEFAULT(_null_) BKI_FORCE_NULL;	/* statistics target */
 	char		stxkind[1] BKI_FORCE_NOT_NULL;	/* statistics kinds requested
 												 * to build */
 	pg_node_tree stxexprs;		/* A list of expression trees for stats
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index aadaf67f574..70a21df0fee 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3269,7 +3269,7 @@ typedef struct AlterStatsStmt
 {
 	NodeTag		type;
 	List	   *defnames;		/* qualified name (list of String) */
-	int			stxstattarget;	/* statistics target */
+	Node	   *stxstattarget;	/* statistics target */
 	bool		missing_ok;		/* skip error if statistics object is missing */
 } AlterStatsStmt;
 

base-commit: 6b41ef03306f50602f68593d562cd73d5e39a9b9
-- 
2.44.0

v5-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchtext/plain; charset=UTF-8; name=v5-0002-Generalize-handling-of-nullable-pg_attribute-colu.patchDownload
From 03338d740f2f7f9521e020fc4fe24a2bfaf3b79d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 16:44:55 +0100
Subject: [PATCH v5 2/3] Generalize handling of nullable pg_attribute columns
 in DDL

DDL code uses tuple descriptors to pass around pg_attribute values
during table and index creation.  But tuple descriptors don't include
the variable-length/nullable columns of pg_attribute, so they have to
be handled separately.  Right now, the attoptions field is handled in
a one-off way with a separate argument passed to
InsertPgAttributeTuples().  The other affected fields of pg_attribute
are right now not needed at relation creation time.

The goal of this patch is to generalize this to allow handling
additional variable-length/nullable columns of pg_attribute in a
similar manner.  For that, create a new struct
FormExtraData_pg_attribute, which is to be passed around in parallel
to the tuple descriptor and optionally supplies the additional
columns.  Right now, this struct only contains one field for
attoptions, so no functionality is actually changed by this.

Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         | 21 ++++++++++++++-------
 src/backend/catalog/index.c        | 16 +++++++++++++++-
 src/include/catalog/heap.h         |  2 +-
 src/include/catalog/pg_attribute.h | 13 +++++++++++++
 src/tools/pgindent/typedefs.list   |  1 +
 5 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 5e773740f4d..de982c2c529 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -684,10 +684,11 @@ CheckAttributeType(const char *attname,
  *		Construct and insert a set of tuples in pg_attribute.
  *
  * Caller has already opened and locked pg_attribute.  tupdesc contains the
- * attributes to insert.  attcacheoff is always initialized to -1.  attoptions
- * supplies the values for the attoptions fields and must contain the same
- * number of elements as tupdesc or be NULL.  The other variable-length fields
- * of pg_attribute are always initialized to null values.
+ * attributes to insert.  attcacheoff is always initialized to -1.
+ * tupdesc_extra supplies the values for certain variable-length/nullable
+ * pg_attribute fields and must contain the same number of elements as tupdesc
+ * or be NULL.  The other variable-length fields of pg_attribute are always
+ * initialized to null values.
  *
  * indstate is the index state for CatalogTupleInsertWithInfo.  It can be
  * passed as NULL, in which case we'll fetch the necessary info.  (Don't do
@@ -701,7 +702,7 @@ void
 InsertPgAttributeTuples(Relation pg_attribute_rel,
 						TupleDesc tupdesc,
 						Oid new_rel_oid,
-						const Datum *attoptions,
+						const FormExtraData_pg_attribute tupdesc_extra[],
 						CatalogIndexState indstate)
 {
 	TupleTableSlot **slot;
@@ -723,6 +724,7 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 	while (natts < tupdesc->natts)
 	{
 		Form_pg_attribute attrs = TupleDescAttr(tupdesc, natts);
+		const FormExtraData_pg_attribute *attrs_extra = tupdesc_extra ? &tupdesc_extra[natts] : NULL;
 
 		ExecClearTuple(slot[slotCount]);
 
@@ -754,10 +756,15 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(attrs->attislocal);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1] = Int16GetDatum(attrs->attinhcount);
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
-		if (attoptions && attoptions[natts] != (Datum) 0)
-			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
+		if (attrs_extra)
+		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
+		}
 		else
+		{
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
+		}
 
 		/*
 		 * The remaining fields are not set for new columns.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index e6140853c05..7e428f3eb79 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -512,6 +512,20 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
 	TupleDesc	indexTupDesc;
+	FormExtraData_pg_attribute *attrs_extra = NULL;
+
+	if (attopts)
+	{
+		attrs_extra = palloc0_array(FormExtraData_pg_attribute, indexRelation->rd_att->natts);
+
+		for (int i = 0; i < indexRelation->rd_att->natts; i++)
+		{
+			if (attopts[i])
+				attrs_extra[i].attoptions.value = attopts[i];
+			else
+				attrs_extra[i].attoptions.isnull = true;
+		}
+	}
 
 	/*
 	 * open the attribute relation and its indexes
@@ -525,7 +539,7 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 	 */
 	indexTupDesc = RelationGetDescr(indexRelation);
 
-	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attopts, indstate);
+	InsertPgAttributeTuples(pg_attribute, indexTupDesc, InvalidOid, attrs_extra, indstate);
 
 	CatalogCloseIndexes(indstate);
 
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 1d7f8380d90..21e31f9c974 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -98,7 +98,7 @@ extern List *heap_truncate_find_FKs(List *relationIds);
 extern void InsertPgAttributeTuples(Relation pg_attribute_rel,
 									TupleDesc tupdesc,
 									Oid new_rel_oid,
-									const Datum *attoptions,
+									const FormExtraData_pg_attribute tupdesc_extra[],
 									CatalogIndexState indstate);
 
 extern void InsertPgClassTuple(Relation pg_class_desc,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 9f2706c4e87..1cc7a848f03 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -208,6 +208,19 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
  */
 typedef FormData_pg_attribute *Form_pg_attribute;
 
+/*
+ * FormExtraData_pg_attribute contains (some of) the fields that are not in
+ * FormData_pg_attribute because they are excluded by CATALOG_VARLEN.  It is
+ * meant to be used by DDL code so that the combination of
+ * FormData_pg_attribute (often via tuple descriptor) and
+ * FormExtraData_pg_attribute can be used to pass around all the information
+ * about an attribute.  Fields can be included here as needed.
+ */
+typedef struct FormExtraData_pg_attribute
+{
+	NullableDatum attoptions;
+} FormExtraData_pg_attribute;
+
 DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, pg_attribute, btree(attrelid oid_ops, attname name_ops));
 DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, pg_attribute, btree(attrelid oid_ops, attnum int2_ops));
 
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index aa7a25b8f8c..9ba7a9a56c0 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -850,6 +850,7 @@ FormData_pg_ts_parser
 FormData_pg_ts_template
 FormData_pg_type
 FormData_pg_user_mapping
+FormExtraData_pg_attribute
 Form_pg_aggregate
 Form_pg_am
 Form_pg_amop
-- 
2.44.0

v5-0003-Add-attstattarget-to-FormExtraData_pg_attribute.patchtext/plain; charset=UTF-8; name=v5-0003-Add-attstattarget-to-FormExtraData_pg_attribute.patchDownload
From 62a60b1b55596f745dea7c900f5a09537a1cdb32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 16:44:56 +0100
Subject: [PATCH v5 3/3] Add attstattarget to FormExtraData_pg_attribute

This allows setting attstattarget when a relation is created.

We make use of this by having index_concurrently_create_copy() copy
over the attstattarget values when the new index is created, instead
of having index_concurrently_swap() fix it up later.

Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184c76@eisentraut.org
---
 src/backend/catalog/heap.c         |  5 +-
 src/backend/catalog/index.c        | 97 +++++++++---------------------
 src/backend/catalog/toasting.c     |  2 +-
 src/backend/commands/indexcmds.c   |  2 +-
 src/include/catalog/index.h        |  1 +
 src/include/catalog/pg_attribute.h |  1 +
 6 files changed, 36 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index de982c2c529..cc31909012d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -758,18 +758,21 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
 		slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
 		if (attrs_extra)
 		{
+			slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.value;
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = attrs_extra->attstattarget.isnull;
+
 			slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.value;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = attrs_extra->attoptions.isnull;
 		}
 		else
 		{
+			slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 			slot[slotCount]->tts_isnull[Anum_pg_attribute_attoptions - 1] = true;
 		}
 
 		/*
 		 * The remaining fields are not set for new columns.
 		 */
-		slot[slotCount]->tts_isnull[Anum_pg_attribute_attstattarget - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attacl - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attfdwoptions - 1] = true;
 		slot[slotCount]->tts_isnull[Anum_pg_attribute_attmissingval - 1] = true;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7e428f3eb79..78265a1c1f0 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -107,7 +107,7 @@ static TupleDesc ConstructTupleDescriptor(Relation heapRelation,
 										  const Oid *opclassIds);
 static void InitializeAttributeOids(Relation indexRelation,
 									int numatts, Oid indexoid);
-static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts);
+static void AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets);
 static void UpdateIndexRelation(Oid indexoid, Oid heapoid,
 								Oid parentIndexId,
 								const IndexInfo *indexInfo,
@@ -507,7 +507,7 @@ InitializeAttributeOids(Relation indexRelation,
  * ----------------------------------------------------------------
  */
 static void
-AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
+AppendAttributeTuples(Relation indexRelation, const Datum *attopts, const NullableDatum *stattargets)
 {
 	Relation	pg_attribute;
 	CatalogIndexState indstate;
@@ -524,6 +524,11 @@ AppendAttributeTuples(Relation indexRelation, const Datum *attopts)
 				attrs_extra[i].attoptions.value = attopts[i];
 			else
 				attrs_extra[i].attoptions.isnull = true;
+
+			if (stattargets)
+				attrs_extra[i].attstattarget = stattargets[i];
+			else
+				attrs_extra[i].attstattarget.isnull = true;
 		}
 	}
 
@@ -730,6 +735,7 @@ index_create(Relation heapRelation,
 			 const Oid *opclassIds,
 			 const Datum *opclassOptions,
 			 const int16 *coloptions,
+			 const NullableDatum *stattargets,
 			 Datum reloptions,
 			 bits16 flags,
 			 bits16 constr_flags,
@@ -1024,7 +1030,7 @@ index_create(Relation heapRelation,
 	/*
 	 * append ATTRIBUTE tuples for the index
 	 */
-	AppendAttributeTuples(indexRelation, opclassOptions);
+	AppendAttributeTuples(indexRelation, opclassOptions, stattargets);
 
 	/* ----------------
 	 *	  update pg_index
@@ -1303,6 +1309,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	Datum	   *opclassOptions;
 	oidvector  *indclass;
 	int2vector *indcoloptions;
+	NullableDatum *stattargets;
 	bool		isnull;
 	List	   *indexColNames = NIL;
 	List	   *indexExprs = NIL;
@@ -1407,6 +1414,23 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
 		opclassOptions[i] = get_attoptions(oldIndexId, i + 1);
 
+	/* Extract statistic targets for each attribute */
+	stattargets = palloc0_array(NullableDatum, newInfo->ii_NumIndexAttrs);
+	for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
+	{
+		HeapTuple   tp;
+		Datum       dat;
+
+		tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(i + 1));
+		if (!HeapTupleIsValid(tp))
+			elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+				 i + 1, oldIndexId);
+		dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
+		ReleaseSysCache(tp);
+		stattargets[i].value = dat;
+		stattargets[i].isnull = isnull;
+	}
+
 	/*
 	 * Now create the new index.
 	 *
@@ -1428,6 +1452,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							  indclass->values,
 							  opclassOptions,
 							  indcoloptions->values,
+							  stattargets,
 							  reloptionsDatum,
 							  INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
 							  0,
@@ -1771,72 +1796,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	/* Copy data of pg_statistic from the old index to the new one */
 	CopyStatistics(oldIndexId, newIndexId);
 
-	/* Copy pg_attribute.attstattarget for each index attribute */
-	{
-		HeapTuple	attrTuple;
-		Relation	pg_attribute;
-		SysScanDesc scan;
-		ScanKeyData key[1];
-
-		pg_attribute = table_open(AttributeRelationId, RowExclusiveLock);
-		ScanKeyInit(&key[0],
-					Anum_pg_attribute_attrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(newIndexId));
-		scan = systable_beginscan(pg_attribute, AttributeRelidNumIndexId,
-								  true, NULL, 1, key);
-
-		while (HeapTupleIsValid((attrTuple = systable_getnext(scan))))
-		{
-			Form_pg_attribute att = (Form_pg_attribute) GETSTRUCT(attrTuple);
-			HeapTuple	tp;
-			Datum		dat;
-			bool		isnull;
-			Datum		repl_val[Natts_pg_attribute];
-			bool		repl_null[Natts_pg_attribute];
-			bool		repl_repl[Natts_pg_attribute];
-			HeapTuple	newTuple;
-
-			/* Ignore dropped columns */
-			if (att->attisdropped)
-				continue;
-
-			/*
-			 * Get attstattarget from the old index and refresh the new value.
-			 */
-			tp = SearchSysCache2(ATTNUM, ObjectIdGetDatum(oldIndexId), Int16GetDatum(att->attnum));
-			if (!HeapTupleIsValid(tp))
-				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-					 att->attnum, oldIndexId);
-			dat = SysCacheGetAttr(ATTNUM, tp, Anum_pg_attribute_attstattarget, &isnull);
-			ReleaseSysCache(tp);
-
-			/*
-			 * No need for a refresh if old index value is null.  (All new
-			 * index values are null at this point.)
-			 */
-			if (isnull)
-				continue;
-
-			memset(repl_val, 0, sizeof(repl_val));
-			memset(repl_null, false, sizeof(repl_null));
-			memset(repl_repl, false, sizeof(repl_repl));
-
-			repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
-			repl_val[Anum_pg_attribute_attstattarget - 1] = dat;
-
-			newTuple = heap_modify_tuple(attrTuple,
-										 RelationGetDescr(pg_attribute),
-										 repl_val, repl_null, repl_repl);
-			CatalogTupleUpdate(pg_attribute, &newTuple->t_self, newTuple);
-
-			heap_freetuple(newTuple);
-		}
-
-		systable_endscan(scan);
-		table_close(pg_attribute, RowExclusiveLock);
-	}
-
 	/* Close relations */
 	table_close(pg_class, RowExclusiveLock);
 	table_close(pg_index, RowExclusiveLock);
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 21be81c1fb3..738bc46ae82 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -323,7 +323,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 				 list_make2("chunk_id", "chunk_seq"),
 				 BTREE_AM_OID,
 				 rel->rd_rel->reltablespace,
-				 collationIds, opclassIds, NULL, coloptions, (Datum) 0,
+				 collationIds, opclassIds, NULL, coloptions, NULL, (Datum) 0,
 				 INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
 
 	table_close(toast_rel, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index de89be8d759..7b20d103c86 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1210,7 +1210,7 @@ DefineIndex(Oid tableId,
 					 stmt->oldNumber, indexInfo, indexColNames,
 					 accessMethodId, tablespaceId,
 					 collationIds, opclassIds, opclassOptions,
-					 coloptions, reloptions,
+					 coloptions, NULL, reloptions,
 					 flags, constr_flags,
 					 allowSystemTableMods, !check_rights,
 					 &createdConstraintId);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 2ef8512dbff..2dea96f47c3 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -80,6 +80,7 @@ extern Oid	index_create(Relation heapRelation,
 						 const Oid *opclassIds,
 						 const Datum *opclassOptions,
 						 const int16 *coloptions,
+						 const NullableDatum *stattargets,
 						 Datum reloptions,
 						 bits16 flags,
 						 bits16 constr_flags,
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 1cc7a848f03..1c62b8bfcb5 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -218,6 +218,7 @@ typedef FormData_pg_attribute *Form_pg_attribute;
  */
 typedef struct FormExtraData_pg_attribute
 {
+	NullableDatum attstattarget;
 	NullableDatum attoptions;
 } FormExtraData_pg_attribute;
 
-- 
2.44.0

#12Tomas Vondra
tomas.vondra@enterprisedb.com
In reply to: Peter Eisentraut (#11)
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:

-      <term><replaceable
class="parameter">new_target</replaceable></term>
+      <term><literal>SET STATISTICS { <replaceable
class="parameter">integer</replaceable> | DEFAULT }</literal></term>

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

#13Peter Eisentraut
peter@eisentraut.org
In reply to: Tomas Vondra (#12)
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.

#14Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#13)
1 attachment(s)
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

Attachments:

fix_compiler_warnings.patchtext/x-diff; charset=us-asciiDownload
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,