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.


> 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.
> 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.

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

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Attachment: 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:

Reply via email to