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

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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Jul 6, 2011 at 1:09 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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

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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Jul 6, 2011 at 5:06 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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

Robert, Thanks for your reviewing.

2011/7/6 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jun 28, 2011 at 4:17 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Fri, Jul 8, 2011 at 9:06 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/7/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Jul 8, 2011 at 9:06 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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.