Thread

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

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-06-14T12:33:16Z

   The attached patch can be applied on the part-0 patch, and enables to
   consolidate routines to handle DropStmt into one common function;
       void RemoveObjects(DropStmt *stmt);
   
   The top-half of object deletion steps are almost consist of the following steps.
   1) Look up object-Id of the supplied object name
      If not found, it raises an error or a notice with skip this deletion.
   2) Apply ownership checks on the target object itself or the schema
   that underlies
     the target object.
   3) Add the ObjectAddress of the target object into ObjectAddresses, then invokes
      performMultipleDeletions() or performDeletion().
   
   However, we don't need to have individual routines for each object classes,
   because get_object_address takes up the portion of (1), and
   check_object_ownership also takes up the portion of (2).
   Here is no differences between most of objects classes on the (3).
   
   So, the new RemoveObjects() consolidates routines to handle DropStmt for
   each object classes. Instead of this common function, the following routines
   were eliminated.
     RemoveRelations(DropStmt *drop);
     RemoveTypes(DropStmt *drop);
     DropCollationsCommand(DropStmt *drop);
     DropConversionsCommand(DropStmt *drop);
     RemoveSchemas(DropStmt *drop);
     RemoveTSParsers(DropStmt *drop);
     RemoveTSDictionaries(DropStmt *drop);
     RemoveTSTemplates(DropStmt *drop);
     RemoveTSConfigurations(DropStmt *drop);
     RemoveExtensions(DropStmt *drop);
   
   Routines to handle other DROP statement (such as RemoveFuncStmt or
   DropPLangStmt) are not scope of this patch to restrain the patch size.
   However, it is not a tough work to fit these objects with this structure.
   (It may need a discussion for databases, tablespaces and roles)
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

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

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-06-28T08:17:51Z

   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.
   
   Thanks,
   
   2011/6/14 Kohei KaiGai <kaigai@kaigai.gr.jp>:
   > The attached patch can be applied on the part-0 patch, and enables to
   > consolidate routines to handle DropStmt into one common function;
   >    void RemoveObjects(DropStmt *stmt);
   >
   > The top-half of object deletion steps are almost consist of the following steps.
   > 1) Look up object-Id of the supplied object name
   >   If not found, it raises an error or a notice with skip this deletion.
   > 2) Apply ownership checks on the target object itself or the schema
   > that underlies
   >  the target object.
   > 3) Add the ObjectAddress of the target object into ObjectAddresses, then invokes
   >   performMultipleDeletions() or performDeletion().
   >
   > However, we don't need to have individual routines for each object classes,
   > because get_object_address takes up the portion of (1), and
   > check_object_ownership also takes up the portion of (2).
   > Here is no differences between most of objects classes on the (3).
   >
   > So, the new RemoveObjects() consolidates routines to handle DropStmt for
   > each object classes. Instead of this common function, the following routines
   > were eliminated.
   >  RemoveRelations(DropStmt *drop);
   >  RemoveTypes(DropStmt *drop);
   >  DropCollationsCommand(DropStmt *drop);
   >  DropConversionsCommand(DropStmt *drop);
   >  RemoveSchemas(DropStmt *drop);
   >  RemoveTSParsers(DropStmt *drop);
   >  RemoveTSDictionaries(DropStmt *drop);
   >  RemoveTSTemplates(DropStmt *drop);
   >  RemoveTSConfigurations(DropStmt *drop);
   >  RemoveExtensions(DropStmt *drop);
   >
   > Routines to handle other DROP statement (such as RemoveFuncStmt or
   > DropPLangStmt) are not scope of this patch to restrain the patch size.
   > However, it is not a tough work to fit these objects with this structure.
   > (It may need a discussion for databases, tablespaces and roles)
   >
   > Thanks,
   > --
   > KaiGai Kohei <kaigai@kaigai.gr.jp>
   >
   
   
   
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

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

   Robert Haas <robertmhaas@gmail.com> — 2011-07-06T16:40:39Z

   On Tue, Jun 28, 2011 at 4:17 AM, Kohei KaiGai <kaigai@kaigai.gr.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
   
   

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

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-07-06T17:09:11Z

   Excerpts from Robert Haas's message of mié jul 06 12:40:39 -0400 2011:
   
   > 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.
   
   Yeah.  Myself, I love the fact that the dropmsgstrings thing is gone.  I
   wonder if the routine to obtain "foo doesn't exist, skipping" messages
   could be replaced by judicious use of getObjectDescription.
   
   >  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
   
   +1 for this general approach.
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

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

   Robert Haas <robertmhaas@gmail.com> — 2011-07-06T18:02:13Z

   On Wed, Jul 6, 2011 at 1:09 PM, Alvaro Herrera
   <alvherre@commandprompt.com> wrote:
   > Excerpts from Robert Haas's message of mié jul 06 12:40:39 -0400 2011:
   >
   >> 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.
   >
   > Yeah.  Myself, I love the fact that the dropmsgstrings thing is gone.  I
   > wonder if the routine to obtain "foo doesn't exist, skipping" messages
   > could be replaced by judicious use of getObjectDescription.
   
   I've been told we don't want to go further in that direction for
   reasons of translatability.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

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

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-07-06T21:06:03Z

   Excerpts from Robert Haas's message of mié jul 06 14:02:13 -0400 2011:
   > On Wed, Jul 6, 2011 at 1:09 PM, Alvaro Herrera
   > <alvherre@commandprompt.com> wrote:
   > > Excerpts from Robert Haas's message of mié jul 06 12:40:39 -0400 2011:
   > >
   > >> 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.
   > >
   > > Yeah.  Myself, I love the fact that the dropmsgstrings thing is gone.  I
   > > wonder if the routine to obtain "foo doesn't exist, skipping" messages
   > > could be replaced by judicious use of getObjectDescription.
   > 
   > I've been told we don't want to go further in that direction for
   > reasons of translatability.
   
   Well, you can split sentences using a colon instead of building them;
   something like
   	errmsg("object cannot be found, skipping: %s", getObjectDescription(object))
   which renders as
   	object cannot be found, skipping: table foobar
   
   Now people can complain that these messages are "worse" than the
   originals which were more specific in nature, but I don't personally see
   a problem with that.  I dunno what's the general opinion though.
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

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

   Robert Haas <robertmhaas@gmail.com> — 2011-07-07T00:37:09Z

   On Wed, Jul 6, 2011 at 5:06 PM, Alvaro Herrera
   <alvherre@commandprompt.com> wrote:
   > Excerpts from Robert Haas's message of mié jul 06 14:02:13 -0400 2011:
   >> On Wed, Jul 6, 2011 at 1:09 PM, Alvaro Herrera
   >> <alvherre@commandprompt.com> wrote:
   >> > Excerpts from Robert Haas's message of mié jul 06 12:40:39 -0400 2011:
   >> >
   >> >> 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.
   >> >
   >> > Yeah.  Myself, I love the fact that the dropmsgstrings thing is gone.  I
   >> > wonder if the routine to obtain "foo doesn't exist, skipping" messages
   >> > could be replaced by judicious use of getObjectDescription.
   >>
   >> I've been told we don't want to go further in that direction for
   >> reasons of translatability.
   >
   > Well, you can split sentences using a colon instead of building them;
   > something like
   >        errmsg("object cannot be found, skipping: %s", getObjectDescription(object))
   > which renders as
   >        object cannot be found, skipping: table foobar
   >
   > Now people can complain that these messages are "worse" than the
   > originals which were more specific in nature, but I don't personally see
   > a problem with that.  I dunno what's the general opinion though.
   
   I'm going to try to stay out of it, since I only use PostgreSQL in English...
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

8. 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>
   

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

   Robert Haas <robertmhaas@gmail.com> — 2011-07-09T02:15:55Z

   On Fri, Jul 8, 2011 at 9:06 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > I definitely agree with this idea. It will enables to eliminate ugly switch
   > statements for error-messaging reasons.
   
   All right, so please submit a patch that introduces that concept
   first, and then resubmit this patch rebased over those changes.
   
   In view of the time remaining in this CommitFest, I am going to mark
   this Returned with Feedback for now.  The next CommitFest starts
   September 15th, but I'll try to review it sooner as time permits.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

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

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-07-09T07:01:19Z

    2011/7/9 Robert Haas <robertmhaas@gmail.com>:
    > On Fri, Jul 8, 2011 at 9:06 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> I definitely agree with this idea. It will enables to eliminate ugly switch
    >> statements for error-messaging reasons.
    >
    > All right, so please submit a patch that introduces that concept
    > first, and then resubmit this patch rebased over those changes.
    >
    > In view of the time remaining in this CommitFest, I am going to mark
    > this Returned with Feedback for now.  The next CommitFest starts
    > September 15th, but I'll try to review it sooner as time permits.
    >
    OK, Thanks for your reviewing.
    
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    

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

    Peter Eisentraut <peter_e@gmx.net> — 2011-07-12T09:53:52Z

    On ons, 2011-07-06 at 12:40 -0400, Robert Haas wrote:
    > 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 
    
    Yeah, I was thinking of the same thing a while ago.
    
    For large parts of the DDL support for collations I literally copied
    over the conversion support and ran sed over it.  That must be made
    better.  Take that as a test case if you will.