Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt

On Tue, Jun 28, 2011 at 4:17 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is rebased one to consolidate routines to remove objects
> using the revised get_object_address().
>
> The new RemoveObjects() replaces the following routines; having
> similar structure.
>  - RemoveRelations
>  - RemoveTypes
>  - DropCollationsCommand
>  - DropConversionsCommand
>  - RemoveSchemas
>  - RemoveTSParsers
>  - RemoveTSDictionaries
>  - RemoveTSTemplates
>  - RemoveTSConfigurations
>  - RemoveExtensions
>
> I guess the most arguable part of this patch is modifications to
> get_relation_by_qualified_name().
>
> This patch breaks down the relation_openrv_extended() into
> a pair of RangeVarGetRelid() and relation_open() to inject
> LockRelationOid() between them, because index_drop() logic
> requires the table owning the target index to be locked prior to
> the index itself.

This patch removes an impressive amount of boilerplate code and
replaces it with something much more compact. I like that. In the
interest of full disclosure, I suggested this approach to KaiGai at
PGCon, so I'm biased: but even so, I'm pleasantly surprised by the
amount of consolidation that appears possible here.

I think that get_object_namespace() needs to be rethought. If you
take a look at AlterObjectNamespace() and its callers, you'll notice
that we already have, encoded in those call sites, the knowledge of
which object types can be looked up in which system caches, and which
columns within the resulting heap tuples will contain the schema OIDs.
Here, we're essentially duplicating that information in another
place, which doesn't seem good. I think perhaps we should create a
big static array, each row of which would contain:

- ObjectType
- system cache ID for OID lookups
- system catalog table OID for scans
- attribute number for the name attribute, where applicable (see
AlterObjectNamespace)
- attribute number for the namespace attribute
- attribute number for the owner attribute
- ...and maybe some other properties

We could then create a function (in objectaddress.c, probably) that
you call with an ObjectType argument, and it returns a pointer to the
appropriate struct entry, or calls elog() if none is found. This
function could be used by the existing object_exists() functions in
lieu of having its own giant switch statement, by
AlterObjectNamespace(), and by this new consolidated DROP code. If
you agree with this approach, we should probably make it a separate
patch.

Other minor things I noticed:

- get_object_namespace() itself does not need to take both an
ObjectType and an ObjectAddress - just an ObjectAddress should be
sufficient.

- dropcmds.c has a header comment saying dropcmd.c

- RemoveObjects() could use forboth() instead of inventing its own way
to loop over two lists in parallel

- Why does RemoveObjects() need to call RelationForgetRelation()?

- It might be cleaner, rather than hacking up
get_relation_by_qualified_name(), to just have special-purpose code
for dropping relations. it looks like you will need at least two
special cases for relations as opposed to other types of objects that
someone might want to drop, so it may make sense to keep them
separate. Remember, in any case, that we don't want to redefine
get_relation_by_qualified_name() for other callers, who don't have the
same needs as RemoveObjects(). This would also avoid things like
this:

-ERROR: view "atestv4" does not exist
+ERROR: relation "atestv4" does not exist

...which are probably not desirable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company