Re: [HACKERS] DROP FUNCTION of multiple functions

[Peter Eisentraut]

On 2/27/17 01:46, Michael Paquier wrote:
> On Sat, Feb 25, 2017 at 10:27 PM, Peter Eisentraut
>  wrote:
>> Here is a new patch set that addresses your comments.  The structure is
>> still the same, just a bunch of things have been renamed based on
>> suggestions.
> +  
> +   Drop multiple functions in one command:
> +
> +DROP FUNCTION sqrt(integer), sqrt(bigint);
> +
>   
> Perhaps adding as well on the page of DROP OPERATOR an example
> dropping multiple operators at once?

Committed with additional documentation.  Thanks.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Michael Paquier]

On Sat, Feb 25, 2017 at 10:27 PM, Peter Eisentraut
 wrote:
> Here is a new patch set that addresses your comments.  The structure is
> still the same, just a bunch of things have been renamed based on
> suggestions.

+  
+   Drop multiple functions in one command:
+
+DROP FUNCTION sqrt(integer), sqrt(bigint);
+
  
Perhaps adding as well on the page of DROP OPERATOR an example
dropping multiple operators at once?

-DropPolicyStmt:
-   DROP POLICY name ON any_name opt_drop_behavior
-   {
Oh, nice. I can see that you have decided to bit the bullet. Thanks
for considering that.

I am marking this patch as ready for committer, aka you.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Michael Paquier]

On Wed, Jan 18, 2017 at 2:00 PM, Michael Paquier
 wrote:
> On Wed, Jan 18, 2017 at 5:26 AM, Peter Eisentraut
>  wrote:
>> On 1/10/17 1:52 AM, Michael Paquier wrote:
>>> I don't see any problems with 0001.
>>
>> I was wondering, should we rename funcname -> name, and funcargs ->
>> args, or perhaps the whole FuncWithArgs struct, so there is no confusion
>> when used with operators?
>
> FuncWithArgs implies that this is related to a function, so removing
> func as prefix may make things cleaner.
>
>>> One comment though: there are still many list_make2() or even
>>> list_make3 calls for some object types. Would it make sense to replace
>>> those lists with a decided number of items by a Node and simplify the
>>> interface?
>>
>> (I don't see any list_make3.)
>
> Indeed, I am watching too much code.
>
>> It would be nice to refine this further,
>> but the remaining uses are quite marginal.  The main problem was that
>> before you had to create singleton lists and then unpack them, because
>> there was no other way.  The remaining uses are more genuine lists or lcons.
>
> OK. Of course, I am not saying that this patch in particular should
> shake more the world. I have been just trying to point out future
> potential improvements and keep a trace of them in the archives while
> thinking about it.
>
>>> In 0005, a nit:
>>> +DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
>>> functest_IS_3(int);
>>>  -- Cleanups
>>> The DROP query could be moved below the cleanup comment.
>>
>> I can do that, but the idea was that the commands below the cleanups
>> line weren't really tests.
>
> That's a nit, you can ignore that.
>
>>> While looking at 0006... DROP POLICY and DROP RULE could be unified. I
>>> just noticed that while reading the code.
>>
>> DROP TRIGGER also looks similar.  drop_type3 then. ;-)
>
> Or drop_type_on, drop_type_on_table, etc.

I am marking the patch as returned with feedback, as this thread has
died 2 weeks ago.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Michael Paquier]

On Wed, Jan 18, 2017 at 5:26 AM, Peter Eisentraut
 wrote:
> On 1/10/17 1:52 AM, Michael Paquier wrote:
>> I don't see any problems with 0001.
>
> I was wondering, should we rename funcname -> name, and funcargs ->
> args, or perhaps the whole FuncWithArgs struct, so there is no confusion
> when used with operators?

FuncWithArgs implies that this is related to a function, so removing
func as prefix may make things cleaner.

>> One comment though: there are still many list_make2() or even
>> list_make3 calls for some object types. Would it make sense to replace
>> those lists with a decided number of items by a Node and simplify the
>> interface?
>
> (I don't see any list_make3.)

Indeed, I am watching too much code.

> It would be nice to refine this further,
> but the remaining uses are quite marginal.  The main problem was that
> before you had to create singleton lists and then unpack them, because
> there was no other way.  The remaining uses are more genuine lists or lcons.

OK. Of course, I am not saying that this patch in particular should
shake more the world. I have been just trying to point out future
potential improvements and keep a trace of them in the archives while
thinking about it.

>> In 0005, a nit:
>> +DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
>> functest_IS_3(int);
>>  -- Cleanups
>> The DROP query could be moved below the cleanup comment.
>
> I can do that, but the idea was that the commands below the cleanups
> line weren't really tests.

That's a nit, you can ignore that.

>> While looking at 0006... DROP POLICY and DROP RULE could be unified. I
>> just noticed that while reading the code.
>
> DROP TRIGGER also looks similar.  drop_type3 then. ;-)

Or drop_type_on, drop_type_on_table, etc.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Peter Eisentraut]

On 1/10/17 1:52 AM, Michael Paquier wrote:
> I don't see any problems with 0001.

I was wondering, should we rename funcname -> name, and funcargs ->
args, or perhaps the whole FuncWithArgs struct, so there is no confusion
when used with operators?

 In 0002, the comment of
> class_args/CreateOpClassItem in parsenodes.h needs to be updated,
> because this becomes used by operators as well.

I think that aspect might be a mistake in my patch.  I'll check this
further, but I think the comment ought to remain true.

> One comment though: there are still many list_make2() or even
> list_make3 calls for some object types. Would it make sense to replace
> those lists with a decided number of items by a Node and simplify the
> interface?

(I don't see any list_make3.)  It would be nice to refine this further,
but the remaining uses are quite marginal.  The main problem was that
before you had to create singleton lists and then unpack them, because
there was no other way.  The remaining uses are more genuine lists or lcons.

> drop_type1 and drop_type2 are not really explicit, especially after
> reading that one allows schema-qualified names, not the other. Could
> you use something like drop_type_qualified? The same applies to
> comment_type and security_label.

Yeah, I'll try to find better names.

> 0004 could be merged with 0002, no? That looks good to me.

I think 0001 through 0004 would be committed together.  The split is
just for review.

> In 0005, a nit:
> +DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
> functest_IS_3(int);
>  -- Cleanups
> The DROP query could be moved below the cleanup comment.

I can do that, but the idea was that the commands below the cleanups
line weren't really tests.

> It may be a good idea to add an example of query dropping two
> functions at once in the docs.

Yes.

> Could you add some regression tests for the drop of aggregates and
> operators to stress the parsing code path?

Yes.

> While looking at 0006... DROP POLICY and DROP RULE could be unified. I
> just noticed that while reading the code.

DROP TRIGGER also looks similar.  drop_type3 then. ;-)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Michael Paquier]

On Sun, Jan 1, 2017 at 1:17 AM, Peter Eisentraut
 wrote:
> On 12/1/16 9:32 PM, Peter Eisentraut wrote:
>> I think it would be better to get rid of objargs and have objname be a
>> general Node that can contain more specific node types so that there is
>> some amount of type tracking.  FuncWithArgs would be one such type,
>> Typename would be another, Value would be used for simple strings, and
>> we could create some other ones, or stick with lcons for some simple
>> cases.  But then we don't have to make stuff into one-item lists to just
>> to satisfy the currently required List.
>>
>> That's the general idea.  But that's a rather big change that I would
>> rather break down into smaller pieces.
>
> People wanted to the whole thing at once, so here it is.
>
> Patches 0001 and 0002 are some prep work.  Patch 0003 is the main patch
> that removes the objargs fields and changes the objname to a generic
> Node*.  0004 is an API simplification that could be committed together
> with 0003 but I kept it separate for easier review.  0005 accomplishes
> the DROP FUNCTION changes.  0006 is some other cleanup that fell out of
> this.

I don't see any problems with 0001. In 0002, the comment of
class_args/CreateOpClassItem in parsenodes.h needs to be updated,
because this becomes used by operators as well.

Regarding 0003, which is large, but actually quite simple... The
approach makes sense. All the checks on the object formats are moved
out of the static routines in objectaddress.c and moved into a single
place.

+   if (list_length(name) != 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("name list length must be exactly %d", 1)));
+   objnode = linitial(name);
Those are the sanity checks for each object are moved to
pg_get_object_address(). This has as result some loss of verbosity in
the error messages. As the object type is passed by the user when
calling the SQL-callable interface, this new way looks fine to me.

One comment though: there are still many list_make2() or even
list_make3 calls for some object types. Would it make sense to replace
those lists with a decided number of items by a Node and simplify the
interface? Using a Node instead of a list would save some cycles. OK
that's really few though as that's basically comparing a list lookup
with a direct pointer lookup.

+   | DROP drop_type1 any_name_list opt_drop_behavior
+   {
+   DropStmt *n = makeNode(DropStmt);
+   n->removeType = $2;
+   n->missing_ok = FALSE;
+   n->objects = $3;
+   n->behavior = $4;
+   n->concurrent = false;
+   $$ = (Node *)n;
+   }
+   | DROP drop_type2 IF_P EXISTS name_list opt_drop_behavior
+   {
drop_type1 and drop_type2 are not really explicit, especially after
reading that one allows schema-qualified names, not the other. Could
you use something like drop_type_qualified? The same applies to
comment_type and security_label.

0004 could be merged with 0002, no? That looks good to me.

In 0005, a nit:
+DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
functest_IS_3(int);
 -- Cleanups
The DROP query could be moved below the cleanup comment.

It may be a good idea to add an example of query dropping two
functions at once in the docs.

Could you add some regression tests for the drop of aggregates and
operators to stress the parsing code path?

While looking at 0006... DROP POLICY and DROP RULE could be unified. I
just noticed that while reading the code.

This patch series look in rather good shape to me at the end.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Jim Nasby]

Forwarding some comments I neglected to send to the list...

On 1/3/17 9:16 AM, Peter Eisentraut wrote:
> On 1/2/17 1:04 PM, Jim Nasby wrote:

On 12/31/16 10:17 AM, Peter Eisentraut wrote:

--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -80,9 +80,6 @@ create event trigger regress_event_trigger2 on
ddl_command_start
execute procedure test_event_trigger();
 -- OK
 comment on event trigger regress_event_trigger is 'test comment';
--- should fail, event triggers are not schema objects
-comment on event trigger wrong.regress_event_trigger is 'test comment';
-ERROR:  event trigger name cannot be qualified
 -- drop as non-superuser should fail

I'm guessing that's a spurious change...


>> Well, the test is no longer useful as it was before, because the parser
>> just rejects the command.
>
> Ah. I would have just left it in to make sure a sane error was still
> generated, so I was wondering if something else was going on.


+++ b/src/test/regress/expected/object_address.out
@@ -287,7 +287,7 @@ WARNING:  error for function of access
method,{eins,zwei,drei},{integer}: argume
 SELECT pg_get_object_address('language', '{one}', '{}');
 ERROR:  language "one" does not exist
 SELECT pg_get_object_address('language', '{one,two}', '{}');
-ERROR:  language name cannot be qualified
+ERROR:  name list length must be exactly 1

Arguably, in this usage, that should be array length, not list length.
IIRC there's a function that will map an object type to it's name, so it
wouldn't be hard to restore the previous error message if you wanted to
go that route. pg_get_object_address() has to do a bunch of similar
error checks too, maybe there's a way to combine this stuff.

It's somewhat unfortunate that
pg_get_object_address()/pg_identify_object_as_address() won't match
these changes, but I don't see any good way to fix that.

BTW, it would also be damn nice if there was some form of wildcard
available for DROP FUNCTION (and maybe ALTER). If you've created several
overloaded versions of a function, making sure you change all of them
can be a PITA.

>> Yeah, that's the next patch after this. ;-)
>
> Yay :)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Robert Haas]

On Sat, Dec 31, 2016 at 11:17 AM, Peter Eisentraut
 wrote:
> On 12/1/16 9:32 PM, Peter Eisentraut wrote:
>> I think it would be better to get rid of objargs and have objname be a
>> general Node that can contain more specific node types so that there is
>> some amount of type tracking.  FuncWithArgs would be one such type,
>> Typename would be another, Value would be used for simple strings, and
>> we could create some other ones, or stick with lcons for some simple
>> cases.  But then we don't have to make stuff into one-item lists to just
>> to satisfy the currently required List.
>>
>> That's the general idea.  But that's a rather big change that I would
>> rather break down into smaller pieces.
>
> People wanted to the whole thing at once, so here it is.
>
> Patches 0001 and 0002 are some prep work.  Patch 0003 is the main patch
> that removes the objargs fields and changes the objname to a generic
> Node*.  0004 is an API simplification that could be committed together
> with 0003 but I kept it separate for easier review.  0005 accomplishes
> the DROP FUNCTION changes.  0006 is some other cleanup that fell out of
> this.

I haven't reviewed this in any detail but I think the general approach
is sensible.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Robert Haas]

On Thu, Dec 1, 2016 at 9:32 PM, Peter Eisentraut
 wrote:
> I think it would be better to get rid of objargs and have objname be a
> general Node that can contain more specific node types so that there is
> some amount of type tracking.  FuncWithArgs would be one such type,
> Typename would be another, Value would be used for simple strings, and
> we could create some other ones, or stick with lcons for some simple
> cases.  But then we don't have to make stuff into one-item lists to just
> to satisfy the currently required List.
>
> That's the general idea.  But that's a rather big change that I would
> rather break down into smaller pieces.  I have a separate patch in
> progress for that, which I have attached here.  It breaks some
> regression tests in object_address.sql, which I haven't evaluated yet,
> but that's the idea.

I think I disagree with this concept. I wouldn't shed many tears if
objname and objargs got replaced with some other kind of name
representation.  But I don't think that should be done incrementally,
because then we'll be in this transitional zone where it's unclear
what the right way to do things is for a long time, possibly forever.
I'd be fine with a plan to rip out objname/objargs and replace it with
something less arbitrary, but until that's done I think new code
should stick with the existing representations.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 1:32 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 11/23/16 5:04 PM, Tom Lane wrote:
> > I looked at this briefly.  I agree that 0001-0003 are simple cleanup of
> > the grammar and could be pushed without further ado.
>
> Done.
>
> > However, starting
> > with 0004 I begin to get queasy.  The plan seems to be that instead of
> > "objname" always being a List of strings, for functions (and then
> > aggregates and operators) it will be a one-element List of some new
> struct
> > that has then got a name list inside it.
>
> I think the original idea of representing an object by a list of name
> components plus optionally a list of arguments has accumulated too many
> warts and should be replaced.  For example: A Typename isn't a list, so
> it has to be packed into a List to be able to pass it around.  Some
> objects only have a single-component string as a name.  For a cast, we
> arbitrarily put the source type into a the name list and the target type
> into the argument list.  For an operator class on the other hand we
> create a cons of name and access method.  The pending logical
> replication patch has more such arbitrary examples.  This pattern has to
> be repeated consistently in gram.y for all cases where the object is
> referenced (ALTER EXTENSION, DROP, COMMENT, RENAME, SET SCHEMA, OWNER
> TO) and then consistently unpacked in objectaddress.c.
>
> I think it would be better to get rid of objargs and have objname be a
> general Node that can contain more specific node types so that there is
> some amount of type tracking.  FuncWithArgs would be one such type,
> Typename would be another, Value would be used for simple strings, and
> we could create some other ones, or stick with lcons for some simple
> cases.  But then we don't have to make stuff into one-item lists to just
> to satisfy the currently required List.
>
> That's the general idea.  But that's a rather big change that I would
> rather break down into smaller pieces.  I have a separate patch in
> progress for that, which I have attached here.  It breaks some
> regression tests in object_address.sql, which I haven't evaluated yet,
> but that's the idea.
>
> However, in these current patches I just wanted to take the first step
> to normalize the representation of functions so that actual
> functionality changes could be built in top of that.
>
> > It leads to code with random changes of data representation at seemingly
> > random places, like this bit from 0005:
> >
> > + if (stmt->removeType == OBJECT_AGGREGATE ||
> stmt->removeType == OBJECT_FUNCTION)
> > + objname = list_make1(objname);
>
> Yeah, the reason for that is that when we want to store something as
> objname, it needs to be a list, and the convention is to wrap non-lists
> into a single-member list.  So then objectaddress.c is coded to linitial
> such lists when working on object types such as functions or types.
> RemoveObjects() takes the list it gets from the grammar and passes each
> element to get_object_address().  But a function_with_argtypes_list is a
> list of FuncWithArgs, so we have to make those into single-member lists
> first.  The reason this works for types is that type_name_list looks
> like this:
>
> type_name_list:
> Typename   { $$ = list_make1(list_make1($1)); }
>   | type_name_list ',' Typename{ $$ = lappend($1, list_make1($3)); }
>
> I suppose we could make function_with_argtypes look like that as well
> (and later change it back if we redesign it as discussed above).  I
> think if we did it that way, it would get rid of the warts in this patch
> set.


Moved to next CF with "needs review" status.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] DROP FUNCTION of multiple functions

[Peter Eisentraut]

On 11/23/16 5:04 PM, Tom Lane wrote:
> I looked at this briefly.  I agree that 0001-0003 are simple cleanup of
> the grammar and could be pushed without further ado.

Done.

> However, starting
> with 0004 I begin to get queasy.  The plan seems to be that instead of
> "objname" always being a List of strings, for functions (and then
> aggregates and operators) it will be a one-element List of some new struct
> that has then got a name list inside it.

I think the original idea of representing an object by a list of name
components plus optionally a list of arguments has accumulated too many
warts and should be replaced.  For example: A Typename isn't a list, so
it has to be packed into a List to be able to pass it around.  Some
objects only have a single-component string as a name.  For a cast, we
arbitrarily put the source type into a the name list and the target type
into the argument list.  For an operator class on the other hand we
create a cons of name and access method.  The pending logical
replication patch has more such arbitrary examples.  This pattern has to
be repeated consistently in gram.y for all cases where the object is
referenced (ALTER EXTENSION, DROP, COMMENT, RENAME, SET SCHEMA, OWNER
TO) and then consistently unpacked in objectaddress.c.

I think it would be better to get rid of objargs and have objname be a
general Node that can contain more specific node types so that there is
some amount of type tracking.  FuncWithArgs would be one such type,
Typename would be another, Value would be used for simple strings, and
we could create some other ones, or stick with lcons for some simple
cases.  But then we don't have to make stuff into one-item lists to just
to satisfy the currently required List.

That's the general idea.  But that's a rather big change that I would
rather break down into smaller pieces.  I have a separate patch in
progress for that, which I have attached here.  It breaks some
regression tests in object_address.sql, which I haven't evaluated yet,
but that's the idea.

However, in these current patches I just wanted to take the first step
to normalize the representation of functions so that actual
functionality changes could be built in top of that.

> It leads to code with random changes of data representation at seemingly
> random places, like this bit from 0005:
>  
> + if (stmt->removeType == OBJECT_AGGREGATE || stmt->removeType == 
> OBJECT_FUNCTION)
> + objname = list_make1(objname);

Yeah, the reason for that is that when we want to store something as
objname, it needs to be a list, and the convention is to wrap non-lists
into a single-member list.  So then objectaddress.c is coded to linitial
such lists when working on object types such as functions or types.
RemoveObjects() takes the list it gets from the grammar and passes each
element to get_object_address().  But a function_with_argtypes_list is a
list of FuncWithArgs, so we have to make those into single-member lists
first.  The reason this works for types is that type_name_list looks
like this:

type_name_list:
Typename   { $$ = list_make1(list_make1($1)); }
  | type_name_list ',' Typename{ $$ = lappend($1, list_make1($3)); }

I suppose we could make function_with_argtypes look like that as well
(and later change it back if we redesign it as discussed above).  I
think if we did it that way, it would get rid of the warts in this patch
set.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 3af0716745895bfe24b5e2e7ab1f0d4aaa3ec011 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 12 Nov 2016 12:00:00 -0500
Subject: [PATCH] Remove objargs

WIP
---
 src/backend/catalog/objectaddress.c |  92 +---
 src/backend/commands/alter.c|   9 ++-
 src/backend/commands/comment.c  |   4 +-
 src/backend/commands/dropcmds.c |  50 ++---
 src/backend/commands/extension.c|   4 +-
 src/backend/commands/seclabel.c |   4 +-
 src/backend/commands/tablecmds.c|   1 -
 src/backend/nodes/copyfuncs.c   |   8 ---
 src/backend/nodes/equalfuncs.c  |   8 ---
 src/backend/parser/gram.y   | 136 
 src/backend/parser/parse_utilcmd.c  |   2 -
 src/include/catalog/objectaddress.h |   6 +-
 src/include/nodes/parsenodes.h  |   8 ---
 13 files changed, 117 insertions(+), 215 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d531d17..58d04fa 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -684,11 +684,11 @@ static ObjectAddress get_object_address_type(ObjectType objtype,
 static ObjectAddress get_object_address_opcf(ObjectType objtype, List *objname,
 		bool missing_ok);
 static ObjectAddress get_object_address_opf_member(ObjectType objtype,
-			  List *objname, 

Re: [HACKERS] DROP FUNCTION of multiple functions

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> Here is a patch series that implements several changes in the internal
>> grammar and node representation of function signatures.  They are not
>> necessarily meant to be applied separately, but they explain the
>> progression of the changes nicely, so I left them like that for review.

> I think patches 1-4 and 6 are pretty clearly straight cleanup and it
> seems fine to me to push them ahead of the rest.  I didn't review super
> carefully but it looks good on a quick glance.  I'd just advise to pay
> special attention to the objectaddress.sql test and the UI involved
> there (which you appear to have done already).

I looked at this briefly.  I agree that 0001-0003 are simple cleanup of
the grammar and could be pushed without further ado.  However, starting
with 0004 I begin to get queasy.  The plan seems to be that instead of
"objname" always being a List of strings, for functions (and then
aggregates and operators) it will be a one-element List of some new struct
that has then got a name list inside it.  This seems seriously bug-prone.
It leads to code with random changes of data representation at seemingly
random places, like this bit from 0005:
 
+   if (stmt->removeType == OBJECT_AGGREGATE || stmt->removeType == 
OBJECT_FUNCTION)
+   objname = list_make1(objname);

I've got no confidence that you've found all the places that will be
broken by that, nor that it won't introduce new bugs later.

Also, it seems like the end goal ought to involve getting rid of the
separate objarg/objargs fields in places like RenameStmt, as well
as the separate objargs argument to get_object_address and friends,
but the patch mostly doesn't do that --- 0006 does change
CreateOpClassItem, and 0007 changes AlterOperatorStmt, but that seems
like just the tip of the iceberg.

I wonder whether it would be useful to invent an ObjectName node type

typedef struct ObjectName
{
NodeTag type;
List *names;   /* list of String, representing 
possibly-qualified name */
List *args;/* list of TypeName, if needed for object type */
} ObjectName;

and replace all the places that currently have separate objname/objargs
fields or arguments with a uniform "ObjectName *" field/argument.  (This
node type could replace FuncWithArgs, obviously.)  Although that would
be more invasive, you could be certain that you'd looked at every place
that's affected by the change of representation.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DROP FUNCTION of multiple functions

[Fabrízio de Royes Mello]

On Tue, Nov 1, 2016 at 2:55 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> Here is a patch series that implements several changes in the internal
> grammar and node representation of function signatures.  They are not
> necessarily meant to be applied separately, but they explain the
> progression of the changes nicely, so I left them like that for review.
>
> The end goal is to make some user-visible changes in DROP FUNCTION and
> possibly other commands that refer to functions.
>
> With these patches, it is now possible to use DROP FUNCTION to drop
> multiple functions at once: DROP FUNCTION func1(), func2(), func3().
> Other DROP commands already supported that, but DROP FUNCTION didn't
> because the internal representation was complicated and couldn't handle
it.
>
> The next step after this would be to allow referring to functions
> without having to supply the arguments, if the name is unique.  This is
> an SQL-standard feature and would be very useful for dealing "business
> logic" functions with 10+ arguments.  The details of that are to be
> worked out, but with the help of the present changes, this would be a
> quite localized change, because the grammar representation is well
> encapsulated.
>

Really nice... just a little about 006, can't we reduce the code bellow?

@@ -823,8 +823,7 @@ get_object_address(ObjectType objtype, List *objname,
List *objargs,
 {
 FuncWithArgs *fwa = (FuncWithArgs *) linitial(objname);
 address.classId = ProcedureRelationId;
-address.objectId =
-LookupAggNameTypeNames(fwa->funcname,
fwa->funcargs, missing_ok);
+address.objectId = LookupAggWithArgs(fwa, missing_ok);
 address.objectSubId = 0;
 break;
 }
@@ -832,8 +831,7 @@ get_object_address(ObjectType objtype, List *objname,
List *objargs,
 {
 FuncWithArgs *fwa = (FuncWithArgs *) linitial(objname);
 address.classId = ProcedureRelationId;
-address.objectId =
-LookupFuncNameTypeNames(fwa->funcname,
fwa->funcargs, missing_ok);
+address.objectId = LookupFuncWithArgs(fwa, missing_ok);
 address.objectSubId = 0;
 break;
 }

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] DROP FUNCTION of multiple functions

[Alvaro Herrera]

Peter Eisentraut wrote:
> Here is a patch series that implements several changes in the internal
> grammar and node representation of function signatures.  They are not
> necessarily meant to be applied separately, but they explain the
> progression of the changes nicely, so I left them like that for review.

I think patches 1-4 and 6 are pretty clearly straight cleanup and it
seems fine to me to push them ahead of the rest.  I didn't review super
carefully but it looks good on a quick glance.  I'd just advise to pay
special attention to the objectaddress.sql test and the UI involved
there (which you appear to have done already).

Patch 5 is obviously a new feature and I would put that one in a
separate commit from the first four plus six.  (I'm not clear why you
put 6 after 5 instead of before.)

I'm not at all sure about patch 7.  Maybe that one is okay, not sure,
but since it's bigger I didn't look at it.



-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers