Hi, I have started with the review for this patch and would like to share some of my initial review comments that requires author's attention.
1) I am getting some trailing whitespace errors when trying to apply this patch using git apply command. Following are the error messages i am getting. [edb@localhost postgresql]$ git apply test_pg_stat_statements-v1.patch test_pg_stat_statements-v1.patch:28: trailing whitespace. $(MAKE) -C $(top_builddir)/src/test/regress all test_pg_stat_statements-v1.patch:41: space before tab in indent. $(REGRESSCHECKS) warning: 2 lines add whitespace errors. 2) The test-case you have added is failing that is because queryid is going to be different every time you execute the test-case. I think it would be better to remove the queryid column from "SELECT queryid, query, calls, rows from pg_stat_statements ORDER BY rows". Below is the output i got upon test-case execution. make regresscheck ============== running regression test queries ============== test pg_stat_statements ... FAILED ============== shutting down postmaster ============== 3) Ok. As you mentioned upthread, I do agree with the fact that we can't add pg_stat_statements tests for installcheck as this module requires shared_preload_libraries to be set. But, if there is an already existing installation with pg_stat_statements module loaded we should allow the test to run on this installation if requested explicitly by the user. I think we can add some target like 'installcheck-force' in the MakeFile that would help the end users to run the test on his own installation. Thoughts? Also, I am changed the status in the commit-fest from "Needs review" to "waiting on Author". On Mon, Sep 19, 2016 at 6:26 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund <and...@anarazel.de> wrote: >>> On 2016-08-30 07:57:19 +0530, Amit Kapila wrote: >>>> I will write such a test case either in this week or early next week. >>> >>> Great. >>> >> >> Okay, attached patch adds some simple tests for pg_stat_statements. >> One thing to note is that we can't add pg_stat_statements tests for >> installcheck as this module requires shared_preload_libraries to be >> set. Hope this suffices the need. >> > > Registered this patch for next CF. > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com > > > -- > Sent via pgsql-hackers mailing list (firstname.lastname@example.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers