Dimitri Fontaine <dimi...@2ndquadrant.fr> writes:
> Side note: as soon as we have CREATE EXTENSION AS $$ script $$; we will
> be able to add those cases as regression tests. That's not the main
> usage of that feature, by far, but I can't resits the occasion :)
Oh, I did already mention it :)
> I have some local failures in `make check` that I'm not sure originate
> from that patch. Still wanted to have an opinion about the idea before
> cleaning up.
Sorry for sending unfinished preliminary version, I just had the
opportunity to look at what happened: views will create a composite type
that needs its pg_class row updated when doing ALTER VIEW SET SCHEMA.
That means that we need proper tracking for that operation even when it
happens outside of an extension update script, as in the attached
version 4 of the patch.
I think the way forward is to use the simplest one for back branches and
this one for master only, unless it is appreciated of light enough
impact, right? (provided it's ok, too)
git diff --stat
src/backend/commands/alter.c | 14 +----
src/backend/commands/extension.c | 48 +++++++++------
src/backend/commands/tablecmds.c | 122
+++++++++++++++++++++++++++-----------
src/backend/commands/typecmds.c | 33 +++++++++-
src/include/commands/alter.h | 4 +-
src/include/commands/tablecmds.h | 7 ++-
src/include/commands/typecmds.h | 6 +-
7 files changed, 161 insertions(+), 73 deletions(-)
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
***************
*** 247,253 **** ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
* object doesn't have a schema.
*/
Oid
! AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
{
Oid oldNspOid = InvalidOid;
ObjectAddress dep;
--- 247,254 ----
* object doesn't have a schema.
*/
Oid
! AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
! ObjectAddresses *objsMoved)
{
Oid oldNspOid = InvalidOid;
ObjectAddress dep;
***************
*** 261,280 **** AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
case OCLASS_CLASS:
{
Relation rel;
- Relation classRel;
rel = relation_open(objid, AccessExclusiveLock);
oldNspOid = RelationGetNamespace(rel);
! classRel = heap_open(RelationRelationId, RowExclusiveLock);
!
! AlterRelationNamespaceInternal(classRel,
! objid,
! oldNspOid,
! nspOid,
! true);
!
! heap_close(classRel, RowExclusiveLock);
relation_close(rel, NoLock);
break;
--- 262,272 ----
case OCLASS_CLASS:
{
Relation rel;
rel = relation_open(objid, AccessExclusiveLock);
oldNspOid = RelationGetNamespace(rel);
! AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
relation_close(rel, NoLock);
break;
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
***************
*** 2203,2208 **** AlterExtensionNamespace(List *names, const char *newschema)
--- 2203,2209 ----
Relation depRel;
SysScanDesc depScan;
HeapTuple depTup;
+ ObjectAddresses *objsMoved;
if (list_length(names) != 1)
ereport(ERROR,
***************
*** 2277,2282 **** AlterExtensionNamespace(List *names, const char *newschema)
--- 2278,2285 ----
errmsg("extension \"%s\" does not support SET SCHEMA",
NameStr(extForm->extname))));
+ objsMoved = new_object_addresses();
+
/*
* Scan pg_depend to find objects that depend directly on the extension,
* and alter each one's schema.
***************
*** 2316,2343 **** AlterExtensionNamespace(List *names, const char *newschema)
if (dep.objectSubId != 0) /* should not happen */
elog(ERROR, "extension should not have a sub-object dependency");
! dep_oldNspOid = AlterObjectNamespace_oid(dep.classId,
! dep.objectId,
! nspOid);
! /*
! * Remember previous namespace of first object that has one
! */
! if (oldNspOid == InvalidOid && dep_oldNspOid != InvalidOid)
! oldNspOid = dep_oldNspOid;
! /*
! * If not all the objects had the same old namespace (ignoring any
! * that are not in namespaces), complain.
! */
! if (dep_oldNspOid != InvalidOid && dep_oldNspOid != oldNspOid)
! ereport(ERROR,
! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("extension \"%s\" does not support SET SCHEMA",
! NameStr(extForm->extname)),
! errdetail("%s is not in the extension's schema \"%s\"",
! getObjectDescription(&dep),
! get_namespace_name(oldNspOid))));
}
systable_endscan(depScan);
--- 2319,2351 ----
if (dep.objectSubId != 0) /* should not happen */
elog(ERROR, "extension should not have a sub-object dependency");
! /* Relocate the object, if it hasn't been already relocated */
! if (!object_address_present(&dep, objsMoved))
! {
! dep_oldNspOid = AlterObjectNamespace_oid(dep.classId,
! dep.objectId,
! nspOid,
! objsMoved);
! /*
! * Remember previous namespace of first object that has one
! */
! if (oldNspOid == InvalidOid && dep_oldNspOid != InvalidOid)
! oldNspOid = dep_oldNspOid;
! /*
! * If not all the objects had the same old namespace (ignoring any
! * that are not in namespaces), complain.
! */
! if (dep_oldNspOid != InvalidOid && dep_oldNspOid != oldNspOid)
! ereport(ERROR,
! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("extension \"%s\" does not support SET SCHEMA",
! NameStr(extForm->extname)),
! errdetail("%s is not in the extension's schema \"%s\"",
! getObjectDescription(&dep),
! get_namespace_name(oldNspOid))));
! }
}
systable_endscan(depScan);
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 263,270 **** static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid);
static void AlterSeqNamespaces(Relation classRel, Relation rel,
! Oid oldNspOid, Oid newNspOid,
! const char *newNspName, LOCKMODE lockmode);
static void ATExecValidateConstraint(Relation rel, char *constrName,
bool recurse, bool recursing, LOCKMODE lockmode);
static int transformColumnNameList(Oid relId, List *colList,
--- 263,270 ----
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid);
static void AlterSeqNamespaces(Relation classRel, Relation rel,
! Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
! LOCKMODE lockmode);
static void ATExecValidateConstraint(Relation rel, char *constrName,
bool recurse, bool recursing, LOCKMODE lockmode);
static int transformColumnNameList(Oid relId, List *colList,
***************
*** 9710,9716 **** AlterTableNamespace(AlterObjectSchemaStmt *stmt)
Oid relid;
Oid oldNspOid;
Oid nspOid;
- Relation classRel;
RangeVar *newrv;
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
--- 9710,9715 ----
***************
*** 9752,9778 **** AlterTableNamespace(AlterObjectSchemaStmt *stmt)
/* common checks on switching namespaces */
CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
/* OK, modify the pg_class row and pg_depend entry */
classRel = heap_open(RelationRelationId, RowExclusiveLock);
! AlterRelationNamespaceInternal(classRel, relid, oldNspOid, nspOid, true);
/* Fix the table's row type too */
! AlterTypeNamespaceInternal(rel->rd_rel->reltype, nspOid, false, false);
/* Fix other dependent stuff */
if (rel->rd_rel->relkind == RELKIND_RELATION)
{
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid);
! AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid, stmt->newschema,
! AccessExclusiveLock);
! AlterConstraintNamespaces(relid, oldNspOid, nspOid, false);
}
heap_close(classRel, RowExclusiveLock);
-
- /* close rel, but keep lock until commit */
- relation_close(rel, NoLock);
}
/*
--- 9751,9809 ----
/* common checks on switching namespaces */
CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
+ AlterTableNamespaceInternal(rel, oldNspOid, nspOid, NULL);
+
+ /* close rel, but keep lock until commit */
+ relation_close(rel, NoLock);
+ }
+
+ /*
+ * The guts of relocating a table to another namespace: besides moving
+ * the table itself, its dependent objects are relocated to the new schema.
+ *
+ * That function can be part of implementing ALTER EXTENSION SET SCHEMA, and
+ * direct dependencies in an extension include both sequences and types:
+ * tracking needs to be passed down those.
+ *
+ * To list which objects need further tracking and check why indexes and
+ * constraint are ok here, see src/backend/commands/alter.c
+ * AlterObjectNamespace_oid() function and its switch.
+ */
+ void
+ AlterTableNamespaceInternal(Relation rel, Oid oldNspOid, Oid nspOid,
+ ObjectAddresses *objsMoved)
+ {
+ Relation classRel;
+
/* OK, modify the pg_class row and pg_depend entry */
classRel = heap_open(RelationRelationId, RowExclusiveLock);
! if (objsMoved == NULL)
! {
! /* we need to track dependencies for types under views, those are
! * composite type and will get back to pg_class
! */
! objsMoved = new_object_addresses();
! }
!
! AlterRelationNamespaceInternal(classRel, RelationGetRelid(rel), oldNspOid,
! nspOid, true, objsMoved);
/* Fix the table's row type too */
! AlterTypeNamespaceInternal(rel->rd_rel->reltype,
! nspOid, false, false, objsMoved);
/* Fix other dependent stuff */
if (rel->rd_rel->relkind == RELKIND_RELATION)
{
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid);
! AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid,
! objsMoved, AccessExclusiveLock);
! AlterConstraintNamespaces(RelationGetRelid(rel), oldNspOid, nspOid,
! false);
}
heap_close(classRel, RowExclusiveLock);
}
/*
***************
*** 9783,9792 **** AlterTableNamespace(AlterObjectSchemaStmt *stmt)
void
AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
Oid oldNspOid, Oid newNspOid,
! bool hasDependEntry)
{
HeapTuple classTup;
Form_pg_class classForm;
classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
if (!HeapTupleIsValid(classTup))
--- 9814,9824 ----
void
AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
Oid oldNspOid, Oid newNspOid,
! bool hasDependEntry, ObjectAddresses *objsMoved)
{
HeapTuple classTup;
Form_pg_class classForm;
+ ObjectAddress thisobj;
classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
if (!HeapTupleIsValid(classTup))
***************
*** 9795,9821 **** AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
Assert(classForm->relnamespace == oldNspOid);
! /* check for duplicate name (more friendly than unique-index failure) */
! if (get_relname_relid(NameStr(classForm->relname),
! newNspOid) != InvalidOid)
! ereport(ERROR,
! (errcode(ERRCODE_DUPLICATE_TABLE),
! errmsg("relation \"%s\" already exists in schema \"%s\"",
! NameStr(classForm->relname),
! get_namespace_name(newNspOid))));
! /* classTup is a copy, so OK to scribble on */
! classForm->relnamespace = newNspOid;
! simple_heap_update(classRel, &classTup->t_self, classTup);
! CatalogUpdateIndexes(classRel, classTup);
! /* Update dependency on schema if caller said so */
! if (hasDependEntry &&
! changeDependencyFor(RelationRelationId, relOid,
! NamespaceRelationId, oldNspOid, newNspOid) != 1)
! elog(ERROR, "failed to change schema dependency for relation \"%s\"",
! NameStr(classForm->relname));
heap_freetuple(classTup);
}
--- 9827,9874 ----
Assert(classForm->relnamespace == oldNspOid);
! /* we need thisobj for tracking with objsMoved */
! if (objsMoved)
! {
! thisobj.classId = RelationRelationId;
! thisobj.objectId = relOid;
! thisobj.objectSubId = 0;
! }
! /*
! Do nothing when there's nothing to do. Note that currently the relation
! shouldn't already be in objsMoved, as this function is only called for
! plain relations and we have no dependency path leading to them.
! */
! /* if (classForm->relnamespace != newNspOid */
! /* && (objsMoved && !object_address_present(&thisobj, objsMoved))) */
! if (objsMoved && !object_address_present(&thisobj, objsMoved))
! {
! /* check for duplicate name (more friendly than unique-index failure) */
! if (get_relname_relid(NameStr(classForm->relname),
! newNspOid) != InvalidOid)
! ereport(ERROR,
! (errcode(ERRCODE_DUPLICATE_TABLE),
! errmsg("relation \"%s\" already exists in schema \"%s\"",
! NameStr(classForm->relname),
! get_namespace_name(newNspOid))));
!
! /* classTup is a copy, so OK to scribble on */
! classForm->relnamespace = newNspOid;
! simple_heap_update(classRel, &classTup->t_self, classTup);
! CatalogUpdateIndexes(classRel, classTup);
! /* Update dependency on schema if caller said so */
! if (hasDependEntry &&
! changeDependencyFor(RelationRelationId, relOid,
! NamespaceRelationId, oldNspOid, newNspOid) != 1)
! elog(ERROR, "failed to change schema dependency for relation \"%s\"",
! NameStr(classForm->relname));
!
! if (objsMoved)
! add_exact_object_address(&thisobj, objsMoved);
! }
heap_freetuple(classTup);
}
***************
*** 9846,9852 **** AlterIndexNamespaces(Relation classRel, Relation rel,
*/
AlterRelationNamespaceInternal(classRel, indexOid,
oldNspOid, newNspOid,
! false);
}
list_free(indexList);
--- 9899,9905 ----
*/
AlterRelationNamespaceInternal(classRel, indexOid,
oldNspOid, newNspOid,
! false, NULL);
}
list_free(indexList);
***************
*** 9861,9867 **** AlterIndexNamespaces(Relation classRel, Relation rel,
*/
static void
AlterSeqNamespaces(Relation classRel, Relation rel,
! Oid oldNspOid, Oid newNspOid, const char *newNspName, LOCKMODE lockmode)
{
Relation depRel;
SysScanDesc scan;
--- 9914,9921 ----
*/
static void
AlterSeqNamespaces(Relation classRel, Relation rel,
! Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
! LOCKMODE lockmode)
{
Relation depRel;
SysScanDesc scan;
***************
*** 9913,9926 **** AlterSeqNamespaces(Relation classRel, Relation rel,
/* Fix the pg_class and pg_depend entries */
AlterRelationNamespaceInternal(classRel, depForm->objid,
oldNspOid, newNspOid,
! true);
/*
* Sequences have entries in pg_type. We need to be careful to move
* them to the new namespace, too.
*/
AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
! newNspOid, false, false);
/* Now we can close it. Keep the lock till end of transaction. */
relation_close(seqRel, NoLock);
--- 9967,9980 ----
/* Fix the pg_class and pg_depend entries */
AlterRelationNamespaceInternal(classRel, depForm->objid,
oldNspOid, newNspOid,
! true, objsMoved);
/*
* Sequences have entries in pg_type. We need to be careful to move
* them to the new namespace, too.
*/
AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
! newNspOid, false, false, objsMoved);
/* Now we can close it. Keep the lock till end of transaction. */
relation_close(seqRel, NoLock);
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
***************
*** 3360,3366 **** AlterTypeNamespace_oid(Oid typeOid, Oid nspOid)
format_type_be(elemOid))));
/* and do the work */
! return AlterTypeNamespaceInternal(typeOid, nspOid, false, true);
}
/*
--- 3360,3366 ----
format_type_be(elemOid))));
/* and do the work */
! return AlterTypeNamespaceInternal(typeOid, nspOid, false, true, NULL);
}
/*
***************
*** 3381,3387 **** AlterTypeNamespace_oid(Oid typeOid, Oid nspOid)
Oid
AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
bool isImplicitArray,
! bool errorOnTableType)
{
Relation rel;
HeapTuple tup;
--- 3381,3388 ----
Oid
AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
bool isImplicitArray,
! bool errorOnTableType,
! ObjectAddresses *objsMoved)
{
Relation rel;
HeapTuple tup;
***************
*** 3389,3394 **** AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
--- 3390,3416 ----
Oid oldNspOid;
Oid arrayOid;
bool isCompositeType;
+ ObjectAddress thisobj;
+
+ if (objsMoved)
+ {
+ /*
+ * When doing ALTER EXTENSION SET SCHEMA, we follow dependencies, and
+ * more than one path leads to types. Check that we didn't cross this
+ * one previously.
+ */
+ thisobj.classId = TypeRelationId;
+ thisobj.objectId = typeOid;
+ thisobj.objectSubId = 0;
+
+ if( object_address_present(&thisobj, objsMoved))
+ /*
+ * We don't have the old namespace OID arround as soon as
+ * CommandCounterIncrement() has been done, which we have no way to
+ * know about from here.
+ */
+ return InvalidOid;
+ }
rel = heap_open(TypeRelationId, RowExclusiveLock);
***************
*** 3449,3455 **** AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
AlterRelationNamespaceInternal(classRel, typform->typrelid,
oldNspOid, nspOid,
! false);
heap_close(classRel, RowExclusiveLock);
--- 3471,3477 ----
AlterRelationNamespaceInternal(classRel, typform->typrelid,
oldNspOid, nspOid,
! false, objsMoved);
heap_close(classRel, RowExclusiveLock);
***************
*** 3482,3490 **** AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
heap_close(rel, RowExclusiveLock);
/* Recursively alter the associated array type, if any */
if (OidIsValid(arrayOid))
! AlterTypeNamespaceInternal(arrayOid, nspOid, true, true);
return oldNspOid;
}
--- 3504,3515 ----
heap_close(rel, RowExclusiveLock);
+ if (objsMoved)
+ add_exact_object_address(&thisobj, objsMoved);
+
/* Recursively alter the associated array type, if any */
if (OidIsValid(arrayOid))
! AlterTypeNamespaceInternal(arrayOid, nspOid, true, true, objsMoved);
return oldNspOid;
}
*** a/src/include/commands/alter.h
--- b/src/include/commands/alter.h
***************
*** 14,25 ****
#ifndef ALTER_H
#define ALTER_H
#include "utils/acl.h"
#include "utils/relcache.h"
extern void ExecRenameStmt(RenameStmt *stmt);
extern void ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt);
! extern Oid AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid);
extern Oid AlterObjectNamespace(Relation rel, int oidCacheId, int nameCacheId,
Oid objid, Oid nspOid,
int Anum_name, int Anum_namespace, int Anum_owner,
--- 14,27 ----
#ifndef ALTER_H
#define ALTER_H
+ #include "catalog/dependency.h"
#include "utils/acl.h"
#include "utils/relcache.h"
extern void ExecRenameStmt(RenameStmt *stmt);
extern void ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt);
! extern Oid AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
! ObjectAddresses *objsMoved);
extern Oid AlterObjectNamespace(Relation rel, int oidCacheId, int nameCacheId,
Oid objid, Oid nspOid,
int Anum_name, int Anum_namespace, int Anum_owner,
*** a/src/include/commands/tablecmds.h
--- b/src/include/commands/tablecmds.h
***************
*** 15,20 ****
--- 15,21 ----
#define TABLECMDS_H
#include "access/htup.h"
+ #include "catalog/dependency.h"
#include "nodes/parsenodes.h"
#include "storage/lock.h"
#include "utils/relcache.h"
***************
*** 36,44 **** extern void AlterTableInternal(Oid relid, List *cmds, bool recurse);
extern void AlterTableNamespace(AlterObjectSchemaStmt *stmt);
extern void AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
Oid oldNspOid, Oid newNspOid,
! bool hasDependEntry);
extern void CheckTableNotInUse(Relation rel, const char *stmt);
--- 37,49 ----
extern void AlterTableNamespace(AlterObjectSchemaStmt *stmt);
+ extern void AlterTableNamespaceInternal(Relation rel, Oid oldNspOid,
+ Oid nspOid, ObjectAddresses *objsMoved);
+
extern void AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
Oid oldNspOid, Oid newNspOid,
! bool hasDependEntry,
! ObjectAddresses *objsMoved);
extern void CheckTableNotInUse(Relation rel, const char *stmt);
*** a/src/include/commands/typecmds.h
--- b/src/include/commands/typecmds.h
***************
*** 15,20 ****
--- 15,21 ----
#define TYPECMDS_H
#include "access/htup.h"
+ #include "catalog/dependency.h"
#include "nodes/parsenodes.h"
***************
*** 47,53 **** extern void AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId,
extern void AlterTypeNamespace(List *names, const char *newschema, ObjectType objecttype);
extern Oid AlterTypeNamespace_oid(Oid typeOid, Oid nspOid);
extern Oid AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
! bool isImplicitArray,
! bool errorOnTableType);
#endif /* TYPECMDS_H */
--- 48,55 ----
extern void AlterTypeNamespace(List *names, const char *newschema, ObjectType objecttype);
extern Oid AlterTypeNamespace_oid(Oid typeOid, Oid nspOid);
extern Oid AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
! bool isImplicitArray,
! bool errorOnTableType,
! ObjectAddresses *objsMoved);
#endif /* TYPECMDS_H */
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
