On Thu, Feb 04, 2021 at 03:52:44PM +0100, Ronan Dunklau wrote: >> Hmmm, that sure seems like a bug, or at least unexpected behavior (that >> I don't see mentioned in the docs).
Yeah, per the rule of consistency, this classifies as a bug to me. > Please find attached a correct patch. ConstructTupleDescriptor() does not matter much, but this patch is not acceptable to me as it touches the area of the index creation while statistics on an index expression can only be changed with a special flavor of ALTER INDEX with column numbers. This would imply an ABI breakage, so it cannot be backpatched as-is. Let's copy this data in index_concurrently_swap() instead. The attached patch does that, and adds a test cheaper than what was proposed. There is a minor release planned for next week, so I may be better to wait after that so as we have enough time to agree on a solution. -- Michael
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index ae720c1496..77871aaefc 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -90,6 +90,7 @@ 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/backend/catalog/index.c b/src/backend/catalog/index.c
index 1cb9172a5f..9403ae64f8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1884,6 +1884,63 @@ 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);
+ 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;
+
+ /*
+ * Get attstattarget from the old index and refresh the new
+ * value.
+ */
+ attstattarget = get_attstattarget(oldIndexId, att->attnum);
+
+ /* no need for a refresh if both match */
+ if (attstattarget == att->attstattarget)
+ continue;
+
+ 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] = Int32GetDatum(attstattarget);
+
+ 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/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 85c458bc46..6bba5f8ec4 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -871,6 +871,33 @@ 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/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index ce734f7ef3..2c07940b98 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2559,6 +2559,7 @@ CREATE UNIQUE INDEX concur_exprs_index_pred ON concur_exprs_tab (c1)
CREATE UNIQUE INDEX concur_exprs_index_pred_2
ON concur_exprs_tab ((1 / c1))
WHERE ('-H') >= (c2::TEXT) COLLATE "C";
+ALTER INDEX concur_exprs_index_expr ALTER COLUMN 1 SET STATISTICS 100;
ANALYZE concur_exprs_tab;
SELECT starelid::regclass, count(*) FROM pg_statistic WHERE starelid IN (
'concur_exprs_index_expr'::regclass,
@@ -2638,6 +2639,20 @@ SELECT starelid::regclass, count(*) FROM pg_statistic WHERE starelid IN (
concur_exprs_index_expr | 1
(1 row)
+-- attstattarget should remain intact
+SELECT attrelid::regclass, attnum, attstattarget
+ FROM pg_attribute WHERE attrelid IN (
+ 'concur_exprs_index_expr'::regclass,
+ 'concur_exprs_index_pred'::regclass,
+ 'concur_exprs_index_pred_2'::regclass)
+ ORDER BY 'concur_exprs_index_expr'::regclass::text, attnum;
+ attrelid | attnum | attstattarget
+---------------------------+--------+---------------
+ concur_exprs_index_expr | 1 | 100
+ concur_exprs_index_pred | 1 | -1
+ concur_exprs_index_pred_2 | 1 | -1
+(3 rows)
+
DROP TABLE concur_exprs_tab;
-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
-- ON COMMIT PRESERVE ROWS, the default.
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index fd4f30876e..0f00275274 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1079,6 +1079,7 @@ CREATE UNIQUE INDEX concur_exprs_index_pred ON concur_exprs_tab (c1)
CREATE UNIQUE INDEX concur_exprs_index_pred_2
ON concur_exprs_tab ((1 / c1))
WHERE ('-H') >= (c2::TEXT) COLLATE "C";
+ALTER INDEX concur_exprs_index_expr ALTER COLUMN 1 SET STATISTICS 100;
ANALYZE concur_exprs_tab;
SELECT starelid::regclass, count(*) FROM pg_statistic WHERE starelid IN (
'concur_exprs_index_expr'::regclass,
@@ -1103,6 +1104,13 @@ SELECT starelid::regclass, count(*) FROM pg_statistic WHERE starelid IN (
'concur_exprs_index_pred'::regclass,
'concur_exprs_index_pred_2'::regclass)
GROUP BY starelid ORDER BY starelid::regclass::text;
+-- attstattarget should remain intact
+SELECT attrelid::regclass, attnum, attstattarget
+ FROM pg_attribute WHERE attrelid IN (
+ 'concur_exprs_index_expr'::regclass,
+ 'concur_exprs_index_pred'::regclass,
+ 'concur_exprs_index_pred_2'::regclass)
+ ORDER BY 'concur_exprs_index_expr'::regclass::text, attnum;
DROP TABLE concur_exprs_tab;
-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
signature.asc
Description: PGP signature
