8000 Fix reindexing of pg_class indexes some more. · postgrespro/postgres_cluster@f912d7d · GitHub
[go: up one dir, main page]
Skip to content

Commit f912d7d

Browse files
tglsfdctglsfdc
committed
Fix reindexing of pg_class indexes some more.
Commits 3dbb317 et al failed under CLOBBER_CACHE_ALWAYS testing. Investigation showed that to reindex pg_class_oid_index, we must suppress accesses to the index (via SetReindexProcessing) before we call RelationSetNewRelfilenode, or at least before we do CommandCounterIncrement therein; otherwise, relcache reloads happening within the CCI may try to fetch pg_class rows using the index's new relfilenode value, which is as yet an empty file. Of course, the point of 3dbb317 was that that ordering didn't work either, because then RelationSetNewRelfilenode's own update of the index's pg_class row cannot access the index, should it need to. There are various ways we might have got around that, but Andres Freund came up with a brilliant solution: for a mapped index, we can really just skip the pg_class update altogether. The only fields it was actually changing were relpages etc, but it was just setting them to zeroes which is useless make-work. (Correct new values will be installed at the end of index build.) All pg_class indexes are mapped and probably always will be, so this eliminates the problem by removing work rather than adding it, always a pleasant outcome. Having taught RelationSetNewRelfilenode to do it that way, we can revert the code reordering in reindex_index. (But I left the moved setup code where it was; there seems no reason why it has to run without use of the old index. If you're trying to fix a busted pg_class index, you'll have had to disable system index use altogether to get this far.) Moreover, this means we don't need RelationSetIndexList at all, because reindex_relation's hacking to make "REINDEX TABLE pg_class" work is likewise now unnecessary. We'll leave that code in place in the back branches, but a follow-on patch will remove it in HEAD. In passing, do some minor cleanup for commit 5c15606 (in HEAD only), notably removing a duplicate newrnode assignment. Patch by me, using a core idea due to Andres Freund. Back-patch to all supported branches, as 3dbb317 was. Discussion: https://postgr.es/m/28926.1556664156@sss.pgh.pa.us
1 parent 2bf372a commit f912d7d

File tree

2 files changed

+60
-75
lines changed

2 files changed

+60
-75
lines changed

‎src/backend/catalog/index.c

Lines changed: 3 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -3330,20 +3330,15 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
33303330
indexInfo->ii_ExclusionStrats = NULL;
33313331
}
33323332

3333-
/*
3334-
* Build a new physical relation for the index. Need to do that before
3335-
* "officially" starting the reindexing with SetReindexProcessing -
3336-
* otherwise the necessary pg_class changes cannot be made with
3337-
* encountering assertions.
3338-
*/
3339-
RelationSetNewRelfilenode(iRel, persistence);
3340-
33413333
/* ensure SetReindexProcessing state isn't leaked */
33423334
PG_TRY();
33433335
{
33443336
/* Suppress use of the target index while rebuilding it */
33453337
SetReindexProcessing(heapId, indexId);
33463338

3339+
/* Create a new physical relation for the index */
3340+
RelationSetNewRelfilenode(iRel, persistence);
3341+
33473342
/* Initialize the index and rebuild */
33483343
/* Note: we do not need to re-establish pkey setting */
33493344
index_build(heapRelation, iRel, indexInfo, true, true);
@@ -3491,7 +3486,6 @@ reindex_relation(Oid relid, int flags, int options)
34913486
Relation rel;
34923487
Oid toast_relid;
34933488
List *indexIds;
3494-
bool is_pg_class;
34953489
bool result;
34963490
int i;
34973491

@@ -3527,32 +3521,8 @@ reindex_relation(Oid relid, int flags, int options)
35273521
*/
35283522
indexIds = RelationGetIndexList(rel);
35293523

3530-
/*
3531-
* reindex_index will attempt to update the pg_class rows for the relation
3532-
* and index. If we are processing pg_class itself, we want to make sure
3533-
* that the updates do not try to insert index entries into indexes we
3534-
* have not processed yet. (When we are trying to recover from corrupted
3535-
* indexes, that could easily cause a crash.) We can accomplish this
3536-
* because CatalogTupleInsert/CatalogTupleUpdate will use the relcache's
3537-
* index list to know which indexes to update. We just force the index
3538-
* list to be only the stuff we've processed.
3539-
*
3540-
* It is okay to not insert entries into the indexes we have not processed
3541-
* yet because all of this is transaction-safe. If we fail partway
3542-
* through, the updated rows are dead and it doesn't matter whether they
3543-
* have index entries. Also, a new pg_class index will be created with a
3544-
* correct entry for its own pg_class row because we do
3545-
* RelationSetNewRelfilenode() before we do index_build().
3546-
*/
3547-
is_pg_class = (RelationGetRelid(rel) == RelationRelationId);
3548-
3549-
/* Ensure rd_indexattr is valid; see comments for RelationSetIndexList */
3550-
if (is_pg_class)
3551-
(void) RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_ALL);
3552-
35533524
PG_TRY();
35543525
{
3555-
List *doneIndexes;
35563526
ListCell *indexId;
35573527
char persistence;
35583528

@@ -3580,15 +3550,11 @@ reindex_relation(Oid relid, int flags, int options)
35803550
persistence = rel->rd_rel->relpersistence;
35813551

35823552
/* Reindex all the indexes. */
3583-
doneIndexes = NIL;
35843553
i = 1;
35853554
foreach(indexId, indexIds)
35863555
{
35873556
Oid indexOid = lfirst_oid(indexId);
35883557

3589-
if (is_pg_class)
3590-
RelationSetIndexList(rel, doneIndexes);
3591-
35923558
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
35933559
persistence, options);
35943560

@@ -3597,9 +3563,6 @@ reindex_relation(Oid relid, int flags, int options)
35973563
/* Index should no longer be in the pending list */
35983564
Assert(!ReindexIsProcessingIndex(indexOid));
35993565

3600-
if (is_pg_class)
3601-
doneIndexes = lappend_oid(doneIndexes, indexOid);
3602-
36033566
/* Set index rebuild count */
36043567
pgstat_progress_update_param(PROGRESS_CLUSTER_INDEX_REBUILD_COUNT,
36053568
i);
@@ -3615,9 +3578,6 @@ reindex_relation(Oid relid, int flags, int options)
36153578
PG_END_TRY();
36163579
ResetReindexPending();
36173580

3618-
if (is_pg_class)
3619-
RelationSetIndexList(rel, indexIds);
3620-
36213581
/*
36223582
* Close rel, but continue to hold the lock.
36233583
*/

‎src/backend/utils/cache/relcache.c

Lines changed: 57 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -3421,7 +3421,8 @@ RelationBuildLocalRelation(const char *relname,
34213421
/*
34223422
* RelationSetNewRelfilenode
34233423
*
3424-
* Assign a new relfilenode (physical file name) to the relation.
3424+
* Assign a new relfilenode (physical file name), and possibly a new
3425+
* persistence setting, to the relation.
34253426
*
34263427
* This allows a full rewrite of the relation to be done with transactional
34273428
* safety (since the filenode assignment can be rolled back). Note however
@@ -3463,13 +3464,10 @@ RelationSetNewRelfilenode(Relation relation, char persistence)
34633464
*/
34643465
RelationDropStorage(relation);
34653466

3466-
/* initialize new relfilenode from old relfilenode */
3467-
newrnode = relation->rd_node;
3468-
34693467
/*
3470-
* Create storage for the main fork of the new relfilenode. If it's
3471-
* table-like object, call into table AM to do so, which'll also create
3472-
* the table's init fork.
3468+
* Create storage for the main fork of the new relfilenode. If it's a
3469+
* table-like object, call into the table AM to do so, which'll also
3470+
* create the table's init fork if needed.
34733471
*
34743472
* NOTE: If relevant for the AM, any conflict in relfilenode value will be
34753473
* caught here, if GetNewRelFileNode messes up for any reason.
@@ -3479,18 +3477,10 @@ RelationSetNewRelfilenode(Relation relation, char persistence)
34793477

34803478
switch (relation->rd_rel->relkind)
34813479
{
3482-
/* shouldn't be called for these */
3483-
case RELKIND_VIEW:
3484-
case RELKIND_COMPOSITE_TYPE:
3485-
case RELKIND_FOREIGN_TABLE:
3486-
case RELKIND_PARTITIONED_TABLE:
3487-
case RELKIND_PARTITIONED_INDEX:
3488-
elog(ERROR, "should not have storage");
3489-
break;
3490-
34913480
case RELKIND_INDEX:
34923481
case RELKIND_SEQUENCE:
34933482
{
3483+
/* handle these directly, at least for now */
34943484
SMgrRelation srel;
34953485

34963486
srel = RelationCreateStorage(newrnode, persistence);
@@ -3505,41 +3495,76 @@ RelationSetNewRelfilenode(Relation relation, char persistence)
35053495
persistence,
35063496
&freezeXid, &minmulti);
35073497
break;
3498+
3499+
default:
3500+
/* we shouldn't be called for anything else */
3501+
elog(ERROR, "relation \"%s\" does not have storage",
3502+
RelationGetRelationName(relation));
3503+
break;
35083504
}
35093505

35103506
/*
3511-
* However, if we're dealing with a mapped index, pg_class.relfilenode
3512-
* doesn't change; instead we have to send the update to the relation
3513-
* mapper.
3507+
* If we're dealing with a mapped index, pg_class.relfilenode doesn't
3508+
* change; instead we have to send the update to the relation mapper.
3509+
*
3510+
* For mapped indexes, we don't actually change the pg_class entry at all;
3511+
* this is essential when reindexing pg_class itself. That leaves us with
3512+
* possibly-inaccurate values of relpages etc, but those will be fixed up
3513+
* later.
35143514
*/
35153515
if (RelationIsMapped(relation))
3516+
{
3517+
/* This case is only supported for indexes */
3518+
Assert(relation->rd_rel->relkind == RELKIND_INDEX);
3519+
3520+
/* Since we're not updating pg_class, these had better not change */
3521+
Assert(classform->relfrozenxid == freezeXid);
3522+
Assert(classform->relminmxid == minmulti);
3523+
Assert(classform->relpersistence == persistence);
3524+
3525+
/*
3526+
* In some code paths it's possible that the tuple update we'd
3527+
* otherwise do here is the only thing that would assign an XID for
3528+
* the current transaction. However, we must have an XID to delete
3529+
* files, so make sure one is assigned.
3530+
*/
3531+
(void) GetCurrentTransactionId();
3532+
3533+
/* Do the deed */
35163534
RelationMapUpdateMap(RelationGetRelid(relation),
35173535
newrelfilenode,
35183536
relation->rd_rel->relisshared,
35193537
false);
3538+
3539+
/* Since we're not updating pg_class, must trigger inval manually */
3540+
CacheInvalidateRelcache(relation);
3541+
}
35203542
else
3543+
{
3544+
/* Normal case, update the pg_class entry */
35213545
classform->relfilenode = newrelfilenode;
35223546

3523-
/* These changes are safe even for a mapped relation */
3524-
if (relation->rd_rel->relkind != RELKIND_SEQUENCE)
3525-
{
3526-
classform->relpages = 0; /* it's empty until further notice */
3527-
classform->reltuples = 0;
3528-
classform->relallvisible = 0;
3529-
}
3530-
classform->relfrozenxid = freezeXid;
3531-
classform->relminmxid = minmulti;
3532-
classform->relpersistence = persistence;
3547+
/* relpages etc. never change for sequences */
3548+
if (relation->rd_rel->relkind != RELKIND_SEQUENCE)
3549+
{
3550+
classform->relpages = 0; /* it's empty until further notice */
3551+
classform->reltuples = 0;
3552+
classform->relallvisible = 0;
3553+
}
3554+
classform->relfrozenxid = freezeXid;
3555+
classform->relminmxid = minmulti;
3556+
classform->relpersistence = persistence;
35333557

3534-
CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
3558+
CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
3559+
}
35353560

35363561
heap_freetuple(tuple);
35373562

35383563
table_close(pg_class, RowExclusiveLock);
35393564

35403565
/*
3541-
* Make the pg_class row change visible, as well as the relation map
3542-
* change if any. This will cause the relcache entry to get updated, too.
3566+
* Make the pg_class row change or relation map change visible. This will
3567+
* cause the relcache entry to get updated, too.
35433568
*/
35443569
CommandCounterIncrement();
35453570

0 commit comments

Comments
 (0)
0