Re: Preserve attstattarget on REINDEX CONCURRENTLY
On Wed, Feb 10, 2021 at 12:58:05AM -0600, Justin Pryzby wrote: > If I'm not wrong, you meant to ORDER BY attrelid::regclass::text, attnum; Indeed, I meant that. Thanks, Justin! -- Michael signature.asc Description: PGP signature
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On Fri, Feb 05, 2021 at 11:17:48AM +0900, Michael Paquier wrote: > 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 > +++ b/src/test/regress/sql/create_index.sql > @@ -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; If I'm not wrong, you meant to ORDER BY attrelid::regclass::text, attnum; -- Justin
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On Sun, Feb 07, 2021 at 09:39:36AM +0900, Michael Paquier wrote: > I'll see about applying this stuff after the next version is tagged > then. The new versions have been tagged, so done as of bd12080 and back-patched. I have added a note in the commit log about the approach to use index_create() instead for HEAD. -- Michael signature.asc Description: PGP signature
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On Sat, Feb 06, 2021 at 10:39:53PM +0100, Tomas Vondra wrote: > Copying this info in index_concurrently_swap seems a bit strange - we're > copying other stuff there, but this is modifying something we've already > copied before. I understand why we do it there to make this backpatchable, > but maybe it'd be good to mention this in a comment (or at least the commit > message). We could do this in the backbranches only and the "correct" way in > master, but that does not seem worth it. Thanks. > One minor comment - the code says this: > > /* no need for a refresh if both match */ > if (attstattarget == att->attstattarget) > continue; > > Isn't that just a different way to say "attstattarget is not default")? For REINDEX CONCURRENTLY, yes. I was thinking here about the case where this code is used for other purposes in the future, where attstattarget may not be -1. I'll see about applying this stuff after the next version is tagged then. -- Michael signature.asc Description: PGP signature
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On 2/5/21 8:43 AM, Michael Paquier wrote: On Fri, Feb 05, 2021 at 08:22:17AM +0100, Ronan Dunklau wrote: I'm not surprised by this answer, the good news is it's being back-patched. Yes, I have no problem with that. Until this gets fixed, the damage can be limited with an extra ALTER INDEX, that takes a ShareUpdateExclusiveLock so there is no impact on the concurrent activity. Looks good to me ! Thank you. Thanks for looking at it. Tomas, do you have any comments? -- Not really. Copying this info in index_concurrently_swap seems a bit strange - we're copying other stuff there, but this is modifying something we've already copied before. I understand why we do it there to make this backpatchable, but maybe it'd be good to mention this in a comment (or at least the commit message). We could do this in the backbranches only and the "correct" way in master, but that does not seem worth it. One minor comment - the code says this: /* no need for a refresh if both match */ if (attstattarget == att->attstattarget) continue; Isn't that just a different way to say "attstattarget is not default")? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On Fri, Feb 05, 2021 at 08:22:17AM +0100, Ronan Dunklau wrote: > I'm not surprised by this answer, the good news is it's being back-patched. Yes, I have no problem with that. Until this gets fixed, the damage can be limited with an extra ALTER INDEX, that takes a ShareUpdateExclusiveLock so there is no impact on the concurrent activity. > Looks good to me ! Thank you. Thanks for looking at it. Tomas, do you have any comments? -- Michael signature.asc Description: PGP signature
Re: Preserve attstattarget on REINDEX CONCURRENTLY
Le vendredi 5 février 2021, 03:17:48 CET Michael Paquier a écrit : > 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. I'm not surprised by this answer, the good news is it's being back-patched. > > 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. Looks good to me ! Thank you. -- Ronan Dunklau
Re: Preserve attstattarget on REINDEX CONCURRENTLY
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([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, >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
Re: Preserve attstattarget on REINDEX CONCURRENTLY
Hmmm, that sure seems like a bug, or at least unexpected behavior (that
I don't see mentioned in the docs).
But the patch seems borked in some way:
$ patch -p1 < ~/keep_attstattargets_on_reindex_concurrently.patch
patch: Only garbage was found in the patch input.
There seem to be strange escape characters and so on, how did you
create
the patch? Maybe some syntax coloring, or something?
You're right, I had syntax coloring in the output, sorry.
Please find attached a correct patch.
Regards,
--
Ronan Dunklaudiff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5a70fe4d2c..b4adb32af0 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -107,7 +107,8 @@ static TupleDesc ConstructTupleDescriptor(Relation heapRelation,
List *indexColNames,
Oid accessMethodObjectId,
Oid *collationObjectId,
- Oid *classObjectId);
+ Oid *classObjectId,
+ int32 *attstattargets);
static void InitializeAttributeOids(Relation indexRelation,
int numatts, Oid indexoid);
static void AppendAttributeTuples(Relation indexRelation, Datum *attopts);
@@ -272,7 +273,8 @@ ConstructTupleDescriptor(Relation heapRelation,
List *indexColNames,
Oid accessMethodObjectId,
Oid *collationObjectId,
- Oid *classObjectId)
+ Oid *classObjectId,
+ int32 *attstattargets)
{
int numatts = indexInfo->ii_NumIndexAttrs;
int numkeyatts = indexInfo->ii_NumIndexKeyAttrs;
@@ -310,12 +312,14 @@ 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) ?
collationObjectId[i] : InvalidOid;
-
+ if(attstattargets != NULL)
+ to->attstattarget = attstattargets[i];
+ else
+ to->attstattarget = -1;
/*
* Set the attribute name as specified by caller.
*/
@@ -697,6 +701,7 @@ index_create(Relation heapRelation,
Oid *collationObjectId,
Oid *classObjectId,
int16 *coloptions,
+ int32 *colstattargets,
Datum reloptions,
bits16 flags,
bits16 constr_flags,
@@ -882,7 +887,8 @@ index_create(Relation heapRelation,
indexColNames,
accessMethodObjectId,
collationObjectId,
- classObjectId);
+ classObjectId,
+ colstattargets);
/*
* Allocate an OID for the index, unless we were told what to use.
@@ -1413,11 +1419,13 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
optionDatum;
oidvector *indclass;
int2vector *indcoloptions;
+ int32 *colstattargets;
bool isnull;
List *indexColNames = NIL;
List *indexExprs = NIL;
List *indexPreds = NIL;
+
indexRelation = index_open(oldIndexId, RowExclusiveLock);
/* The new index needs some information from the old index */
@@ -1501,10 +1509,11 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
true);
/*
- * Extract the list of column names and the column numbers for the new
+ * Extract the list of column names, column numbers and stattargets for the new
* index information. All this information will be used for the index
* creation.
*/
+ colstattargets = palloc(sizeof(int32) * oldInfo->ii_NumIndexAttrs);
for (int i = 0; i < oldInfo->ii_NumIndexAttrs; i++)
{
TupleDesc indexTupDesc = RelationGetDescr(indexRelation);
@@ -1512,8 +1521,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indexColNames = lappend(indexColNames, NameStr(att->attname));
newInfo->ii_IndexAttrNumbers[i] = oldInfo->ii_IndexAttrNumbers[i];
+ colstattargets[i] = att->attstattarget;
}
-
/*
* Now create the new index.
*
@@ -1534,13 +1543,14 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indexRelation->rd_indcollation,
indclass->values,
indcoloptions->values,
+ colstattargets,
optionDatum,
INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
0,
true, /* allow table to be a system catalog? */
false, /* is_internal? */
NULL);
-
+ pfree(colstattargets);
/* Close the relations used and clean up */
index_close(indexRelation, NoLock);
ReleaseSysCache(indexTuple);
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index d7b806020d..09c1be3611 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -313,7 +313,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
rel->rd_rel->reltablespace,
- collationObjectId, classObjectId, coloptions, (Datum) 0,
+ collationObjectId, classObjectId, coloptions, NULL, (Datum) 0,
INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
Re: Preserve attstattarget on REINDEX CONCURRENTLY
On 2/4/21 11:04 AM, Ronan Dunklau wrote:
> Hello !
>
> ...
>
> junk=# REINDEX INDEX CONCURRENTLY t1_date_trunc_idx;
> REINDEX
> junk=# \d+ t1_date_trunc_idx
> Index "public.t1_date_trunc_idx"
>Column |Type | Key? | Definition
>
> | Storage | Stats target
> +-+--
> +-+-+--
> date_trunc | timestamp without time zone | yes | date_trunc('day'::text,
> ts)
> | plain |
> btree, for table "public.t1"
>
>
> I'm attaching a patch possibly solving the problem, but maybe the proposed
> changes will be too intrusive ?
>
Hmmm, that sure seems like a bug, or at least unexpected behavior (that
I don't see mentioned in the docs).
But the patch seems borked in some way:
$ patch -p1 < ~/keep_attstattargets_on_reindex_concurrently.patch
patch: Only garbage was found in the patch input.
There seem to be strange escape characters and so on, how did you create
the patch? Maybe some syntax coloring, or something?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Preserve attstattarget on REINDEX CONCURRENTLY
Hello !
We encountered the following bug recently in production: when running REINDEX
CONCURRENTLY on an index, the attstattarget is reset to 0.
Consider the following example:
junk=# \d+ t1_date_trunc_idx
Index "public.t1_date_trunc_idx"
Column |Type | Key? | Definition
| Storage | Stats target
+-+--
+-+-+--
date_trunc | timestamp without time zone | yes | date_trunc('day'::text, ts)
| plain | 1000
btree, for table "public.t1"
junk=# REINDEX INDEX t1_date_trunc_idx;
REINDEX
junk=# \d+ t1_date_trunc_idx
Index "public.t1_date_trunc_idx"
Column |Type | Key? | Definition
| Storage | Stats target
+-+--
+-+-+--
date_trunc | timestamp without time zone | yes | date_trunc('day'::text, ts)
| plain | 1000
btree, for table "public.t1"
junk=# REINDEX INDEX CONCURRENTLY t1_date_trunc_idx;
REINDEX
junk=# \d+ t1_date_trunc_idx
Index "public.t1_date_trunc_idx"
Column |Type | Key? | Definition
| Storage | Stats target
+-+--
+-+-+--
date_trunc | timestamp without time zone | yes | date_trunc('day'::text, ts)
| plain |
btree, for table "public.t1"
I'm attaching a patch possibly solving the problem, but maybe the proposed
changes will be too intrusive ?
Regards,
--
Ronan Dunklau[1mdiff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c[m
[1mindex 5a70fe4d2c..b4adb32af0 100644[m
[1m--- a/src/backend/catalog/index.c[m
[1m+++ b/src/backend/catalog/index.c[m
[36m@@ -107,7 +107,8 @@[m [mstatic TupleDesc ConstructTupleDescriptor(Relation heapRelation,[m
List *indexColNames,[m
Oid accessMethodObjectId,[m
Oid *collationObjectId,[m
[31m- Oid *classObjectId);[m
[32m+[m [32m Oid *classObjectId,[m
[32m+[m [32m int32 *attstattargets);[m
static void InitializeAttributeOids(Relation indexRelation,[m
int numatts, Oid indexoid);[m
static void AppendAttributeTuples(Relation indexRelation, Datum *attopts);[m
[36m@@ -272,7 +273,8 @@[m [mConstructTupleDescriptor(Relation heapRelation,[m
List *indexColNames,[m
Oid accessMethodObjectId,[m
Oid *collationObjectId,[m
[31m- Oid *classObjectId)[m
[32m+[m [32m Oid *classObjectId,[m
[32m+[m [32m int32 *attstattargets)[m
{[m
int numatts = indexInfo->ii_NumIndexAttrs;[m
int numkeyatts = indexInfo->ii_NumIndexKeyAttrs;[m
[36m@@ -310,12 +312,14 @@[m [mConstructTupleDescriptor(Relation heapRelation,[m
[m
MemSet(to, 0, ATTRIBUTE_FIXED_PART_SIZE);[m
to->attnum = i + 1;[m
[31m- to->attstattarget = -1;[m
to->attcacheoff = -1;[m
to->attislocal = true;[m
to->attcollation = (i < numkeyatts) ?[m
collationObjectId[i] : InvalidOid;[m
[31m-[m
[32m+[m [32mif(attstattargets != NULL)[m
[32m+[m [32mto->attstattarget = attstattargets[i];[m
[32m+[m [32melse[m
[32m+[m [32mto->attstattarget = -1;[m
/*[m
* Set the attribute name as specified by caller.[m
*/[m
[36m@@ -697,6 +701,7 @@[m [mindex_create(Relation heapRelation,[m
Oid *collationObjectId,[m
Oid *classObjectId,[m
int16 *coloptions,[m
[32m+[m [32m int32 *colstattargets,[m
Datum reloptions,[m
bits16 flags,[m
bits16 constr_flags,[m
[36m@@ -882,7 +887,8 @@[m [mindex_create(Relation heapRelation,[m
indexColNames,[m
accessMethodObjectId,[m
collationObjectId,[m
[31m- classObjectId);[m
[32m+[m [32mclassObjectId,[m
[32m+[m [32mcolstattargets);[m
[m
/*[m
* Allocate an OID for the index, unless we were told what to use.[m
[36m@@ -1413,11 +1419,13 @@[m [mindex_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,[m
optionDatum;[m
oidvector *indclass;[m
int2vector *indcoloptions;[m
[32m+[m [32mint32 *colstattargets;[m
bool isnull;[m
List *indexColNames = NIL;[m
List *indexExprs = NIL;[m
List *indexPreds = NIL;[m
[m
[32m+[m
indexRelation = index_open(oldIndexId, RowExclusiveLock);[m
[m
/* The new index needs some information from the old index */[m
[36m@@ -1501,10 +1509,11 @@[m [mindex_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,[m
true);[m
[m
/*[m
[31m- * Extract the list of column names and the column numbers for the new[m
[32m+[m [32m * Extract the list of column names, column numbers and stattargets for the new[m
*
