On Wed, Nov 2, 2016 at 12:48 PM, Ashutosh Sharma <ashu.coe...@gmail.com> wrote: > 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. >
Thanks. > 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. > Fixed. > 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. > It shouldn't be different each time you execute test as this is a hash code, but I agree that it is not stable and it can vary across platforms, so removed it from test. > > 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. > I had also thought about it while preparing patch, but I couldn't find any clear use case. I think it could be useful during development of a module, but not sure if it is worth to add a target. I think there is no harm in adding such a target, but lets take an opinion from committer or others as well before adding this target. What do you say? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
test_pg_stat_statements-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
