Tom Lane <t...@sss.pgh.pa.us> writes:
> I've been testing this patch with an extension having this definition
> file:
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 :)
> -----
> create table t1(f1 serial primary key, f2 text);
>
> create table t2(f1 int primary key, f2 text);
>
> create sequence ss2;
>
> alter sequence ss2 owned by t2.f1;
>
> create sequence ss3;
>
> create table t3(f1 int primary key, f2 text);
>
> alter sequence ss3 owned by t3.f1;
> -----
This exact same script pass with the attached patch, a patch on top
Álvaro's version:
dim=# create extension extseq;
CREATE EXTENSION
dim=# create schema foo;
CREATE SCHEMA
dim=# alter extension extseq set schema foo;
ALTER EXTENSION
dim=# \dx+ extseq
Objects in extension "extseq"
Object Description
------------------------
sequence foo.ss2
sequence foo.ss3
sequence foo.t1_f1_seq
table foo.t1
table foo.t2
table foo.t3
(6 rows)
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.
> nothing if the sequence was already moved. We could maybe refactor
> so that AlterRelationNamespaceInternal's test covers the type case too,
> but I don't think that is the way forward, because it won't cover any
> non-sequence cases where a type is reached through two different
> dependency paths.
I tried to care about that in the attached. Spent so much time rolling
it in my head in every possible angle that I really need another pair of
eyes on it though.
> So it appears to me that a real fix involves extending the check and
> add logic to at least relations and types, and perhaps eventually to
> everything that AlterObjectNamespace_oid can call. That's fairly
> invasive, especially if we try to do the whole nine yards immediately.
> But perhaps for now we need only fix the relation and type cases.
I think INDEX and CONSTRAINTS (the only other things that can be called
from that point) are safe because there's no explicit support for them
in the AlterObjectNamespace_oid() function.
> BTW, I experimented with inserting CommandCounterIncrement calls
> into the AlterExtensionNamespace loop, and eventually decided that
> that's probably not the best path to a solution. The killer problem is
> that it messes up the logic in AlterExtensionNamespace that tries to
> verify that all the objects had been in the same namespace. If the
> subroutines report that the object is now in the target namespace,
> is that okay or not? You can't tell.
Agreed.
> I think that the right way to proceed is to *not* do
> CommandCounterIncrement in the AlterExtensionNamespace loop, and also
> *not* have a test in AlterExtensionNamespace for an object already
> having been moved. Rather, since we already know we need that test down
> in AlterRelationNamespaceInternal and AlterTypeNamespaceInternal, do it
> only at that level. This combination of choices ensures that we'll get
> back valid reports of the old namespace for each object, and so the
> are-they-all-the-same logic in AlterExtensionNamespace still works.
See attached.
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,9801 ----
/* 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);
! 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))
--- 9806,9816 ----
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);
}
--- 9819,9865 ----
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)))
! {
! /* 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);
--- 9890,9896 ----
*/
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;
--- 9905,9912 ----
*/
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);
--- 9958,9971 ----
/* 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, NULL);
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
***************
*** 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 */
--- 47,54 ----
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
