On Wed, Nov 25, 2020 at 04:12:15PM +0900, Michael Paquier wrote: > Yeah, we had better fix and I guess backpatch something here. That's > annoying.
I have considered that over the weekend, and let's be conservative. pg_database has gained a toast table since 12, and before that we would just have an error "row is too big", but nobody has really complained about that either. > One thing that strikes me as unwise is that we could run into similar > problems with vac_update_relstats() in the future, and there have been > recent talks about having more toast tables like for pg_class. That > is not worth caring about on stable branches because it is not an > active problem there, but we could do something better on HEAD. For now, I have added just a comment at the top of heap_inplace_update() to warn callers. Junfeng and Ashwin also mentioned to me off-list that their patch used a second copy for performance reasons, but I don't see why this could become an issue as we update each pg_database row only once a job is done. So I'd like to apply something like the attached on HEAD, comments are welcome. -- Michael
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 1b2f70499e..992e559216 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -5710,6 +5710,9 @@ heap_abort_speculative(Relation relation, ItemPointer tid) * * tuple is an in-memory tuple structure containing the data to be written * over the target tuple. Also, tuple->t_self identifies the target tuple. + * + * Note that the tuple updated here had better not come directly from the + * syscache if the relation has toasted values as these would be flattened. */ void heap_inplace_update(Relation relation, HeapTuple tuple) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index f1112111de..98270a1049 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1361,6 +1361,7 @@ vac_update_datfrozenxid(void) MultiXactId lastSaneMinMulti; bool bogus = false; bool dirty = false; + ScanKeyData key[1]; /* * Restrict this task to one backend per database. This avoids race @@ -1479,10 +1480,25 @@ vac_update_datfrozenxid(void) /* Now fetch the pg_database tuple we need to update. */ relation = table_open(DatabaseRelationId, RowExclusiveLock); - /* Fetch a copy of the tuple to scribble on */ - tuple = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId)); + /* + * Get the pg_database tuple to scribble on. Note that this does not + * directly rely on the syscache to avoid issues with flattened toast + * values for the in-place update. + */ + ScanKeyInit(&key[0], + Anum_pg_database_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(MyDatabaseId)); + + scan = systable_beginscan(relation, DatabaseOidIndexId, true, + NULL, 1, key); + tuple = systable_getnext(scan); + tuple = heap_copytuple(tuple); + systable_endscan(scan); + if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for database %u", MyDatabaseId); + dbform = (Form_pg_database) GETSTRUCT(tuple); /*
signature.asc
Description: PGP signature
