Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt
От | Kohei KaiGai |
---|---|
Тема | Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt |
Дата | |
Msg-id | CADyhKSWwXv2A1ruYhB_nCoia4ja00MhTJXDc-A724hqOT60KWg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt (Robert Haas <robertmhaas@gmail.com>) |
Ответы |
Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt
|
Список | pgsql-hackers |
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>
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Peter GeogheganДата:
Сообщение: Re: Latch implementation that wakes on postmaster death on both win32 and Unix