On Thu, Feb 27, 2020 at 09:07:35AM +0100, Julien Rouhaud wrote: > While looking at it, I see that invalid indexes seem to leaked when the table > is dropped, with no way to get rid of them: > > Shouldn't DROP TABLE be fixed to also drop invalid indexes?
Hmm. The problem here is that I think that we don't have the correct correct interface to handle the dependency switching between the old and new indexes from the start, and 68ac9cf made things better in some aspects (like non-cancellation and old index drop) but not in others (like yours, or even a column drop). changeDependenciesOf/On() have been added especially for REINDEX CONCURRENTLY, but they are not actually able to handle the case we want them to handle: do a switch for both relations within the same scan. It is possible to use three times the existing routines with a couple of CCIs in-between and what I would call a fake placeholder OID to switch all the records cleanly, but it would be actually cleaner to do a single scan of pg_depend and switch the dependencies of both objects at once. Attached is a draft patch to take care of that problem for HEAD. It still needs a lot of polishing (variable names are not actually old or new anymore, etc.) but that's enough to show the idea. If a version reaches PG12, we would need to keep around the past routines to avoid an ABI breakage, even if I doubt there are callers of it, but who knows.. -- Michael
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 0cd6fcf027..5811d082b0 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -200,10 +200,10 @@ extern long changeDependencyFor(Oid classId, Oid objectId,
Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
-extern long changeDependenciesOf(Oid classId, Oid oldObjectId,
+extern long switchDependenciesOf(Oid classId, Oid oldObjectId,
Oid newObjectId);
-extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+extern long switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
extern Oid getExtensionOfObject(Oid classId, Oid objectId);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..fb9bb276bb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,14 +1675,10 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
}
/*
- * Move all dependencies of and on the old index to the new one. First
- * remove any dependencies that the new index may have to provide an
- * initial clean state for the dependency switch, and then move all the
- * dependencies from the old index to the new one.
+ * Switch all dependencies of and on the old and new indexes.
*/
- deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
- changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
- changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+ switchDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
+ switchDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
/*
* Copy over statistics from old to new index
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f9af245eec..11caa38083 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -396,7 +396,7 @@ changeDependencyFor(Oid classId, Oid objectId,
}
/*
- * Adjust all dependency records to come from a different object of the same type
+ * Switch dependency records between two objects of the same type
*
* classId/oldObjectId specify the old referencing object.
* newObjectId is the new referencing object (must be of class classId).
@@ -404,38 +404,41 @@ changeDependencyFor(Oid classId, Oid objectId,
* Returns the number of records updated.
*/
long
-changeDependenciesOf(Oid classId, Oid oldObjectId,
+switchDependenciesOf(Oid classId, Oid oldObjectId,
Oid newObjectId)
{
long count = 0;
Relation depRel;
- ScanKeyData key[2];
+ ScanKeyData key;
SysScanDesc scan;
HeapTuple tup;
depRel = table_open(DependRelationId, RowExclusiveLock);
- ScanKeyInit(&key[0],
+ ScanKeyInit(&key,
Anum_pg_depend_classid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(classId));
- ScanKeyInit(&key[1],
- Anum_pg_depend_objid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(oldObjectId));
scan = systable_beginscan(depRel, DependDependerIndexId, true,
- NULL, 2, key);
+ NULL, 1, &key);
while (HeapTupleIsValid((tup = systable_getnext(scan))))
{
Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
+ if (depform->objid != oldObjectId &&
+ depform->objid != newObjectId)
+ continue;
+
/* make a modifiable copy */
tup = heap_copytuple(tup);
depform = (Form_pg_depend) GETSTRUCT(tup);
- depform->objid = newObjectId;
+ if (depform->objid == oldObjectId)
+ depform->objid = newObjectId;
+ else
+ depform->objid = oldObjectId;
CatalogTupleUpdate(depRel, &tup->t_self, tup);
@@ -452,7 +455,7 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
}
/*
- * Adjust all dependency records to point to a different object of the same type
+ * Switch all dependency records between two objects of the same type.
*
* refClassId/oldRefObjectId specify the old referenced object.
* newRefObjectId is the new referenced object (must be of class refClassId).
@@ -460,74 +463,75 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
* Returns the number of records updated.
*/
long
-changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId)
{
long count = 0;
Relation depRel;
- ScanKeyData key[2];
+ ScanKeyData key;
SysScanDesc scan;
HeapTuple tup;
- ObjectAddress objAddr;
- bool newIsPinned;
+ ObjectAddress newObjAddr;
+ ObjectAddress oldObjAddr;
depRel = table_open(DependRelationId, RowExclusiveLock);
/*
- * If oldRefObjectId is pinned, there won't be any dependency entries on
- * it --- we can't cope in that case. (This isn't really worth expending
- * code to fix, in current usage; it just means you can't rename stuff out
- * of pg_catalog, which would likely be a bad move anyway.)
+ * If oldRefObjectId or newRefObjectId are pinned, there won't be any
+ * dependency entries on it --- we can't cope in that case. (This
+ * isn't really worth expending code to fix, in current usage; it
+ * just means you can't rename stuff out of pg_catalog, which would
+ * likely be a bad move anyway.)
*/
- objAddr.classId = refClassId;
- objAddr.objectId = oldRefObjectId;
- objAddr.objectSubId = 0;
+ oldObjAddr.classId = refClassId;
+ oldObjAddr.objectId = oldRefObjectId;
+ oldObjAddr.objectSubId = 0;
- if (isObjectPinned(&objAddr, depRel))
+ if (isObjectPinned(&oldObjAddr, depRel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot remove dependency on %s because it is a system object",
- getObjectDescription(&objAddr))));
+ getObjectDescription(&oldObjAddr))));
- /*
- * We can handle adding a dependency on something pinned, though, since
- * that just means deleting the dependency entry.
- */
- objAddr.objectId = newRefObjectId;
+ newObjAddr.classId = refClassId;
+ newObjAddr.objectId = newRefObjectId;
+ newObjAddr.objectSubId = 0;
- newIsPinned = isObjectPinned(&objAddr, depRel);
+ if (isObjectPinned(&newObjAddr, depRel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot remove dependency on %s because it is a system object",
+ getObjectDescription(&newObjAddr))));
/* Now search for dependency records */
- ScanKeyInit(&key[0],
+ ScanKeyInit(&key,
Anum_pg_depend_refclassid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(refClassId));
- ScanKeyInit(&key[1],
- Anum_pg_depend_refobjid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(oldRefObjectId));
scan = systable_beginscan(depRel, DependReferenceIndexId, true,
- NULL, 2, key);
+ NULL, 1, &key);
while (HeapTupleIsValid((tup = systable_getnext(scan))))
{
Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
- if (newIsPinned)
- CatalogTupleDelete(depRel, &tup->t_self);
- else
- {
- /* make a modifiable copy */
- tup = heap_copytuple(tup);
- depform = (Form_pg_depend) GETSTRUCT(tup);
+ if (depform->refobjid != oldRefObjectId &&
+ depform->refobjid != newRefObjectId)
+ continue;
+ /* make a modifiable copy */
+ tup = heap_copytuple(tup);
+ depform = (Form_pg_depend) GETSTRUCT(tup);
+
+ if (depform->refobjid == oldRefObjectId)
depform->refobjid = newRefObjectId;
+ else
+ depform->refobjid = oldRefObjectId;
- CatalogTupleUpdate(depRel, &tup->t_self, tup);
+ CatalogTupleUpdate(depRel, &tup->t_self, tup);
- heap_freetuple(tup);
- }
+ heap_freetuple(tup);
count++;
}
signature.asc
Description: PGP signature
