Re: [HACKERS] Add more regression tests for CREATE OPERATOR
On 26 June 2013 02:26, Robins Tharakan thara...@gmail.com wrote:
So technically I hope this regression patch I submitted could go through
since this feedback isn't towards that patch, but in my part I am quite
intrigued about this test (and how it passes) and probably I'd get back on
this thread about this particular commented out test in question, as time
permits.
Attached is an updated (cumulative) patch, that takes care of the issue
mentioned above and tests two more cases that were skipped earlier.
--
Robins Tharakan
diff --git a/src/test/regress/expected/create_operator.out
b/src/test/regress/expected/create_operator.out
index 8656864..2e6c764 100644
--- a/src/test/regress/expected/create_operator.out
+++ b/src/test/regress/expected/create_operator.out
@@ -29,3 +29,145 @@ CREATE OPERATOR #%# (
-- Test comments
COMMENT ON OPERATOR ## (int4, NONE) IS 'bad right unary';
ERROR: operator does not exist: integer ##
+-- Show deprecated message. = is deprecated now
+CREATE OPERATOR = (
+ leftarg = int8, -- right unary
+ procedure = numeric_fac
+);
+WARNING: = is deprecated as an operator name
+DETAIL: This name may be disallowed altogether in future versions of
PostgreSQL.
+-- Should fail. CREATE OPERATOR requires USAGE on SCHEMA
+BEGIN TRANSACTION;
+CREATE ROLE regress_rol_op1;
+CREATE SCHEMA schema_op1;
+GRANT USAGE ON SCHEMA schema_op1 TO PUBLIC;
+REVOKE USAGE ON SCHEMA schema_op1 FROM regress_rol_op1;
+SET ROLE regress_rol_op1;
+CREATE OPERATOR schema_op1.#*# (
+ leftarg = int8, -- right unary
+ procedure = numeric_fac
+);
+ERROR: permission denied for schema schema_op1
+ROLLBACK;
+-- Should fail. SETOF type functions not allowed as argument (testing leftarg)
+BEGIN TRANSACTION;
+CREATE OPERATOR #*# (
+ leftarg = SETOF int8,
+ procedure = numeric_fac
+);
+ERROR: SETOF type not allowed for operator argument
+ROLLBACK;
+-- Should fail. SETOF type functions not allowed as argument (testing rightarg)
+BEGIN TRANSACTION;
+CREATE OPERATOR #*# (
+ rightarg = SETOF int8,
+ procedure = numeric_fac
+);
+ERROR: SETOF type not allowed for operator argument
+ROLLBACK;
+-- Should work. Sample text-book case
+BEGIN TRANSACTION;
+CREATE OR REPLACE FUNCTION fn_op2(boolean, boolean)
+RETURNS boolean AS $$
+SELECT NULL::BOOLEAN;
+$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR === (
+LEFTARG = boolean,
+RIGHTARG = boolean,
+PROCEDURE = fn_op2,
+COMMUTATOR = ===,
+NEGATOR = !==,
+RESTRICT = contsel,
+JOIN = contjoinsel,
+SORT1, SORT2, LTCMP, GTCMP, HASHES, MERGES
+);
+ROLLBACK;
+-- Should fail. Invalid attribute
+CREATE OPERATOR #@%# (
+ leftarg = int8, -- right unary
+ procedure = numeric_fac,
+ invalid_att = int8
+);
+WARNING: operator attribute invalid_att not recognized
+-- Should fail. At least leftarg or rightarg should be mandatorily specified
+CREATE OPERATOR #@%# (
+ procedure = numeric_fac
+);
+ERROR: at least one of leftarg or rightarg must be specified
+-- Should fail. Procedure should be mandatorily specified
+CREATE OPERATOR #@%# (
+ leftarg = int8
+);
+ERROR: operator procedure must be specified
+-- Should fail. CREATE OPERATOR requires USAGE on TYPE
+BEGIN TRANSACTION;
+CREATE ROLE regress_rol_op3;
+CREATE TYPE type_op3 AS ENUM ('new', 'open', 'closed');
+CREATE FUNCTION fn_op3(type_op3, int8)
+RETURNS int8 AS $$
+SELECT NULL::int8;
+$$ LANGUAGE sql IMMUTABLE;
+REVOKE USAGE ON TYPE type_op3 FROM regress_rol_op3;
+REVOKE USAGE ON TYPE type_op3 FROM PUBLIC; -- Need to do this so that
regress_rol_op3 is not allowed USAGE via PUBLIC
+SET ROLE regress_rol_op3;
+CREATE OPERATOR #*# (
+ leftarg = type_op3,
+ rightarg = int8,
+ procedure = fn_op3
+);
+ERROR: permission denied for type type_op3
+ROLLBACK;
+-- Should fail. CREATE OPERATOR requires USAGE on TYPE (need to check
separately for rightarg)
+BEGIN TRANSACTION;
+CREATE ROLE regress_rol_op4;
+CREATE TYPE type_op4 AS ENUM ('new', 'open', 'closed');
+CREATE FUNCTION fn_op4(int8, type_op4)
+RETURNS int8 AS $$
+SELECT NULL::int8;
+$$ LANGUAGE sql IMMUTABLE;
+REVOKE USAGE ON TYPE type_op4 FROM regress_rol_op4;
+REVOKE USAGE ON TYPE type_op4 FROM PUBLIC; -- Need to do this so that
regress_rol_op3 is not allowed USAGE via PUBLIC
+SET ROLE regress_rol_op4;
+CREATE OPERATOR #*# (
+ leftarg = int8,
+ rightarg = type_op4,
+ procedure = fn_op4
+);
+ERROR: permission denied for type type_op4
+ROLLBACK;
+-- Should fail. CREATE OPERATOR requires EXECUTE on function
+BEGIN TRANSACTION;
+CREATE ROLE regress_rol_op5;
+CREATE TYPE type_op5 AS ENUM ('new', 'open', 'closed');
+CREATE FUNCTION fn_op5(int8, int8)
+RETURNS int8 AS $$
+SELECT NULL::int8;
+$$ LANGUAGE sql IMMUTABLE;
+REVOKE EXECUTE ON FUNCTION fn_op5(int8, int8) FROM regress_rol_op5;
+REVOKE EXECUTE ON FUNCTION fn_op5(int8, int8) FROM PUBLIC;-- Need to do this
so that regress_rol_op3 is not allowed EXECUTE via PUBLIC
+SET ROLE regress_rol_op5;
+CREATE OPERATOR #*#
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
On 27 June 2013 09:00, Robert Haas robertmh...@gmail.com wrote: On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz mabew...@gmail.com wrote: OK, so I think this patch can be committed, I will change the status. We have a convention that roles created by the regression tests needs to have regress or something of the sort in the name, and that they need to be dropped by the regression tests. The idea is that if someone runs make installcheck against an installed server, it should pass - even if you run it twice in succession. And also, it shouldn't be likely to try to create (and then drop!) a role name that already exists. Setting this to Waiting on Author. Hi Robert, Attached is an updated patch that prepends 'regress' before role names. As for dropping ROLEs is concerned, all the roles created in the previous patch were within transactions. So didn't have to explicitly drop any ROLEs at the end of the script. -- Robins Tharakan regress_createoperator_v2.patch Description: Binary data -- 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] Add more regression tests for CREATE OPERATOR
On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz mabew...@gmail.com wrote: OK, so I think this patch can be committed, I will change the status. We have a convention that roles created by the regression tests needs to have regress or something of the sort in the name, and that they need to be dropped by the regression tests. The idea is that if someone runs make installcheck against an installed server, it should pass - even if you run it twice in succession. And also, it shouldn't be likely to try to create (and then drop!) a role name that already exists. Setting this to Waiting on Author. -- 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] Add more regression tests for CREATE OPERATOR
Sure Robert. I 'll update the tests and get back. Two questions, while we're at it: 1. Any other conventions (for naming)? 2. Should I assume that all database objects that get created, need to be dropped explicitly? Or is this point specifically about ROLES? -- Robins Tharakan On 27 June 2013 09:00, Robert Haas robertmh...@gmail.com wrote: On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz mabew...@gmail.com wrote: OK, so I think this patch can be committed, I will change the status. We have a convention that roles created by the regression tests needs to have regress or something of the sort in the name, and that they need to be dropped by the regression tests. The idea is that if someone runs make installcheck against an installed server, it should pass - even if you run it twice in succession. And also, it shouldn't be likely to try to create (and then drop!) a role name that already exists. Setting this to Waiting on Author. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
Robins Tharakan thara...@gmail.com writes: 2. Should I assume that all database objects that get created, need to be dropped explicitly? Or is this point specifically about ROLES? It's about any global objects (that wouldn't get dropped by dropping the regression database). As far as local objects go, there are benefits to leaving them around, particularly if they present interesting test cases for pg_dump/pg_restore. 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] Add more regression tests for CREATE OPERATOR
Hi Szymon, The commented out test that you're referring to, is an existing test (not that I added or commented). I was going to remove but interestingly its testing a part of code where (prima-facie) it should fail, but it passes (probably why it was disabled in the first place) ! So technically I hope this regression patch I submitted could go through since this feedback isn't towards that patch, but in my part I am quite intrigued about this test (and how it passes) and probably I'd get back on this thread about this particular commented out test in question, as time permits. -- Robins Tharakan On 25 June 2013 04:12, Robins Tharakan thara...@gmail.com wrote: Thanks a ton Szymon (for a reminder on this one). As a coincidental turn of events, I have had to travel half way across the world and am without my personal laptop (without a linux distro etc.) and just recovering from a jet-lag now. I'll try to install a VM on a make-shift laptop and get something going to respond as soon as is possible. Thanks -- Robins Tharakan -- Robins Tharakan On 17 June 2013 05:19, Szymon Guz mabew...@gmail.com wrote: On 23 May 2013 00:34, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of CREATE OPERATOR (src/backend/commands/operatorcmds.c) from 56% to 91%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, there is one commented out test. I think it should be run, or deleted. There is no use of commented sql code which is not run. What do you think? regards, Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
OK, so I think this patch can be committed, I will change the status. thanks, Szymon On 26 June 2013 09:26, Robins Tharakan thara...@gmail.com wrote: Hi Szymon, The commented out test that you're referring to, is an existing test (not that I added or commented). I was going to remove but interestingly its testing a part of code where (prima-facie) it should fail, but it passes (probably why it was disabled in the first place) ! So technically I hope this regression patch I submitted could go through since this feedback isn't towards that patch, but in my part I am quite intrigued about this test (and how it passes) and probably I'd get back on this thread about this particular commented out test in question, as time permits. -- Robins Tharakan On 25 June 2013 04:12, Robins Tharakan thara...@gmail.com wrote: Thanks a ton Szymon (for a reminder on this one). As a coincidental turn of events, I have had to travel half way across the world and am without my personal laptop (without a linux distro etc.) and just recovering from a jet-lag now. I'll try to install a VM on a make-shift laptop and get something going to respond as soon as is possible. Thanks -- Robins Tharakan -- Robins Tharakan On 17 June 2013 05:19, Szymon Guz mabew...@gmail.com wrote: On 23 May 2013 00:34, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of CREATE OPERATOR (src/backend/commands/operatorcmds.c) from 56% to 91%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, there is one commented out test. I think it should be run, or deleted. There is no use of commented sql code which is not run. What do you think? regards, Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
On 06/26/2013 12:29 AM, Szymon Guz wrote: OK, so I think this patch can be committed, I will change the status. Can we have a full review before you mark it ready for committer? How did you test it? What kinds of review have you done? The committer can't know whether it's ready or not if he doesn't have a full report from you. Thanks! -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- 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] Add more regression tests for CREATE OPERATOR
On 26 June 2013 20:55, Josh Berkus j...@agliodbs.com wrote: On 06/26/2013 12:29 AM, Szymon Guz wrote: OK, so I think this patch can be committed, I will change the status. Can we have a full review before you mark it ready for committer? How did you test it? What kinds of review have you done? The committer can't know whether it's ready or not if he doesn't have a full report from you. Thanks! Hi Josh, I will add more detailed descriptions to all patches I set as read for committer. Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
On 26 June 2013 20:57, Szymon Guz mabew...@gmail.com wrote: On 26 June 2013 20:55, Josh Berkus j...@agliodbs.com wrote: On 06/26/2013 12:29 AM, Szymon Guz wrote: OK, so I think this patch can be committed, I will change the status. Can we have a full review before you mark it ready for committer? How did you test it? What kinds of review have you done? The committer can't know whether it's ready or not if he doesn't have a full report from you. Thanks! Hi Josh, so I've got a couple of questions. Is it enough to provide the description in the commitfest app, or is that better to send an email and provide link in commitfest? This is a patch only with regression tests, is that enough to write something like: This patch applies cleanly on trunk code. All tests pass, the test coverage increses as provided.? Or do you expect some more info? thanks, Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
Is it enough to provide the description in the commitfest app, or is that better to send an email and provide link in commitfest? Better to do it here, on the list. This is a patch only with regression tests, is that enough to write something like: This patch applies cleanly on trunk code. All tests pass, the test coverage increses as provided.? Or do you expect some more info? Yes, mainly: a) does it test what it purports to test? b) do the tests pass on your machine? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- 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] Add more regression tests for CREATE OPERATOR
On 26 June 2013 21:10, Josh Berkus j...@agliodbs.com wrote: Is it enough to provide the description in the commitfest app, or is that better to send an email and provide link in commitfest? Better to do it here, on the list. This is a patch only with regression tests, is that enough to write something like: This patch applies cleanly on trunk code. All tests pass, the test coverage increses as provided.? Or do you expect some more info? Yes, mainly: a) does it test what it purports to test? b) do the tests pass on your machine? Done, could you confirm that it is OK now? I've also checked all the patches on the newest trunk. thanks, Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
Thanks a ton Szymon (for a reminder on this one). As a coincidental turn of events, I have had to travel half way across the world and am without my personal laptop (without a linux distro etc.) and just recovering from a jet-lag now. I'll try to install a VM on a make-shift laptop and get something going to respond as soon as is possible. Thanks -- Robins Tharakan -- Robins Tharakan On 17 June 2013 05:19, Szymon Guz mabew...@gmail.com wrote: On 23 May 2013 00:34, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of CREATE OPERATOR (src/backend/commands/operatorcmds.c) from 56% to 91%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, there is one commented out test. I think it should be run, or deleted. There is no use of commented sql code which is not run. What do you think? regards, Szymon
Re: [HACKERS] Add more regression tests for CREATE OPERATOR
On 23 May 2013 00:34, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of CREATE OPERATOR (src/backend/commands/operatorcmds.c) from 56% to 91%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, there is one commented out test. I think it should be run, or deleted. There is no use of commented sql code which is not run. What do you think? regards, Szymon
