public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v13 06/18] More refactoring 3+ messages / 3 participants [nested] [flat]
* [PATCH v13 06/18] More refactoring @ 2020-11-01 19:46 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-11-01 19:46 UTC (permalink / raw) --- src/backend/commands/indexcmds.c | 201 ++++++++++----------- src/test/regress/expected/create_index.out | 4 +- 2 files changed, 93 insertions(+), 112 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index f5fea14ff4..5c5596cd28 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -102,8 +102,8 @@ static void ReindexMultipleInternal(List *relids, ReindexParams *params); static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params); -static List *ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, - ReindexParams *params, MemoryContext private_context); +static List *ReindexIndexesConcurrently(List *indexIds, ReindexParams *params, + MemoryContext private_context); static void update_relispartition(Oid relationId, bool newval); static inline void set_indexsafe_procflags(void); @@ -2652,8 +2652,8 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel) .indexId = indOid, /* other fields set later */ }; + ReindexIndexesConcurrently(list_make1(&idxinfo), - list_make1_oid(IndexGetRelation(indOid, false)), params, CurrentMemoryContext); } else @@ -3023,12 +3023,11 @@ reindex_error_callback(void *arg) /* - * Given a list of index oids, return a list of leaf partitions by removing - * any intermediate parents. heaprels is populated with the corresponding - * tables. + * Given a list of index oids, return a new list of leaf partitions by + * excluding any intermediate parents. */ static List * -leaf_indexes(List *inhoids, int options, List **heaprels) +leaf_indexes(List *inhoids, int options) { List *partitions = NIL; ListCell *lc; @@ -3060,7 +3059,6 @@ leaf_indexes(List *inhoids, int options, List **heaprels) /* Save partition OID in current MemoryContext */ partitions = lappend_oid(partitions, partoid); - *heaprels = lappend_oid(*heaprels, tableoid); } return partitions; @@ -3072,13 +3070,11 @@ leaf_indexes(List *inhoids, int options, List **heaprels) * * Reindex a set of partitions, per the partitioned index or table given * by the caller. - * XXX: should be further refactored with logic from ReindexRelationConcurrently */ static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) { - List *partitions = NIL, - *heaprels = NIL; + List *partitions = NIL; char relkind = get_rel_relkind(relid); char *relname = get_rel_name(relid); char *relnamespace = get_namespace_name(get_rel_namespace(relid)); @@ -3132,7 +3128,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) if (relkind == RELKIND_PARTITIONED_INDEX) { old_context = MemoryContextSwitchTo(reindex_context); - partitions = leaf_indexes(inhoids, params->options, &heaprels); + partitions = leaf_indexes(inhoids, params->options); MemoryContextSwitchTo(old_context); } else { /* Loop over parent tables */ @@ -3145,7 +3141,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) parttable = table_open(partoid, ShareLock); old_context = MemoryContextSwitchTo(reindex_context); partindexes = RelationGetIndexList(parttable); - partindexes = leaf_indexes(partindexes, params->options, &heaprels); + partindexes = leaf_indexes(partindexes, params->options); partitions = list_concat(partitions, partindexes); MemoryContextSwitchTo(old_context); @@ -3154,10 +3150,9 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) } if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 && - relkind == RELKIND_PARTITIONED_INDEX && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { - List *idxinfos = NIL; + List *idxinfos = NIL; ReindexIndexInfo *idxinfo; old_context = MemoryContextSwitchTo(reindex_context); @@ -3172,7 +3167,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) MemoryContextSwitchTo(old_context); /* Process all indexes in a single loop */ - ReindexIndexesConcurrently(idxinfos, heaprels, params, reindex_context); + ReindexIndexesConcurrently(idxinfos, params, reindex_context); } else { /* * Process each partition listed in a separate transaction. Note that @@ -3342,7 +3337,6 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) { - List *heapRelationIds = NIL; List *indexIds = NIL; List *newIndexIds = NIL; ListCell *lc, @@ -3395,14 +3389,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) */ Relation heapRelation; - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track this relation for session locks */ - heapRelationIds = lappend_oid(heapRelationIds, relationOid); - - MemoryContextSwitchTo(oldcontext); - if (IsCatalogRelationOid(relationOid)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -3415,7 +3401,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) ShareUpdateExclusiveLock); /* leave if relation does not exist */ if (!heapRelation) - break; + break; // XXX: lremove } else heapRelation = table_open(relationOid, @@ -3473,14 +3459,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) Relation toastRelation = table_open(toastOid, ShareUpdateExclusiveLock); - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track this relation for session locks */ - heapRelationIds = lappend_oid(heapRelationIds, toastOid); - - MemoryContextSwitchTo(oldcontext); - foreach(lc2, RelationGetIndexList(toastRelation)) { Oid cellOid = lfirst_oid(lc2); @@ -3521,78 +3499,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) break; } case RELKIND_INDEX: - { - Oid heapId = IndexGetRelation(relationOid, - (params->options & REINDEXOPT_MISSING_OK) != 0); - Relation heapRelation; - ReindexIndexInfo *idx; - - /* if relation is missing, leave */ - if (!OidIsValid(heapId)) - break; - - if (IsCatalogRelationOid(heapId)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot reindex system catalogs concurrently"))); - - /* - * Don't allow reindex for an invalid index on TOAST table, as - * if rebuilt it would not be possible to drop it. Match - * error message in reindex_index(). - */ - if (IsToastNamespace(get_rel_namespace(relationOid)) && - !get_index_isvalid(relationOid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot reindex invalid index on TOAST table"))); - - /* - * Check if parent relation can be locked and if it exists, - * this needs to be done at this stage as the list of indexes - * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK - * should not be used once all the session locks are taken. - */ - if ((params->options & REINDEXOPT_MISSING_OK) != 0) - { - heapRelation = try_table_open(heapId, - ShareUpdateExclusiveLock); - /* leave if relation does not exist */ - if (!heapRelation) - break; - } - else - heapRelation = table_open(heapId, - ShareUpdateExclusiveLock); - - if (OidIsValid(params->tablespaceOid) && - IsSystemRelation(heapRelation)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot move system relation \"%s\"", - get_rel_name(relationOid)))); - - table_close(heapRelation, NoLock); - - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track the heap relation of this index for session locks */ - heapRelationIds = list_make1_oid(heapId); - - /* - * Save the list of relation OIDs in private context. Note - * that invalid indexes are allowed here. - */ - idx = palloc(sizeof(ReindexIndexInfo)); - idx->indexId = relationOid; - indexIds = lappend(indexIds, idx); - /* other fields set later */ - - MemoryContextSwitchTo(oldcontext); - break; - } - case RELKIND_PARTITIONED_TABLE: case RELKIND_PARTITIONED_INDEX: default: @@ -3623,10 +3529,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) errmsg("cannot move non-shared relation to tablespace \"%s\"", get_tablespace_name(params->tablespaceOid)))); - Assert(heapRelationIds != NIL); + // Assert(heapRelationIds != NIL); /* Do the work */ - newIndexIds = ReindexIndexesConcurrently(indexIds, heapRelationIds, params, private_context); + newIndexIds = ReindexIndexesConcurrently(indexIds, params, private_context); /* Log what we did */ if ((params->options & REINDEXOPT_VERBOSE) != 0) @@ -3668,9 +3574,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) * This is called by ReindexRelationConcurrently and */ static List * -ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, - ReindexParams *params, MemoryContext private_context) +ReindexIndexesConcurrently(List *indexIds, ReindexParams *params, + MemoryContext private_context) { + List *heapRelationIds = NIL; List *newIndexIds = NIL; List *relationLocks = NIL; List *lockTags = NIL; @@ -3688,6 +3595,80 @@ ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, }; int64 progress_vals[4]; + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (params->tablespaceOid == GLOBALTABLESPACE_OID && false) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move non-shared relation to tablespace \"%s\"", + get_tablespace_name(params->tablespaceOid)))); + + foreach(lc, indexIds) + { + ReindexIndexInfo *idx = lfirst(lc); + Oid indexrelid = idx->indexId; + Oid heapId = IndexGetRelation(indexrelid, + (params->options & REINDEXOPT_MISSING_OK) != 0); + Relation heapRelation; + + /* if relation is missing, leave */ + if (!OidIsValid(heapId)) + break; // XXX: ldelete? + + if (IsCatalogRelationOid(heapId)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot reindex system catalogs concurrently"))); + + /* + * Don't allow reindex for an invalid index on TOAST table, as + * if rebuilt it would not be possible to drop it. Match + * error message in reindex_index(). + */ + if (IsToastNamespace(get_rel_namespace(indexrelid)) && + !get_index_isvalid(indexrelid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot reindex invalid index on TOAST table"))); + + if (OidIsValid(params->tablespaceOid) && + IsCatalogRelationOid(indexrelid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move system relation \"%s\"", + get_rel_name(indexrelid)))); + + /* + * Check if parent relation can be locked and if it exists, + * this needs to be done at this stage as the list of indexes + * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK + * should not be used once all the session locks are taken. + */ + if ((params->options & REINDEXOPT_MISSING_OK) != 0) + { + heapRelation = try_table_open(heapId, + ShareUpdateExclusiveLock); + /* leave if relation does not exist */ + if (!heapRelation) + break; // ldelete + } + else + heapRelation = table_open(heapId, + ShareUpdateExclusiveLock); + table_close(heapRelation, NoLock); + + /* Save the list of relation OIDs in private context */ + oldcontext = MemoryContextSwitchTo(private_context); + + /* Track the heap relation of this index for session locks */ + heapRelationIds = lappend_oid(heapRelationIds, heapId); + // heapRelationIds = list_make1_oid(heapId); + + /* Note that invalid indexes are allowed here. */ + + MemoryContextSwitchTo(oldcontext); + // break; + } + /*----- * Now we have all the indexes we want to process in indexIds. * diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 6f41adf736..830fdddf24 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2470,12 +2470,12 @@ COMMIT; REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation ERROR: cannot reindex system catalogs concurrently REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index -ERROR: concurrent index creation on system catalog tables is not supported +ERROR: cannot reindex system catalogs concurrently -- These are the toast table and index of pg_authid. REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table ERROR: cannot reindex system catalogs concurrently REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index -ERROR: concurrent index creation on system catalog tables is not supported +ERROR: cannot reindex system catalogs concurrently REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM ERROR: cannot reindex system catalogs concurrently -- Warns about catalog relations -- 2.17.0 --QTprm0S8XgL7H0Dt-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: do only critical work during single-user vacuum? @ 2022-02-03 22:02 Robert Haas <[email protected]> 2022-02-04 01:35 ` Re: do only critical work during single-user vacuum? Andres Freund <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Robert Haas @ 2022-02-03 22:02 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: John Naylor <[email protected]>; Peter Geoghegan <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Feb 3, 2022 at 4:50 PM Andres Freund <[email protected]> wrote: > I wonder if we shouldn't add some exceptions to the xid allocation > prevention. It makes sense that we don't allow random DML. But it's e.g. often > more realistic to drop / truncate a few tables with unimportant content, > rather than spend the time vacuuming those. We could e.g. allow xid > consumption within VACUUM, TRUNCATE, DROP TABLE / INDEX when run at the top > level for longer than we allow it for anything else. True, although we currently don't start refusing XID allocation altogether until only 1 million remain, IIRC. And that's cutting it really close if we need to start consuming 1 XID per table we need to drop. We might need to push out some of the thresholds a bit. For the most part, I think that there's no reason why autovacuum shouldn't be able to recover from this situation automatically, as long as old replication slots and prepared transactions are cleaned up and any old transactions are killed off. I don't think we're very far from that Just Working, but we are not all there yet either. Manual intervention to drop tables etc. is reasonable to allow a bit more than we do now, but the big problem IMO is that the behavior when we run short of XIDs has had very little testing and bug fixing, so things that don't really need to break just do anyway. > Indeed. Single user is the worst response to this (and just about anything > else, really). Even just getting into the single user mode takes a while > (shutdown checkpoint). The user interface is completely different (and > awful). The buffer cache is completely cold. The system is slower because > there's no wal writer / checkpointer running. Which basically is a list of > things one absolutely do not wants when confronted with a wraparound > situation. +1. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: do only critical work during single-user vacuum? 2022-02-03 22:02 Re: do only critical work during single-user vacuum? Robert Haas <[email protected]> @ 2022-02-04 01:35 ` Andres Freund <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Andres Freund @ 2022-02-04 01:35 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: John Naylor <[email protected]>; Peter Geoghegan <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 2022-02-03 17:02:15 -0500, Robert Haas wrote: > On Thu, Feb 3, 2022 at 4:50 PM Andres Freund <[email protected]> wrote: > > I wonder if we shouldn't add some exceptions to the xid allocation > > prevention. It makes sense that we don't allow random DML. But it's e.g. often > > more realistic to drop / truncate a few tables with unimportant content, > > rather than spend the time vacuuming those. We could e.g. allow xid > > consumption within VACUUM, TRUNCATE, DROP TABLE / INDEX when run at the top > > level for longer than we allow it for anything else. > > True, although we currently don't start refusing XID allocation > altogether until only 1 million remain, IIRC. And that's cutting it > really close if we need to start consuming 1 XID per table we need to > drop. We might need to push out some of the thresholds a bit. Yea, I'd have no problem leaving the "hard" limit somewhere closer to 1 million (although 100k should be just as well), but introduce a softer "only vacuum/drop/truncate" limit a good bit before that. > For the most part, I think that there's no reason why autovacuum > shouldn't be able to recover from this situation automatically, as > long as old replication slots and prepared transactions are cleaned up > and any old transactions are killed off. To address the "as long as" part: I think that describing better what is holding back the horizon would be a significant usability improvement. Imagine that instead of the generic hints in these messages: ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("database is not accepting commands to avoid wraparound data loss in database \"%s\"", oldest_datname), errhint("Stop the postmaster and vacuum that database in single-user mode.\n" "You might also need to commit or roll back old prepared transactions, or drop stale replication slots."))); and ereport(WARNING, (errmsg("oldest xmin is far in the past"), errhint("Close open transactions soon to avoid wraparound problems.\n" "You might also need to commit or roll back old prepared transactions, or drop stale replication slots."))); we'd actually tell the user a bit more what about what is causing the problem. We can compute the: 1) oldest slot by xmin, with name 2) oldest walsender by xmin, with pid 3) oldest prepared transaction id by xid / xmin, with name 4) oldest in-progress transaction id by xid / xmin, with name 5) oldest database datfrozenxid, with database name If 1-4) are close to 5), there's no point in trying to vacuum aggressively, it won't help. So we instead can say that the xmin horizon (with a better name) is held back by the oldest of these, with enough identifying information for the user to actually know where to look. In contrast, if 5) is older than 1-4), then we can tell the user which database is the problem, as we do right now, but we can stop mentioning the "You might also need to commit ..." bit. Also, adding an SRF providing the above in a useful format would be great for monitoring and for "remote debugging" of problems. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-02-04 01:35 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-01 19:46 [PATCH v13 06/18] More refactoring Justin Pryzby <[email protected]> 2022-02-03 22:02 Re: do only critical work during single-user vacuum? Robert Haas <[email protected]> 2022-02-04 01:35 ` Re: do only critical work during single-user vacuum? Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox