Subject: Re: DROP FUNCTION of multiple functions
To: Michael Paquier <michael.paquier@gmail.com>
References: <6dcd1248-4631-a4c1-65d7-b873fbcada6c@2ndquadrant.com>
 <20161101151503.GA29685@eldon.alvh.no-ip.org> <1932.1479938694@sss.pgh.pa.us>
 <42459fde-dab9-dd27-7576-03a642b8c6c5@2ndquadrant.com>
 <44639202-2e20-78d4-71bf-e390b4d7bf13@2ndquadrant.com>
 <CAB7nPqREoQm+=BCkXegime-kLCFkS+03KQ9ncvU+4=TwbauFNw@mail.gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Organization: 2ndQuadrant
Message-ID: <20b7a398-61d5-6cde-c74c-7155950abe97@2ndquadrant.com>
Date: Tue, 17 Jan 2017 15:26:22 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.6.0
MIME-Version: 1.0
In-Reply-To: 
 <CAB7nPqREoQm+=BCkXegime-kLCFkS+03KQ9ncvU+4=TwbauFNw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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