On Wed, Nov 2, 2016 at 12:48 PM, Ashutosh Sharma <ashu.coek88@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