Re: ALTER OBJECT any_name SET SCHEMA name

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Bernd Helmle <mailings(at)oopsware(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: ALTER OBJECT any_name SET SCHEMA name
Date: 2010-10-31 19:38:31
Message-ID: m2bp6ab1ag.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
...
> related indexes, sequences, and constraints. It seems hard to fit
> that into a general framework, but maybe it could be done for other
> object types.

My guess is that we're talking about having a generic code that would
get exercised directly from src/backend/commands/alter.c in the function
ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt), rather than
having a switch() statement and very similar functions implemented all
over the place to do the actual bit editing.

Let's see some details, if we're getting there.

This line of code is repeated in lots of places, but always a little different:

+ conversionOid = get_conversion_oid(name, false);

+ operOid = LookupOperNameTypeNames(NULL, operatorName,
+ typeName1, typeName2,
+ false, -1);

+ prsId = get_ts_parser_oid(name, false);

+ dictId = get_ts_dict_oid(name, false);

And for operator class and operator family, we're dealing with HeapTuple
objects directly, rather than Oids, because the lookup ain't so
easy. But all in all that code could get moved into another layer in the
generic function, in a object type switch, if we get to HeapTuple based
internal API, because next example shows that in most cases we get the
tuple from the Oid in the same way.

In the following code part, I can see how to get generic code with some
parameters for the first 3 lines, but not so much for the other 2 lines:

+ tup = SearchSysCacheCopy1(CONVOID, ObjectIdGetDatum(conversionOid));
+ if (!HeapTupleIsValid(tup)) /* should not happen */
+ elog(ERROR, "cache lookup failed for conversion %u", conversionOid);
+
+ convForm = (Form_pg_conversion) GETSTRUCT(tup);
+ oldNspOid = convForm->connamespace;

Then there's this code which looks even harder to get generic:

+ /* Otherwise, must be owner of the existing object */
+ if (!pg_conversion_ownercheck(HeapTupleGetOid(tup), GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CONVERSION,
+ NameStr(convForm->conname));

Now the following part could get moved easily to the CheckSetNamespace()
function introduced in the patch, with a boolean to trigger the check,
because as it is not all objects are open to non-superuser changes.

+ /* New owner must have CREATE privilege on namespace */
+ aclresult = pg_namespace_aclcheck(nspOid, GetUserId(), ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
+ get_namespace_name(nspOid));

Then in the end of the function, we have the real work, where I can see
a generic function for the heap update and the dependency tracking, but
not for the tuple editing itself:

+ /* tup is a copy, so we can scribble directly on it */
+ opfForm->opfnamespace = nspOid;
+
+ simple_heap_update(rel, &tup->t_self, tup);
+ CatalogUpdateIndexes(rel, tup);
+
+ /* update dependencies to point to the new schema */
+ changeDependencyFor(OperatorFamilyRelationId, HeapTupleGetOid(tup),
+ NamespaceRelationId, oldNspOid, nspOid);

So it seems to get down to C'fu related to handling Form_pg_* pointers
to get to values and change them then simple_heap_update the tuple. The
other parts are all about the same code with different RelationId and
such.

Well of course you think you could just have a pointer as argument, but
how to get the pointer is something we'd prefer generic too. Maybe we
need a 3-step code here, in aforementioned ExecAlterObjectSchemaStmt:

1. call a generic function that switch on stmt->objectType and
returns an HeapTuple

2. switch on stmt->objectType to get oldNspOid from the tuple and to
check for permissions

3. call another generic function to do the namespace related checks
then the heap update etc

This way the only specific code we need to maintain are the second step
here, object dependent, and in the first function too, but that's not an
entire API like in current patch.

Well of course there's the question of how to do that in a way that
allows some other backend's code to get the action done with Oids as
input rather than a statement, but that looks workable: get from classid
to objectype (aren't they equal?) then call the step 1. function to get
the HeapTyple, and proceed. So in fact the 3 steps are a separate
function that is called either from ExecAlterObjectSchemaStmt or from
the extension's code.

And there's also the question whether that has anything to do with what
Tom would want to see happening, too (or I'll be talking about those
ideas with gcc rather than with the list) :)

Comments?
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Dimitri Fontaine 2010-10-31 19:42:37 Re: ALTER OBJECT any_name SET SCHEMA name
Previous Message Heikki Linnakangas 2010-10-31 19:10:07 Re: ALTER OBJECT any_name SET SCHEMA name