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.
From d98742439bfe82a20cffcdda6d5a05fdd06b46ea Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.von...@enterprisedb.com>
Discussion:
https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184...@eisentraut.org
---
doc/src/sgml/catalogs.sgml | 28 +++++++--------
doc/src/sgml/ref/alter_statistics.sgml | 11 +++---
src/backend/commands/statscmds.c | 46 ++++++++++++++++--------
src/backend/commands/tablecmds.c | 47 +++++++++++++------------
src/backend/parser/gram.y | 4 +--
src/backend/statistics/extended_stats.c | 4 ++-
src/bin/pg_dump/pg_dump.c | 10 +++---
src/bin/psql/describe.c | 3 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_statistic_ext.h | 6 ++--
src/include/nodes/parsenodes.h | 2 +-
11 files changed, 93 insertions(+), 70 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 387a14b1869..2f091ad09d1 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7657,6 +7657,19 @@ <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
From 03338d740f2f7f9521e020fc4fe24a2bfaf3b79d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.von...@enterprisedb.com>
Discussion:
https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184...@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
From 62a60b1b55596f745dea7c900f5a09537a1cdb32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.von...@enterprisedb.com>
Discussion:
https://www.postgresql.org/message-id/flat/4da8d211-d54d-44b9-9847-f2a9f1184...@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
