Thread

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

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-07-08T13:06:30Z

Robert, Thanks for your reviewing.

2011/7/6 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Jun 28, 2011 at 4:17 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 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.
>
I definitely agree with this idea. It will enables to eliminate ugly switch
statements for error-messaging reasons.
This reworked DROP code also contains these switches, such as
notice_object_non_existent() or get_relation_address(). It will be
cleaned up with this table.

> 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.
>
OK, it was fixed.

> - dropcmds.c has a header comment saying dropcmd.c
>
Oops, thank you for this pointing out.

> - RemoveObjects() could use forboth() instead of inventing its own way
> to loop over two lists in parallel
>
forboth() is not available in this case, because DropStmt->arguments
shall be NIL for currently supported object types.
So, I revised the code as follows:

  ListCell   *cell1;
  ListCell   *cell2 = NULL;

  foreach(cell1, stmt->objects)
  {
      List       *objname = lfirst(cell1);
      List       *objargs = NIL;

      if (stmt->arguments)
      {
          cell2 = (!cell2 ? list_head(stmt->arguments) : lnext(cell2));
          objargs = lfirst(cell2);
      }

> - Why does RemoveObjects() need to call RelationForgetRelation()?
>
I thought here is a risk to reference a phantom entry of dropped relation
because of relation_open() in get_object_address(), however, doDeletion()
eventually called RelationForgetRelation().
So, it is not necessary here.

> - 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.
>
I put a new get_relation_address() in dropcmds.c as a special case in
deletion of relation, because of
 - Locks to the table owning the index to be removed
 - Error message issues
 - Sanity checks when we try to drop system catalogs.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>