Re: [HACKERS] psql - add ability to test whether a variable exists
Correct Fabien. I have already removed myself as a reviewer. Thanks. - robins | mobile On 20 Sep. 2017 5:13 pm, "Fabien COELHO" <coe...@cri.ensmp.fr> wrote: > > Hello Robins, > > I was able to test the functionality (which seemed to work fine) and fed in >> my comment to assist anyone else reviewing this patch (and intentionally >> let it's state as 'Needs Review'). >> >> While trying to provide my feedback, on hindsight I should have been more >> detailed about what I didn't test. Being my first review, I didn't >> understand that not checking a box meant 'failure'. For e.g. I read the >> sgml changes, which felt okay but didn't click 'Passed' because my env >> wasn't setup properly. >> > > Hmmm, ISTM that it was enough. The feature is psql specific, so the fact > that it works against a 9.6 server is both expected and fine. So ISTM that > your test "passed". > > Just running "make check" would run the non regression test, which is > basically what you tested online, against the compiled version. > > Probably you should have a little look at the source code and doc as well. > > I've set this back to 'Needs Review' because clearly needs it. >> > > Hmmm. > > If you do a review, which I think you have done, then you have done it:-) > > If you consider that your test was not a review and you do not intend to > provide one, then thanks for the feedback anyway, and maybe you should > consider removing yourself from the "Reviewer" column, otherwise nobody > will provide a review. > > -- > Fabien. >
Re: [HACKERS] psql - add ability to test whether a variable exists
I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. I've set this back to 'Needs Review' because clearly needs it. Apologies for the noise here. The new status of this patch is: Needs review -- 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] psql - add ability to test whether a variable exists
Hi Fabien,
I was able to test the functionality (which seemed to work fine) and fed in
my comment to assist anyone else reviewing this patch (and intentionally
let it's state as 'Needs Review').
While trying to provide my feedback, on hindsight I should have been more
detailed about what I didn't test. Being my first review, I didn't
understand that not checking a box meant 'failure'. For e.g. I read the
sgml changes, which felt okay but didn't click 'Passed' because my env
wasn't setup properly.
I've set this back to 'Needs Review' because clearly needs it.
Apologies for the noise here.
-
robins
On 20 September 2017 at 16:18, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>
> Hello Robins,
>
> Thanks for the review.
>
> The following review has been posted through the commitfest application:
>> make installcheck-world: not tested
>> Implements feature: tested, failed
>>
>
> Where ?
>
> Spec compliant: not tested
>> Documentation:tested, failed
>>
>
> Where ? I just regenerated the html doc on the patch without a glitch.
>
> The patch applies cleanly and compiles + installs fine (although am unable
>> to do installcheck-world on my Cygwin setup). This is how the patch works
>> on my setup.
>>
>> $ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost
>> psql (11devel, server 9.6.1)
>> Type "help" for help.
>>
>> postgres=# \set i 1
>> postgres=# \if :{?i}
>> postgres=# \echo 'testing'
>> testing
>> postgres=# \endif
>> postgres=# \if :{?id}
>> postgres@# \echo 'testing'
>> \echo command ignored; use \endif or Ctrl-C to exit current \if block
>> postgres@# \endif
>> postgres=#
>>
>
> ISTM that this is the expected behavior.
>
> In the first case, "i" is defined, so the test is true and the echo echoes.
>
> In the second case, "id" is undefined, the test is false and the echo is
> skipped.
>
> I do not understand why you conclude that the feature "failed".
>
> --
> Fabien.
>
Re: [HACKERS] psql - add ability to test whether a variable exists
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Spec compliant: not tested
Documentation:tested, failed
The patch applies cleanly and compiles + installs fine (although am unable to
do installcheck-world on my Cygwin setup).
This is how the patch works on my setup.
$ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost
psql (11devel, server 9.6.1)
Type "help" for help.
postgres=# \set i 1
postgres=# \if :{?i}
postgres=# \echo 'testing'
testing
postgres=# \endif
postgres=# \if :{?id}
postgres@# \echo 'testing'
\echo command ignored; use \endif or Ctrl-C to exit current \if block
postgres@# \endif
postgres=#
--
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] Patch: Add --no-comments to skip COMMENTs with pg_dump
On 20 July 2017 at 05:08, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Jul 19, 2017 at 8:59 PM, > > Fabrízio de Royes Mello > > You should add the properly sgml docs for this pg_dumpall change also. > > Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in > extensions are in src/test/modules/test_pg_dump, but you just care > about the former with this patch. And if you implement some new tests, > look at the other tests and base your work on that. > Thanks Michael / Fabrízio. Updated patch (attached) additionally adds SGML changes for pg_dumpall. (I'll try to work on the tests, but sending this nonetheless ). - robins pgdump_nocomments_v3.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] Patch: Add --no-comments to skip COMMENTs with pg_dump
On 18 July 2017 at 23:55, David Fetter <da...@fetter.org> wrote: > > Excellent point about pg_dumpall. I'll see what I can draft up in the > next day or two and report back. Hi David, You may want to consider this patch (attached) which additionally has the pg_dumpall changes. It would be great if you could help with the tests though, am unsure how to go about them. - robins pgdump_nocomments_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
[HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump
Hi, Attached is a patch adds a --no-comments argument to pg_dump to skip generation of COMMENT statements when generating a backup. This is crucial for non-superusers to restore a database backup in a Single Transaction. Currently, this requires one to remove COMMENTs via scripts, which is inelegant at best. A similar discussion had taken place earlier [1], however that stopped (with a Todo entry) since it required more work at the backend. This patch provides a stop-gap solution until then. If the feedback is to tackle this is by filtering comments for specific DB objects, an argument name (for e.g. --no-extension-comments or something) would help and I could submit a revised patch. Alternatively, if there are no objections, I could submit this to the Commitfest. References: [1] https://www.postgresql.org/message-id/1420.1397099637%40sss.pgh.pa.us - robins pgdump_nocomments_v1.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] Allow pg_dumpall to work without pg_authid
On 5 March 2017 at 18:00, Simon Riggs <si...@2ndquadrant.com> wrote: > I'm looking to commit the patch version I posted, so I would like your > comments that it does continue to solve the problems you raised. > Thanks Simon, for confirming. Yes, the updated patch does solve the problem. - robins
Re: [HACKERS] Allow pg_dumpall to work without pg_authid
On 26 February 2017 at 21:37, Robert Haas <robertmh...@gmail.com> wrote: > > How's that not a bug? I mean, it's reasonable for someone to want to > restrict the superuser in a cloud environment, but if they restrict it > so much that you can't take a backup with standard tools, I'd say they > should also patch the tools, though maybe a better idea would be to > restrict the superuser a bit less. > They 'should', and I completely agree; but that isn't the reality today. Thus the patch. Now it's understandable for the community to say 'not my problem' but I'd say that depends on what it considers 'my problem'. If half the people manage their installations on a cloud solution, it unwillingly becomes one. Unrelated, it still does allow a non-superuser to take a dump sans the password, which doesn't seem like a bad idea, since the patch doesn't do anything more than dump from pg_roles, which is anyways available to non-superusers. I await if more object similarly. - robins
Re: [HACKERS] Allow pg_dumpall to work without pg_authid
On 26 February 2017 at 19:26, Robert Haas <robertmh...@gmail.com> wrote: > I am a little surprised that this patch has gotten such a good > reception. We haven't in the past been all that willing to accept > core changes for the benefit of forks of PostgreSQL; extensions, sure, > but forks? Maybe we should take the view that Amazon has broken this > and Amazon ought to fix it, rather than making it our job to (try to) > work around their bugs. > > (Disclaimer: I work at the said company, although don't represent them in any way. This patch is in my personal capacity) To confirm, this did originate by trying to accommodate a fork. But what I can say is that this doesn't appear to be a bug; what they call Super-User isn't effectively one. Personally, I think it would be wise to also consider that this fork has a very large user-base and for that user-base, this 'is' Postgres. Further, case-by-case exceptions still should be considered for important issues (here, this relates to lock-in). Either way, I could pull-back the patch if more people object. - robins
Re: [HACKERS] Allow pg_dumpall to work without pg_authid
Stephen, On 20 February 2017 at 08:50, Stephen Frost <sfr...@snowman.net> wrote: > The other changes to use pg_roles instead of pg_authid when rolpassword > isn't being used look like they should just be changed to use pg_roles > instead of using one or the other. That should be an independent patch > from the one which adds the option we are discussing. > Sure. Attached are 2 patches, of which 1 patch just replaces pg_authid with pg_roles in pg_dumpall. The only exceptions there are buildShSecLabels() & pg_catalog.binary_upgrade_set_next_pg_authid_oid() which I thought should still use pg_authid. Perhaps --no-role-passwords instead? > > Makes Sense. The updated patch uses this name. > > pg_dumpall --no-pgauthid --globals-only > a.sql > > Does that then work with a non-superuser account on a regular PG > instance also? If not, I'd like to suggest that we consider follow-on > patches to provide options for whatever else currently requires > superuser on a regular install. > > If I understand that correctly, the answer is Yes. I didn't test all db objects, but trying to do a pg_dumpall using a non-priviledge user does successfully complete with all existing users dumped successfully. pg_dumpall --globals-only --no-role-password > a.sql > Yes, please do create a commitfest entry for this. > > Created Commitfest entry. - robins use_pgroles_instead_of_pg_authid.diff.gz Description: GNU Zip compressed data no_role_password.diff.gz Description: GNU Zip compressed 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] Allow pg_dumpall to work without pg_authid
On 19 February 2017 at 17:02, Robins Tharakan <thara...@gmail.com> wrote: > > On Sun, 19 Feb 2017 at 10:08 Stephen Frost <sfr...@snowman.net> wrote: > >> If anything, it should use pg_roles, not pg_user. >> >> I don't really like the "--avoid-pgauthid" option, but "--no-passwords" >> would probably work. >> >> > '--no-passwords' is a good alternative. > Would submit a patch soon. > > After reviewing further, it seems that merely adding a password related workaround wouldn't suffice. Further --no-password is already an alias for -w, so that flag is effectively taken. Since the main restriction with AWS RDS is the unavailability of pg_authid, probably that is a better basis to name the flag on. Attaching a patch to add a new flag (--no-pgauthid) to pg_dumpall that can dump Globals without needing pg_authid. So the following works with AWS RDS Postgres databases. pg_dumpall --no-pgauthid --globals-only > a.sql I'll create a Commitfest entry, if there aren't many objections. - robins pgdumpall_nopgauthid_flag.diff.gz Description: GNU Zip compressed 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] Allow pg_dumpall to work without pg_authid
On Sun, 19 Feb 2017 at 10:08 Stephen Frost <sfr...@snowman.net> wrote: > If anything, it should use pg_roles, not pg_user. > > I don't really like the "--avoid-pgauthid" option, but "--no-passwords" > would probably work. > > Am sorry, I meant pg_roles (FWIW, the github URL given earlier uses pg_roles). '--no-passwords' is a good alternative. Would submit a patch soon. - Robins -- - robins
[HACKERS] Allow pg_dumpall to work without pg_authid
Hi, I would like to work on a patch to accommodate restricted environments (such as AWS RDS Postgres) which don't allow pg_authid access since their definition of Superuser is just a regular user with extra permissions. Would you consider a patch to add a flag to work around this restriction, Or, do you prefer that this be maintained outside core? I could add a flag such as --avoid-pgauthid (am open to options) that skips pg_authid and uses pg_user (but essentially resets all User passwords). Mostly this is better than not being able to get the dump at all. I have a fork here (a few weeks old): https://github.com/postgres/postgres/commit/552f4d92fde9518f934447e032b8ffcc945f12d9 Thanks Robins Tharakan http://www.thatguyfromdelhi.com/2016/12/custom-pgdumpall-now-works-with-aws.html -- - robins
Re: [HACKERS] High-CPU consumption on information_schema (only) query
> > > Without having at least compared EXPLAIN outputs from the two boxes, you > have no business jumping to that conclusion. > > If EXPLAIN does show different plans, my first instinct would be to wonder > whether the pg_stats data is equally up-to-date on both boxes. > > regards, tom lane > Thanks. EXPLAIN plans were different but (don't have them now and) didn't know system catalogs were so severely affected by outdated statistics as well (which is why I was looking for any other reasons I might be missing). I reckon an ANALYSE; should solve this? ... Would get back if I have something else to offer. - robins -- - robins
Re: [HACKERS] High-CPU consumption on information_schema (only) query
On Fri, 9 Sep 2016 at 09:39 Andres Freund <and...@anarazel.de> wrote: > On 2016-09-07 23:37:31 +0000, Robins Tharakan wrote: > > If someone asks for I could provide SQL + EXPLAIN, but it feels > irrelevant > > here. > > Why is that? information_schema are normal sql queries, and some of them > far from trivial. > > Andres > Hi Andres, I completely agree. With 'irrelevant' I was only trying to imply that irrespective of the complexity of the query, a replicated box was seeing similar slowness whereas a Restored DB wasn't. It felt that the SQL itself isn't to blame here... In effect, I was trying to ask if I am forgetting / missing something very obvious / important that could cause such an observation. As others recommended, I am unable to have direct access to the production (master / slave) instances and so GDB / stack trace options are out of bounds at this time. I'll revert if I am able to do that. - thanks robins -- - robins
[HACKERS] High-CPU consumption on information_schema (only) query
Hi, An SQL (with only information_schema related JOINS) when triggered, runs with max CPU (and never ends - killed after 2 days). - It runs similarly (very slow) on a replicated server that acts as a read-only slave. - Top shows only postgres as hitting max CPU (nothing else). When query killed, CPU near 0%. - When the DB is restored on a separate test server (with the exact postgresql.conf) the same query works fine. - There is no concurrent usage on the replicated / test server (although the primary is a Production server and has concurrent users). Questions: - If this was a postgres bug or a configuration issue, query on the restored DB should have been slow too. Is there something very basic I am missing here? If someone asks for I could provide SQL + EXPLAIN, but it feels irrelevant here. I amn't looking for a specific solution but what else should I be looking for here? p.s.: All postgres servers are running the v9.3.10 - robins -- - robins
Re: [HACKERS] Accurate list of Keywords / Datatypes?
On 8 May 2016 at 07:38, Euler Taveira <eu...@timbira.com.br> wrote: > src/include/parser/kwlist.h Thanks Everyone. Kwlist.h got me going as to where to look, but I had to improvise (quite) a bit. Any and all advice about any other authoritative lists (like kwlist.h) would be a big help, since currently much is kludgy. Created a GitHub Repo in case someone's interested ( https://github.com/robins/npp-lang-plpgsql). For PLPGSQL, (src/pl/plpgsql/src/pl_scanner.c) For DataTypes, (src/interfaces/ecpg/preproc/c_keywords.c ; src/backend/bootstrap/bootstrap.c) as well as parse all HTML pages under http://www.postgresql.org/docs/devel/static/datatype.html. For Config Parameters (recovery.sample.conf ; postgresql.sample.conf) as well as parse all HTML pages under http://www.postgresql.org/docs/devel/static/runtime-config.html ; http://www.postgresql.org/docs/devel/static/libpq-connect.html For Extensions (Parse HTML: http://www.postgresql.org/docs/devel/static/contrib.html) - robins
[HACKERS] Accurate list of Keywords / Datatypes?
Hi, While creating a Syntax Highlighting XML for Notepad++ (something like a PLSQL one here http://goo.gl/UBbHdt ), I was looking for a list of Keywords (& separately list of Datatypes) that Postgres uses in a given version (Say DEVEL branch). I did find the Reserved Keyword list ( http://www.postgresql.org/docs/devel/static/sql-keywords-appendix.html) but it doesn't look like what I need, because: 1. The ones marked as 'Reserved' isn't all the words I am interested in (since these are just 'Reserved') 2. The entire list seems like an overkill (since it has words such as K and ROUTINE_CATALOG, which aren't really what I'd like highlighted when I am working on an SQL file in NPP) Should I be looking somewhere else? Parse keywords from Git Source file (if so where)? Parse PG Documentation? I tried ( https://goo.gl/OYeYuE ) parsing HTML Tags (such as VARNAME / TOKEN / LITERAL / COMMAND) in PG Documentation but is there's a simpler (more accurate) way for this? Any pointers would be great! - thanks robins
Re: [HACKERS] Pgbench with -f and -S
Got it. Thanks for explaining that. Was looking at -S earlier and didn't notice the new -b and @ weight options in 9.6devel. On Mon, 18 Apr 2016 at 10:54 Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > >> For the future 9.6, scripts options are cumulatives, so -f & -S can be > >> combined. > >> > >> Indeed, for the <= 9.5 it seems that some options are silently ignores, > >> when combining -S/-f, only the last one is kept, it should be warned > about. > > > > Thanks Fabien, for confirming about the missing warning. > > > > Also, by 'combined' I think you mean that both (built-in SELECTs & Custom > > Script) run, although the dev docs don't (yet) say anything about that. > > Hmmm... I think it does implicitely, with "add" and "and" in the > following: > > From http://www.postgresql.org/docs/devel/static/pgbench.html: > >-b scriptname[@weight] >Add the specified builtin script to the list of executed scripts. [...] > > Idem -f. > > And later at the beginning of the Notes: > >pgbench executes test scripts chosen randomly from a specified list. >They include built-in scripts with -b and user-provided custom scripts > with -f. >[...] > > -- > Fabien. -- - robins
Re: [HACKERS] Pgbench with -f and -S
On 17 April 2016 at 21:24, Fabien COELHO <coe...@cri.ensmp.fr> wrote: > For the future 9.6, scripts options are cumulatives, so -f & -S can be > combined. > > Indeed, for the <= 9.5 it seems that some options are silently ignores, > when combining -S/-f, only the last one is kept, it should be warned about. > Thanks Fabien, for confirming about the missing warning. Also, by 'combined' I think you mean that both (built-in SELECTs & Custom Script) run, although the dev docs don't (yet) say anything about that. - robins
[HACKERS] Pgbench with -f and -S
Hi, Pgbench allows -f and -S combinations together where the doc says that -S effectively uses the internal select-only script. Is it okay to assume that -f is disregarded here? Or are they run in round-robin fashion (although then, how does it know which read-only part of my script to run?) or something else like that? Effectively, I think it should raise NOTICE that -f is useless here. - robins -- - robins
Re: [HACKERS] Cross-check recent documentation changes
On 29 October 2015 at 15:35, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > errmsg_plural() function determines whether to output the singular version > or the plural > Duh. Thanks Amit! Should have noticed the function-name change. -- Robins Tharakan
[HACKERS] Cross-check recent documentation changes
Hi, Was reviewing recent commits, and it seems the following commit adds an extra line to some comments. Just wanted to cross-check if that was intentional. Commit: http://goo.gl/zxA00l Pre-Commit: http://goo.gl/2DpLxi Post-Commit: http://goo.gl/eKcNm3 Apologies for the noise, if this was intentional. -- Robins Tharakan
[HACKERS] Per row status during INSERT .. ON CONFLICT UPDATE?
Hi, Is there a way to know which rows were INSERTed and UPDATEd when doing a INSERT ... ON CONFLICT UPDATE? Probably via pseudo column indicating INSERT / UPDATE ? The RETURNING clause just allows us to return columns, but am unable to find a way to know 'what' happened to a given row. Any pointers would be helpful. Couldn't find anything related in 9.5devel docs either. -- thanks Robins
Re: [HACKERS] Per row status during INSERT .. ON CONFLICT UPDATE?
On 19 May 2015 at 23:24, Peter Geoghegan p...@heroku.com wrote: That's certainly something we talked about. It could probably be done with some kind of magical expression. I have to wonder how many of the people that are sure that they need this really do, though. Is it really natural to care about this distinction with idiomatic usage? Thanks everyone for responding promptly. Not sure if I can be authoritative for many, but for me, the need emanates from having to move an ETL off MSSQL Server, which supports OUTPUT $action (similar to RETURNING * in Postgres) where $action is the per-row status (INSERT / UPDATE). My use-case is to create an extra row for all UPDATEd rows (only), which is implemented in MSSQL by enveloping the MERGE with an INSERT (MERGE ... OUTPUT $action) WHERE $action = 'UPDATE'. Am still to test, but looks like Thom's reply earlier could take care of my use-case, so we may need more people requesting this magic field, with a valid use-case. -- Robins Tharakan
Re: [HACKERS] Patch to add regression tests for SCHEMA
On 9 July 2013 08:57, Fabien COELHO coe...@cri.ensmp.fr wrote: I cannot apply the patch, it seems to be truncated: Hi Fabien, Please find attached the updated patch. It seems the only difference between v5 and v6 is probably a newline at the end (Line 291 was the last line), in fact meld doesn't even show anything. I'll try to be more careful though. git reset --hard HEAD git pull patch -p1 ../regress_schema_v6.patch patch -p1 -R ../regress_schema_v6.patch patch -p1 ../regress_schema_v6.patch make clean ./configure --enable-depend --enable-coverage --enable-cassert --enable-debug make -j3 check Do let me know if something is still amiss. -- Robins Tharakan
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi Fabien, Please find attached the updated patch. It seems the only difference between v5 and v6 is probably a newline at the end (Line 291 was the last line), in fact meld doesn't even show anything. I'll try to be more careful though. git reset --hard HEAD git pull patch -p1 ../regress_schema_v6.patch patch -p1 -R ../regress_schema_v6.patch patch -p1 ../regress_schema_v6.patch make clean ./configure --enable-depend --enable-coverage --enable-cassert --enable-debug make -j3 check Do let me know if something is still amiss. -- Robins Tharakan regress_schema_v6.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 some regression tests for SEQUENCE
On 9 July 2013 08:41, Fabien COELHO coe...@cri.ensmp.fr wrote: I do not see any difference between both regress_sequence_v[45].patch**. I guess you sent the earlier version. Thanks Fabien. This was a wrong attachment to the email. Please find attached the updated patch (I've renamed v5 as v6 for clarity). git reset --hard HEAD git pull patch -p1 ../regress_sequence_v6.patch patch -p1 -R ../regress_sequence_v6.patch patch -p1 ../regress_sequence_v6.patch make clean ./configure --enable-depend --enable-coverage --enable-cassert --enable-debug make -j3 check -- Robins Tharakan regress_sequence_v6.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 regression tests for ROLE (USER)
On 6 July 2013 20:25, Robins Tharakan thara...@gmail.com wrote: Do let me know your view on this second point, so that I can remove these tests if so required. Hi, Please find attached the updated patch. It address the first issue regarding reducing the repeated CREATE / DROP ROLEs. It still doesn't address the excessive (syntactical) checks tough. I am still unclear as to how to identify which checks to skip. (As in, although I have a personal preference of checking everything, my question probably wasn't clear in my previous email. I was just asking 'how' to know which checks to not perform.) Should a 'syntax error' in the message be considered as an unnecessary check? If so, its easier for me to identify which checks to skip, while creating future tests. -- Robins Tharakan regress_role_v4.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] Patch to add regression tests for SCHEMA
Hi Fabien, Appreciate you being able to check right away. Seems something went wrong with these set of patches... Would check again and resubmit them soon. -- Robins Tharakan On 9 July 2013 10:57, Fabien COELHO coe...@cri.ensmp.fr wrote: Hello Robins, PFA an updated patch: - Renamed ROLEs as per new feedback (although the previous naming was also as per an earlier feedback) - Merged tests into namespace I cannot apply the patch, it seems to be truncated: sh git apply ../regress_schema_v5.patch ### warnings about trailing whitespace, then: fatal: corrupt patch at line 291 Another go with patch: sh patch -p1 ../regress_schema_v5.patch ... patch unexpectedly ends in middle of line patch: malformed patch at line 290: I have this: sh cksum ../regress_schema_v5.patch 985580529 11319 ../regress_schema_v5.patch -- Fabien.
Re: [HACKERS] Patch to add regression tests for SCHEMA
On 3 July 2013 10:19, Robert Haas robertmh...@gmail.com wrote: In this particular case, I think that adding a new set of tests isn't the right thing anyway. Schemas are also known as namespaces, and the existing namespace test is where related test cases live. Add these tests there instead. Rename regression users to regress_rol_nsp1 etc. per convention established in the CREATE OPERATOR patch. Hi Robert, PFA an updated patch: - Renamed ROLEs as per new feedback (although the previous naming was also as per an earlier feedback) - Merged tests into namespace -- Robins Tharakan regress_schema_v5.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 some regression tests for SEQUENCE
On 3 July 2013 10:13, Robert Haas robertmh...@gmail.com wrote: I think you should rename the roles used here to regress_rol_seq1 etc. to match the CREATE OPERATOR patch. Please find updated patch: - 'make check' successful with recent changes - Renamed ROLEs as per feedback -- Robins Tharakan regress_sequence_v5.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 dbcommands
On 26 June 2013 01:55, Robins Tharakan thara...@gmail.com wrote: Code coverage improved from 36% to 68%. Updated patch: - Renamed ROLEs as per Robert's feedback (prepend regress_xxx) - Added test to serial_schedule (missed out earlier). -- Robins Tharakan regress_db_v5.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 tests for LOCK TABLE
On 23 May 2013 18:19, Robins Tharakan thara...@gmail.com wrote: Please find attached a patch to take code-coverage of LOCK TABLE ( src/backend/commands/lockcmds.c) from 57% to 84%. Updated the patch: - Updated ROLEs as per Robert's feedback - Added test to serial_schedule. -- Robins Tharakan regress_lock_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 regression tests for SET xxx
On 18 June 2013 05:01, Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Updated the patch: - Updated ROLE names as per Robert's feedback (regress_xxx) - Added test to serial_schedule -- Robins Tharakan regress_variable_v5.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 regression tests for DISCARD
On 7 July 2013 14:08, Jeff Janes jeff.ja...@gmail.com wrote: Why does serial_schedule even exist? Couldn't we just run the parallel schedule serially, like what happens when MAX_CONNECTIONS=1? Well, I am sure it works that way, without errors. The 'why' still eludes me though, just that its required for this submission. -- Robins Tharakan
Re: [HACKERS] Add regression tests for ROLE (USER)
However, before it can get committed, I think this set of tests needs streamlining. It does seem to me valuable, but I think it's wasteful in terms of runtime to create so many roles, do just one thing with them, and then drop them. I recommend consolidating some of the tests. For example: Generally, I think that the tests which return a syntax error are of limited value and should probably be dropped. That is unlikely to get broken by accident. If the syntax error is being thrown by something outside of bison proper, that's probably worth testing. But I think that testing random syntax variations is a pretty low-value proposition. Thanks Robert. Although the idea of being repetitive was just about trying to make tests simpler to infer for the next person, but I guess this example was obviously an overkill. Point taken, would correct and revert with an updated patch. However, the other aspect that you mention, I am unsure if I understand correctly. Do you wish that syntactical errors not be tested? If so, probably you're referring to tests such as the one below, and then I think it may get difficult at times to bifurcate how to chose which tests to include and which to not. Can I assume that all errors that spit an error messages with 'syntax error' are to be skipped, probably that'd be an easy test for me to know what you'd consider important? +ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc'; +ERROR: syntax error at or near UNENCRYPTED +LINE 1: ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASS... Personally, I think all tests are important. Unless there is a clear understanding that aiming for 100% code-coverage isn't the goal, I think all tests are important, syntactical or otherwise. Its possible that not all code is reachable (therefore testable) but the vision generally remains at 100%. Do let me know your view on this second point, so that I can remove these tests if so required. Robins Tharakan
Re: [HACKERS] Add regression tests for DISCARD
Thanks Fabrizio. Although parallel_schedule was a miss for this specific patch, however, I guess I seem to have missed out serial_schedule completely (in all patches) and then thanks for pointing this out. Subsequently Robert too noticed the miss at the serial_schedule end. Please find attached a patch, updated towards serial_schedule / parallel_schedule as well as the role name change as per Robert's feedback on CREATE OPERATOR thread. -- Robins Tharakan On 2 July 2013 09:32, Fabrízio de Royes Mello fabriziome...@gmail.comwrote: On Mon, Jul 1, 2013 at 5:59 PM, Robins Tharakan thara...@gmail.comwrote: Thanks Marko for pointing out about guc.sql. Please find attached a patch to move DISCARD related tests from guc.sql to discard.sql. It adds an extra test for a DISCARD PLANS line, although I amn't sure on how to validate that its working. Personally, I wouldn't call this a great patch, since most of the tests were already running, although in a generic script. The separation of DISCARD related tests to another file is arguably good for the long-term though. Robins, You must add this new test case called discard to src/test/regress/parallel_schedule and src/test/regress/serial_schedule, because if we do make check the new discard test case is not executed. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello regress_discard_v5.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 regression tests for DISCARD
On 17 June 2013 18:14, Marko Kreen mark...@gmail.com wrote: Perhaps existing tests in guc.sql should be merged into it? Thanks Marko for pointing out about guc.sql. Please find attached a patch to move DISCARD related tests from guc.sql to discard.sql. It adds an extra test for a DISCARD PLANS line, although I amn't sure on how to validate that its working. Personally, I wouldn't call this a great patch, since most of the tests were already running, although in a generic script. The separation of DISCARD related tests to another file is arguably good for the long-term though. -- Robins Tharakan regress_discard_v3.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 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] New regression test time
On 30 June 2013 02:33, Amit kapila amit.kap...@huawei.com wrote: On Sunday, June 30, 2013 11:37 AM Fabien COELHO wrote: If we had a different set of tests, that would be a valid argument. But we don't, so it's not. And nobody has offered to write a feature to split our tests either. I have done a POC. See: https://commitfest.postgresql.org/action/patch_view?id=1170 I think it is better to submit for next commit fest which is at below link: https://commitfest.postgresql.org/action/commitfest_view?id=19 Hi, - There is a certain value in having separate tests, just that for the big-tests to be any meaningful, if the buildfarm could run on a periodic (daily?) basis and send some kind of automated bug-reports. Without an automatic feedback, most may not inclined to run all tests before submitting a patch and there'd be a big pile up near a release. - For now, the new tests that I submit for review (for next CF) would be for 'make check', until a 'make bigcheck' or whatever is up and running. -- Robins Tharakan
Re: [HACKERS] [PATCH] Add session_preload_libraries configuration parameter
On 12 June 2013 22:12, Peter Eisentraut pete...@gmx.net wrote: This is like shared_preload_libraries except that it takes effect at backend start and can be changed without a full postmaster restart. It is like local_preload_libraries except that it is still only settable by a superuser. This can be a better way to load modules such as auto_explain. Since there are now three preload parameters, regroup the documentation a bit. Put all parameters into one section, explain common functionality only once, update the descriptions to reflect current and future realities. --- Hi, Did some basic checks on this patch. List-wise feedback below. - Cleanly applies to Git-Head: Yes (Minor 1 line offset in guc.c, but that's probably because of the delay in reviewing) - Documentation Updated: Yes - All tests pass: Yes - Removed unnecessary extra-lines: Yes - Do we want it?: ??? - Any visible issues: No - Any compiler warnings: No - Others: Number of new lines added not covered by tests: -107 (Wierd but, it seems to have reduced line coverage). But correct me if am not seeing the elephant in the room. -- Robins Tharakan
Re: [HACKERS] Eliminating PD_ALL_VISIBLE, take 2
On 10 June 2013 00:17, Jeff Davis pg...@j-davis.com wrote: On Thu, 2013-05-30 at 10:07 -0700, Jeff Davis wrote: Come to think of it, even without the torn page checksum issue, do we really want to actively clear the all-visible flags after upgrade? Removed that from the patch and rebased. I think the best approach is to remove the bit opportunistically when we're already dirtying the page for something else. However, right now, there is enough skepticism of the general approach in this patch (and enough related proposals) that I'll leave this to be resolved if and when there is more agreement that my approach is a good one. Did some basic checks on this patch. List-wise feedback below. - Cleanly applies to Git-Head: Yes (Some offsets, but thats probably because of delay in review) - Documentation Updated: No. (Required?) - Tests Updated: No. (Required?) - All tests pass: Yes - Does it Work : ??? - Any visible issues: No - Any compiler warnings: No - Others: Number of uncovered lines: Reduced by 167 lines Robins Tharakan
Re: [HACKERS] [PATCH] Add session_preload_libraries configuration parameter
Number of new lines added not covered by tests: -107 (Wierd but, it seems to have reduced line coverage). But correct me if am not seeing the elephant in the room. My apologies. Number of new untested lines: 108 Robins
Re: [HACKERS] Add more regression tests for dbcommands
Hi Andres, Just an aside, your point about CONNECTION LIMIT was something that just didn't come to my mind and is probably a good way to test ALTER DATABASE with CONNECTION LIMIT. Its just that that actually wasn't what I was testing there. That 'CONNECTION LIMIT' test was coupled with CREATE DATABASE because I wanted to test that 'branch' in the CREATE DATABASE function just to ensure that there was a regression test that tests CONNECTION LIMIT specifically with CREATE DATABASE. That's all. A check to confirm whether connection limit restrictions actually got enforced was something I missed, but well, its out of the window for now anyway. -- Robins Tharakan On 26 June 2013 06:34, Andres Freund and...@2ndquadrant.com wrote: Hi, I am generally a bit unsure whether the regression tests you propose aren't a bit too verbose. Does any of the committers have an opinion about this? My feeling is that they are ok if they aren't slowing down things much. On 2013-06-26 01:55:53 -0500, Robins Tharakan wrote: The CREATE DATABASE test itself was checking whether the 'CONNECTION LIMIT' was working. Removed that as well. You should be able to test that by setting the connection limit to 1 and then try to connect using \c. The old connection is only dropped after the new one has been successfully performed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Add some regression tests for SEQUENCE
Seems like thats because of a recent (15th May 2013) patch (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message that is printed. Its just one line of difference actually. Let me know if this is otherwise good to go. I'll checkout the latest revision and submit this patch again. -- Robins Tharakan On 28 June 2013 15:53, Josh Berkus j...@agliodbs.com wrote: On 05/07/2013 03:40 PM, Robins Tharakan wrote: Hi, Have provided an updated patch as per Fabien's recent response on Commitfest site. Any and all feedback is appreciated. The updated patch is giving a FAILURE for me: parallel group (19 tests): limit temp plancache conversion rowtypes prepare without_oid copy2 xml returning rangefuncs polymorphism with domain truncate largeobject sequence alter_table plpgsql plancache... ok limit... ok plpgsql ... ok copy2... ok temp ... ok domain ... ok rangefuncs ... ok prepare ... ok without_oid ... ok conversion ... ok truncate ... ok alter_table ... ok sequence ... FAILED Thoughts? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
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
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 dbcommands
Hi Andres, Attached is a patch which does not CREATE DATABASE, but now the regression tests do not test the following: - ALTER DATABASE RENAME TO is not allowed on a database in use. Had to remove two tests that were using this. - ALTER DATABASE SET TABLESPACE is also not allowed on a database in use, so removed it (the existing test was supposed to fail too, but it was to fail at a later stage in the function when checking for whether the tablespace exists, but with the 'regression' database, it just doesn't even reach that part) - The CREATE DATABASE test itself was checking whether the 'CONNECTION LIMIT' was working. Removed that as well. Code coverage improved from 36% to 68%. -- Robins Tharakan On 24 June 2013 07:47, Andres Freund and...@2ndquadrant.com wrote: On 2013-05-21 02:58:25 +0530, Robins Tharakan wrote: Attached is an updated patch that does only 1 CREATE DATABASE and reuses that for all other tests. The code-coverage with this patch goes up from 36% to 70%. Even creating one database seems superfluous. The plain CREATE DATABASE has been tested due to the creation of the regression database itself anyway? All the other tests should be doable using the normal regression db? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_db_v4.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] [9.4 CF 1] The Commitfest Slacker List
Hi, Apologies for being unable to respond promptly. I've been traveling (without much access) and this was the fastest I could settle down. I was free for months and had to travel smack in the middle of the commitfest. Incidentally I had reviewed one patch after your direct email, but as someone earlier mentioned, actually pasting my name as 'reviewer' seemed awkward and so didn't then (but currently its set to David Fetter for some reason). I guess the point Tom mentioned earlier makes good sense and I agree with the spirit of the process . In my part would try to review more patches and mark them so on the commitfest page soon . -- Robins Tharakan On 23 June 2013 23:41, Josh Berkus j...@agliodbs.com wrote: Folks, For 9.2, we adopted it as policy that anyone submitting a patch to a commitfest is expected to review at least one patch submitted by someone else. And that failure to do so would affect the attention your patches received in the future. For that reason, I'm publishing the list below of people who submitted patches and have not yet claimed any patch in the commitfest to review. For those of you who are contributing patches for your company, please let your boss know that reviewing is part of contributing, and that if you don't do the one you may not be able to do the other. The two lists below, idle submitters and committers, constitutes 26 people. This *outnumbers* the list of contributors who are busy reviewing patches -- some of them four or more patches. If each of these people took just *one* patch to review, it would almost entirely clear the list of patches which do not have a reviewer. If these folks continue not to do reviews, this commitfest will drag on for at least 2 weeks past its closure date. Andrew Geirth Nick White Peter Eisentrout Alexander Korotkov Etsuro Fujita Hari Babu Jameison Martin Jon Nelson Oleg Bartunov Chris Farmiloe Samrat Revagade Alexander Lakhin Mark Kirkwood Liming Hu Maciej Gajewski Josh Kuperschmidt Mark Wong Gurjeet Singh Robins Tharakan Tatsuo Ishii Karl O Pinc Additionally, the following committers are not listed as reviewers on any patch. Note that I have no way to search which ones might be *committers* on a patch, so these folks are not necessarily slackers (although with Bruce, we know for sure): Bruce Momjian (momjian) Michael Meskes (meskes) Teodor Sigaev (teodor) Andrew Dunstan (adunstan) Magnus Hagander (mha) Itagaki Takahiro (itagaki) -- 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
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
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 regression tests for SET xxx
Thanks ! PFA the updated patch. Also remove a trailing whitespace at the end of SQL script. -- Robins Tharakan On 17 June 2013 17:29, Szymon Guz mabew...@gmail.com wrote: On 26 May 2013 19:56, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) (src/backend/commands/variable.c) from 65% to 82%. 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, the patch applies cleanly on code from trunk, however there are failing tests, diff attached. regards Szymon regress_variable_v3.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] Patch to add support of IF NOT EXISTS to others CREATE statements
Hi,
Did some basic checks on this patch. List-wise feedback below.
- Removed unnecessary extra-lines: Yes
- Cleanly applies to Git-Head: Yes
- Documentation Updated: Yes
- Tests Updated: Yes
- All tests pass: Yes. (But see Note below)
- Does it Work (CREATE AGGREGATE): Yes
- Does it Work (CREATE OPERATOR): Yes
- Does it Work (CREATE TYPE): Yes
- Does it Work (CREATE TEXT SEARCH): Yes
- Does it Work (CREATE COLLATION): Yes
- Do we want it?: ???
- Is this a new feature: Yes
- Does it support pg_dump: Unable to test currently :(
- Does it follow coding guidelines: Yes
- Any visible issues: No
- Any corner cases missed out: Some tests are not extensive (eg. CREATE
COLLATION).
- Performance tests required: No
- Any compiler warnings: A scan.c warning (scan.c:10181:23: warning: unused
variable ‘yyg’ [-Wunused-variable]) although I doubt that is being caused
by this patch.
- Are comments sufficient: Can't comment much on code comments.
- Others:
Number of new lines added not covered by tests: ~208
==
A typical kind of ERROR is emitted in most tests. (Verified at least in
CREATE AGGREGATE / OPERATOR / TEXT SEARCH TEMPLATE).
For e.g. CREATE OPERATOR IF NOT EXISTS tries to create an OPERATOR that is
already created in the test a few lines above. So although the feature is
tested, the test unnecessarily creates the first OPERATOR. If you need to
maintain 'completeness' within each tests, you could use unique numbering
of objects instead.
CREATE OPERATOR ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
CREATE OPERATOR ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
ERROR: operator ## already exists
CREATE OPERATOR IF NOT EXISTS ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
NOTICE: operator ## already exists, skipping
=
--
Robins Tharakan
On 24 May 2013 20:52, Fabrízio de Royes Mello fabriziome...@gmail.comwrote:
Hi all,
I working in a patch to include support of IF NOT EXISTS into CREATE
statements that not have it yet.
I started with DefineStmt section from src/backend/parser/gram.y:
- CREATE AGGREGATE [ IF NOT EXISTS ] ...
- CREATE OPERATOR [ IF NOT EXISTS ] ...
- CREATE TYPE [ IF NOT EXISTS ] ... [AS [{ENUM | RANGE}] (...)]
- CREATE TEXT SEARCH {PARSER | DITIONARY | TEMPLATE | CONFIGURATION} [ IF
NOT EXISTS ] ...
- CREATE COLLATION [ IF NOT EXISTS ] ...
My intention is cover anothers CREATE statements too, not just the above.
If has no objection about this implementation I'll finish him and soon I
sent the patch.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
On 24 May 2013 20:52, Fabrízio de Royes Mello fabriziome...@gmail.comwrote:
Hi all,
I working in a patch to include support of IF NOT EXISTS into CREATE
statements that not have it yet.
I started with DefineStmt section from src/backend/parser/gram.y:
- CREATE AGGREGATE [ IF NOT EXISTS ] ...
- CREATE OPERATOR [ IF NOT EXISTS ] ...
- CREATE TYPE [ IF NOT EXISTS ] ... [AS [{ENUM | RANGE}] (...)]
- CREATE TEXT SEARCH {PARSER | DITIONARY | TEMPLATE | CONFIGURATION} [ IF
NOT EXISTS ] ...
- CREATE COLLATION [ IF NOT EXISTS ] ...
My intention is cover anothers CREATE statements too, not just the above.
If has no objection about this implementation I'll finish him and soon I
sent the patch.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
[HACKERS] Revisit items marked 'NO' in sql_features.txt
Hi,
While reviewing sql_features.txt, found a few items marked NO ('Not
supported') whereas, at the outset, they seemed to be supported. Apologies,
if this is already considered and / or still marked 'NO' for a reason, but
a list of such items mentioned below:
F202TRUNCATE TABLE: identity column restart option NO
F263Comma-separated predicates in simple CASE expressionNO
T041Basic LOB data type support 01 BLOB data type NO
T051Row types NO
T174Identity columnsNO
T176Sequence generator support NO
T177Sequence generator support: simple restart option NO
T522Default values for IN parameters of SQL-invoked procedures
NO
T571Array-returning external SQL-invoked functions NO
--
Robins Tharakan
[HACKERS] Add regression tests for SET xxx
Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) (src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan regress_variable.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
[HACKERS] Add more regression tests for ALTER OPERATOR FAMILY.. ADD / DROP
Hi, Please find attached a patch to take code-coverage of ALTER OPERATOR FAMILY.. ADD / DROP (src/backend/commands/opclasscmds.c) from 50% to 87%. Any and all feedback is welcome. -- Robins Tharakan regress_opclass.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
[HACKERS] Add tests for LOCK TABLE
Hi, Please find attached a patch to take code-coverage of LOCK TABLE ( src/backend/commands/lockcmds.c) from 57% to 84%. Any and all feedback is welcome. -- Robins Tharakan regress_lock.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
[HACKERS] Add more regression tests for CREATE OPERATOR
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 regress_createoperator.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 dbcommands
Hi, Attached is an updated patch that does only 1 CREATE DATABASE and reuses that for all other tests. The code-coverage with this patch goes up from 36% to 70%. -- Robins Tharakan On 13 May 2013 21:04, Robins Tharakan thara...@gmail.com wrote: I believe Tom / Andres and Fabien all have valid points. Net-net, I believe the tests although non-negotiable, are not required to be in make-check. For now, its the slow tests that are the pain points here, and then I would soon try to prune them and commit once again. Whether it goes in make-check or not is obviously not discretion but to me their importance is undoubted since I am sure they increase code-coverage. Actually that is 'how' I create those tests, I look at untouched code and create new tests that check untouched code. Would try to revert with a faster script (preferably with minimal CREATE/DROP). -- Robins Tharakan On 13 May 2013 20:30, Andres Freund and...@2ndquadrant.com wrote: On 2013-05-13 16:52:08 +0200, Fabien COELHO wrote: Hello, Would you be okay if there is one/a few effective create/drop (some tests check that the create or drop fails e.g. depending on permissions, which ISTM is not tested anywhere else), so that tests for various ALTER DATABASE commands are combined together onto these databases? TBH, I do not see that such tests are worth adding, if they are going to significantly slow down the core regression tests. Those tests are run probably hundreds of times a day, in aggregate across all Postgres developers. Adding ten seconds or whatever this would add is a major cost, while the benefit appears trivial. We could consider adding expensive low-value tests like these to some alternate regression target that's only exercised by buildfarm members, perhaps. But I think there's probably a point of diminishing returns even in that context. I'm not sure that the tests are low value, because a commit that would generate a failure on a permission check test would be a potential security issue for Pg. As for the cost, if the proposed tests are indeed too costly, what is not necessarily the case for what I have seen, I do not think that it would be a great problem to have two set of tests, with one a superset of the other, with some convention. Well, tests like permission tests aren't the expensive part. The actual CREATE/DROP DATABASE you do is. The latter essentially are already tested by the buildfarm already. So, trimming the patch to do only the fast stuff should be less controversial? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_db_v3.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 dbcommands
I believe Tom / Andres and Fabien all have valid points. Net-net, I believe the tests although non-negotiable, are not required to be in make-check. For now, its the slow tests that are the pain points here, and then I would soon try to prune them and commit once again. Whether it goes in make-check or not is obviously not discretion but to me their importance is undoubted since I am sure they increase code-coverage. Actually that is 'how' I create those tests, I look at untouched code and create new tests that check untouched code. Would try to revert with a faster script (preferably with minimal CREATE/DROP). -- Robins Tharakan On 13 May 2013 20:30, Andres Freund and...@2ndquadrant.com wrote: On 2013-05-13 16:52:08 +0200, Fabien COELHO wrote: Hello, Would you be okay if there is one/a few effective create/drop (some tests check that the create or drop fails e.g. depending on permissions, which ISTM is not tested anywhere else), so that tests for various ALTER DATABASE commands are combined together onto these databases? TBH, I do not see that such tests are worth adding, if they are going to significantly slow down the core regression tests. Those tests are run probably hundreds of times a day, in aggregate across all Postgres developers. Adding ten seconds or whatever this would add is a major cost, while the benefit appears trivial. We could consider adding expensive low-value tests like these to some alternate regression target that's only exercised by buildfarm members, perhaps. But I think there's probably a point of diminishing returns even in that context. I'm not sure that the tests are low value, because a commit that would generate a failure on a permission check test would be a potential security issue for Pg. As for the cost, if the proposed tests are indeed too costly, what is not necessarily the case for what I have seen, I do not think that it would be a great problem to have two set of tests, with one a superset of the other, with some convention. Well, tests like permission tests aren't the expensive part. The actual CREATE/DROP DATABASE you do is. The latter essentially are already tested by the buildfarm already. So, trimming the patch to do only the fast stuff should be less controversial? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
[HACKERS] Add more regression tests for ASYNC
Hi, Patch to add more regression tests to ASYNC (/src/backend/commands/async). Take code-coverage from 39% to 75%. Any and all feedback is obviously welcome. p.s.: Please let me know whether these tests are useless or would not be committed owing to unrelated reasons. As also, if these tests need to be clubbed (bundled up in 2-3) and submitted as a single submit. -- Robins Tharakan regress_async.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
[HACKERS] Add more regression tests for dbcommands
Hi, Please find attached a patch to take code-coverage of DBCommands (CREATE DATABASE / ALTER DATABASE / DROP DATABASE) from 36% to 71%. Any and all feedback is obviously welcome. -- Robins Tharakan regress_db_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
[HACKERS] Add regression tests for DISCARD
Hi, Please find attached a patch that adds basic regression tests for DISCARD command. Any and all feedback is obviously welcome. -- Robins Tharakan regress_discard_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 regression tests for COLLATE
One workaround is to use DROP COLLATION IF EXISTS, that conveys the message without UTF8 in the message string. But three other tests use ALTER COLLATION and I see no way but to comment / disable them. Currently have them disabled (with comments as to what they do, and why they're disabled). This updated patch doesn't have UTF8 string anywhere. Let me know if you prefer to remove the commented out tests completely. -- Robins Tharakan On 9 May 2013 07:44, Tom Lane t...@sss.pgh.pa.us wrote: Robins Tharakan thara...@gmail.com writes: Fabien pointed out that currently does not check for non-trivial locales. I am still on the learning curve about LOCALEs and so, let me know if this is a show-stopper. I guess I could look at it and get back in some time with more tests as Fabien points out. You really can't, because there is no guarantee that any given machine will have anything except C and POSIX. But there's another problem: I believe this test will fail on any machine where the database is created with an encoding different from UTF8, because that encoding is named in some of the error messages in the expected output. This stuff is not easy to test in a portable way. regards, tom lane regress_collate_v3.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 regression tests for COLLATE
Hi, Please find attached the updated patch. Fabien pointed out that currently does not check for non-trivial locales. I am still on the learning curve about LOCALEs and so, let me know if this is a show-stopper. I guess I could look at it and get back in some time with more tests as Fabien points out. (Apologies for the delay though. An update to the patch was mostly done back in April, but since most of the other Code-Coverage patches (SCHEMA/ROLE/etc.) had no other feedback, I worked on all of them together just this week). -- Robins Tharakan On 12 April 2013 09:28, Michael Paquier michael.paqu...@gmail.com wrote: On Thu, Apr 11, 2013 at 4:14 PM, Robins Tharakan thara...@gmail.comwrote: Hi, Please find attached a patch to take 'make check' code-coverage of COLLATE (/src/backend/commands/collationcmds) from 0% to 96%. Any feedback is more than welcome. Also posting this to Commitfest-next. Just by having a quick look at the patch, using object names of the type cX is too generic even if the tests are done in a private schema. Why not using a name like collate_obj_X or similar? -- Michael regress_collate_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] Patch to add regression tests for SCHEMA
Hi, Please find attached an updated patch with the said changes. I'll try to update the other patches (if they pertain to this feedback) and update on their respective threads (as well as on Commitfest). -- Robins Tharakan On 8 May 2013 13:01, Fabien COELHO coe...@cri.ensmp.fr wrote: Dear Robins, Here is an updated patch that uses different schema / role names for different tests (as per commitfest site feedback). Short review about this version of the patch: This patch work for me. This test is a good thing and allows schema to be thoroughly tested, including corner cases which must fail because of errors or permissions. Two remarks: - test 2 bis: why name 'pg_asdf'? why not 'pg_schema_schsome number' to be homogeneous with other tests? - test 3: why not WHERE schema_name='schema_sch3' instead of two negative comparisons? ISTM that if for some reason in the future a new schema name is added, the test will fail. -- Fabien. regress_schema_v4.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 regression tests for ROLE (USER)
Hi, Please find an updated patch as per comments on Commitfest (comments replicated below for ease of understanding). Feedback 1: fc: role_ro2/3 used twice? rt: Corrected in this update. Feedback 2: fc: I do not understand why asdf conveys anything about an expected failure. Association of Scientists, Developers and Faculties? :-) rt: ASDF is a pattern that I learnt in one of the tests (SEQUENCE?) that pre-existed when I started working. Its a slang for arbit text that I just reused thinking that it is normal practice here. Anyway, have corrected that in this update. Feedback 3: fc: 2030/1/1 - 2030-01-01? maybe use a larger date? rt: 2030/1/1 date is not a failure point of the test. It needs to be a valid date (but sufficiently distant that so that tests don't fail). I tried setting this to 2200/1/1 and I get the same error message. Let me know if this still needs to be a large date. fb: VALID UNTIL '-12-31' works for me... rt: I thought 20 years is a date sufficiently far ahead to ensure that this test doesn't fail. Sure, have updated the test to use /1/1. Also, have added more tests at the end to ensure date-checks are also being validated in ALTER ROLE VALID UNTIL. Let me know if you need anything else changed in this. -- Robins Tharakan On 20 March 2013 03:41, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take 'make check' code-coverage of ROLE (USER) from 59% to 91%. Any feedback is more than welcome. -- Robins Tharakan regress_user_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 some regression tests for SEQUENCE
Hi, Have provided an updated patch as per Fabien's recent response on Commitfest site. Any and all feedback is appreciated. -- Robins Tharakan regress_sequence_v4.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] Patch to add regression tests for SCHEMA
Hi, Here is an updated patch that uses different schema / role names for different tests (as per commitfest site feedback). -- Robins Tharakan On 18 March 2013 17:16, robins thara...@gmail.com wrote: Hi, Attached is an updated patch that uses better schema / role names. -- Robins Tharakan On 18 March 2013 12:59, robins thara...@gmail.com wrote: Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: robins escribió: Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. I think you should use more explicit names for shared objects such as roles -- i.e. not r1 but regression_user_1 and so on. (But be careful about role names used by other tests). The issue is that these tests might be run in a database that contains other stuff; certainly we don't want to drop or otherwise affect previously existing roles. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. I think separate patches is better. Are you adding these patches to the upcoming commitfest, for evaluation? https://commitfest.postgresql.org -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_schema_v3.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] Patch to add regression tests for SCHEMA
Completely agree. Although the poster was kind enough to intimate me by email about his update there, but was wondering that if he hadn't, I wouldnt' have dreamt that there is a feedback on the site, two months down the line. -- Robins Tharakan On 8 May 2013 09:13, Robert Haas robertmh...@gmail.com wrote: On Tue, May 7, 2013 at 7:26 PM, Robins Tharakan thara...@gmail.com wrote: Here is an updated patch that uses different schema / role names for different tests (as per commitfest site feedback). I'm not sure what's going on here. Reviews are to be posted to pgsql-hackers, and then linked from the CommitFest site. Putting reviews only on the CommitFest site is bad practice. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
[HACKERS] How to use EXPLAIN (TIMING)
Hi, While creating regression tests for EXPLAIN I am (somehow) unable to get (TIMING) option to work with EXPLAIN! I must be doing something stupid but all these options below just didn't work. Could someone point me to the right direction? mpf2=# explain (TIMING) SELECT 1; ERROR: EXPLAIN option TIMING requires ANALYZE mpf2=# EXPLAIN (TIMING FALSE) SELECT 1; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) (1 row) mpf2=# EXPLAIN ANALYSE (TIMING) SELECT 1; ERROR: syntax error at or near TIMING LINE 1: EXPLAIN ANALYSE (TIMING) SELECT 1; ^ mpf2=# EXPLAIN ANALYSE (TIMING TRUE) SELECT 1; ERROR: syntax error at or near TIMING LINE 1: EXPLAIN ANALYSE (TIMING TRUE) SELECT 1; ^ mpf2=# EXPLAIN ANALYSE TIMING TRUE SELECT 1; ERROR: syntax error at or near TIMING LINE 1: EXPLAIN ANALYSE TIMING TRUE SELECT 1; ^ mpf2=# EXPLAIN ANALYSE TIMING SELECT 1; ERROR: syntax error at or near TIMING LINE 1: EXPLAIN ANALYSE TIMING SELECT 1; ^ mpf2=# EXPLAIN TIMING ANALYSE SELECT 1; ERROR: syntax error at or near TIMING LINE 1: EXPLAIN TIMING ANALYSE SELECT 1; ^ mpf2=# EXPLAIN (TIMING) ANALYSE SELECT 1; ERROR: syntax error at or near ANALYSE LINE 1: EXPLAIN (TIMING) ANALYSE SELECT 1; ^ mpf2=# EXPLAIN (TIMING) ANALYZE SELECT 1; ERROR: syntax error at or near ANALYZE LINE 1: EXPLAIN (TIMING) ANALYZE SELECT 1; ^ mpf2=# EXPLAIN (TIMING FALSE) ANALYZE SELECT 1; ERROR: syntax error at or near ANALYZE LINE 1: EXPLAIN (TIMING FALSE) ANALYZE SELECT 1; ^ mpf2=# SELECT version(); version -- PostgreSQL 9.2.3 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.4.6 20120 305 (Red Hat 4.4.6-4), 64-bit (1 row) Thanks -- Robins Tharakan
[HACKERS] Add regression tests for COLLATE
Hi, Please find attached a patch to take 'make check' code-coverage of COLLATE (/src/backend/commands/collationcmds) from 0% to 96%. Any feedback is more than welcome. Also posting this to Commitfest-next. -- Robins Tharakan regress_collate_v1.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
[HACKERS] Add regression tests for ROLE (USER)
Hi, Please find attached a patch to take 'make check' code-coverage of ROLE (USER) from 59% to 91%. Any feedback is more than welcome. -- Robins Tharakan regress_user.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] Patch to add regression tests for SCHEMA
Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: robins escribió: Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. I think you should use more explicit names for shared objects such as roles -- i.e. not r1 but regression_user_1 and so on. (But be careful about role names used by other tests). The issue is that these tests might be run in a database that contains other stuff; certainly we don't want to drop or otherwise affect previously existing roles. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. I think separate patches is better. Are you adding these patches to the upcoming commitfest, for evaluation? https://commitfest.postgresql.org -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi, Attached is an updated patch that uses better schema / role names. -- Robins Tharakan On 18 March 2013 12:59, robins thara...@gmail.com wrote: Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: robins escribió: Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. I think you should use more explicit names for shared objects such as roles -- i.e. not r1 but regression_user_1 and so on. (But be careful about role names used by other tests). The issue is that these tests might be run in a database that contains other stuff; certainly we don't want to drop or otherwise affect previously existing roles. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. I think separate patches is better. Are you adding these patches to the upcoming commitfest, for evaluation? https://commitfest.postgresql.org -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_schema_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 some regression tests for SEQUENCE
Hi, Please find an updated patch (reworked on the names of SEQUENCES / ROLES / SCHEMA etc.) Takes code-coverage of 'make check' for SEQUENCE to ~95%. -- Robins Tharakan On 16 March 2013 02:03, robins thara...@gmail.com wrote: Hi, I've added some regression tests for SEQUENCE. A cumulative patch is attached. Barring a (still to decipher) function seq_redo() and trying to learn how to actually test it, this takes care of most branches of ( src/backend/commands/sequence.c) taking code-coverage (of 'make check') to ~95%. Any feedback is more than welcome. regress_sequence_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 some regression tests for SEQUENCE
Duh. Apologies. That's what happens when you make that 1 last change. Please find an updated patch. -- Robins Tharakan On 19 March 2013 04:07, Josh Kupershmidt schmi...@gmail.com wrote: On Mon, Mar 18, 2013 at 3:10 PM, Robins Tharakan thara...@gmail.com wrote: Hi, Please find an updated patch (reworked on the names of SEQUENCES / ROLES / SCHEMA etc.) Takes code-coverage of 'make check' for SEQUENCE to ~95%. There is a typo difference between sequence.out and sequence.sql causing the test to fail: +-- Should fail since seq5 shouldn't exist ... +-- Should fail since seq5 shouldn't exit Josh regress_sequence_v3.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
[HACKERS] Patch to add regression tests for SCHEMA
Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. p.s.: I am currently working on more regression tests (USER / VIEW / DISCARD etc). Please let me know if I need to post these as one large patch, instead of submitting one patch at a time. -- Robins Tharakan regress-schema.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 some regression tests for SEQUENCE
Hi, I've added some regression tests for SEQUENCE. A cumulative patch is attached. Barring a (still to decipher) function seq_redo() and trying to learn how to actually test it, this takes care of most branches of ( src/backend/commands/sequence.c) taking code-coverage (of 'make check') to ~95%. Any feedback is more than welcome. -- Robins Tharakan On 13 March 2013 15:41, robins thara...@gmail.com wrote: Thanks Laurenz. Would correct these (and readup) before submitting next patch. -- Robins Tharakan On 13 March 2013 13:49, Albe Laurenz laurenz.a...@wien.gv.at wrote: robins wrote: Attached is a small patch to test corner cases related to Sequences (basically aimed at increasing code-coverage of sequence.sql in regression tests). Look forward to any and all feedback. Looks ok except that the patch is backwards (all added lines start with -). I found a typo: exit instead of exist. You should add the patch to the next commitfest (http://wiki.postgresql.org/wiki/Submitting_a_Patch). Yours, Laurenz Albe regress-sequence.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 some regression tests for SEQUENCE
Thanks Laurenz. Would correct these (and readup) before submitting next patch. -- Robins Tharakan On 13 March 2013 13:49, Albe Laurenz laurenz.a...@wien.gv.at wrote: robins wrote: Attached is a small patch to test corner cases related to Sequences (basically aimed at increasing code-coverage of sequence.sql in regression tests). Look forward to any and all feedback. Looks ok except that the patch is backwards (all added lines start with -). I found a typo: exit instead of exist. You should add the patch to the next commitfest (http://wiki.postgresql.org/wiki/Submitting_a_Patch). Yours, Laurenz Albe
Re: [HACKERS] Increasing code-coverage of 'make check'
Thanks Alvaro! The thought of psql_help purely because it was the easiest at that time. Since I've just begun my understanding of the code is barely negligible. I began working on SEQUENCE related tests thereafter and hopefully would move to more complicated tests in time. Peter's link is obviously helpful but since I end up doing make check ~100 of times a day, for now its useful only to cross-check how much code is uncommitted :) Robins On 11 March 2013 09:16, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think increasing coverage is a good thing. But psql help? *shrug* backend code is far more interesting and useful. Another thing to keep in mind is that there are some corner cases that are interesting to test that might not necessarily show up in a coverage chart -- for example how stuff behaves in the face of concurrent processes, or when various counters wrap around. Peter Eisentraut has set up a Jenkins instance that publishes coverage info. http://pgci.eisentraut.org/jenkins/job/postgresql_master_coverage/Coverage/ (I think he only has it running make check; doing the isolation tests probably raises percentages a bit).
[HACKERS] Add some regression tests for SEQUENCE
Hi, Attached is a small patch to test corner cases related to Sequences (basically aimed at increasing code-coverage of sequence.sql in regression tests). Look forward to any and all feedback. -- Robins Tharakan commit-sequence.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
[HACKERS] Increasing code-coverage of 'make check'
Hi, I was checking code-coverage of 'make check' and saw that the regression tests don't touch files like psql_help.c at all (read 0%). Attached is a (very small) patch to work on one ABORT help function as a sample. The reason why I post this is, to know if increasing the code-coverage (as a task) is considered important at all (to me it is). If so, I could get going with creating tests for more untouched lines / functions in 'make check'. Any other feedback is more than welcome. (The patch is wrt to yesterday's pull) --- Robins Tharakan commit-fa82e86 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
[HACKERS] Function management in PG
Hi, While making a complex database back-end, I have at-hand about 200 odd functions and frankly 'management of functions' is already getting quite tedious. Since the count is certain to rise, I am looking for a good tool to do this. By management, I guess I am looking at some kind of tagging mechanism, where it could keep stored a list of tags for each function (tags that I provide, for each new function created) and it be able to provide a comfortable way of searching/browsing through the list. Originally, I also wished that this could allow me to see whether a given function directly (or indirectly) 'can' change the database, but I guess the 'Volatile' function type suffices my need there. The problem with that is that I still can't see a function's volatility alongside a list of functions. Now making a front-end in PHP seems a few days of work, and although not impossible, I wondered whether this has already been done. How do you manage large list of functions (where a hard segregation isn't hard and therefore one can't separate functions based on schemas) ?Any pointers to a Free or Commercial tool would be of immense help. Thanks, Robins Tharakan
[HACKERS] Query Builder in pgAdmin for GSoC 2008
Hi, I read about Google Summer of Code recently and being involved with pgAdmin since a few weeks, would like to add the Query Builder feature to pgAdmin, as a part of this programme. I have been reading GSoC related emails in the past year on various PG lists, and believe that keeping a project realistic is as important as it being ambitious. My current time therefore, is spent in reading about wxWidgets and its graphical capabilities, as well as how comfortable I need to get to be able to start working on it. I understand that someone was interested in looking into the query builder at FOSDEM earlier. Since I am still into reading, I thought this probably is the right time to ask. Is anyone working on a Query Builder for pgAdmin yet ? Regards, *Robins Tharakan*
Re: [HACKERS] Query Builder in pgAdmin for GSoC 2008
Sorry! As rightly pointed out, guess I should have posted this to pgadmin-hackers instead. *Robins Tharakan* On Mon, Mar 3, 2008 at 11:19 PM, Robins Tharakan [EMAIL PROTECTED] wrote: Hi, I read about Google Summer of Code recently and being involved with pgAdmin since a few weeks, would like to add the Query Builder feature to pgAdmin, as a part of this programme. I have been reading GSoC related emails in the past year on various PG lists, and believe that keeping a project realistic is as important as it being ambitious. My current time therefore, is spent in reading about wxWidgets and its graphical capabilities, as well as how comfortable I need to get to be able to start working on it. I understand that someone was interested in looking into the query builder at FOSDEM earlier. Since I am still into reading, I thought this probably is the right time to ask. Is anyone working on a Query Builder for pgAdmin yet ? Regards, *Robins Tharakan*
