Add some tests for pg_stat_statements compatibility verification under contrib

Hi,

On Tue, Mar 09, 2021 at 11:35:14AM +0800, Erica Zhang wrote:

Hi All,
On the master branch, it is possible to install multiple versions of pg_stat_statements with CREATE EXTENSION, but all the tests in sql/ on look at the latest version available, without testing past compatibility.

Since we support to install lowest version 1.4 currently, add some tests to verify compatibility, upgrade from lower versions of pg_stat_statements.

The upgrade scripts are already tested as postgres will install 1.4 and perform
all upgrades to reach the default version.

But an additional thing being tested here is the ABI compatibility when there's
a mismatch between the library and the SQL definition, which seems like a
reasonable thing to test.

Looking at the patch:

+SELECT * FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4';

What is this supposed to test? All those tests will break every time we change
the default version, which will add maintenance efforts. It could be good to
have one test breaking when changing the version to remind us to add a test for
the new version, but not more.

Hi Erica,

On Wed, Mar 10, 2021 at 11:14:52AM +0800, Erica Zhang wrote:

Hi Julien,
Thanks a lot for the quick review. Please see my answer below in blue. Attached is the new patch.

Thanks!

The upgrade scripts are already tested as postgres will install 1.4 and perform
all upgrades to reach the default version.

Thanks for pointing that the upgrades paths are covered by upgrade scripts tests. Since I don't need to test the upgrade, I will test the installation of different versions directly, any concern?

I think you should keep your previous approach. The result will be the same
but it will consume less resources for that which is always good.

+SELECT * FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4';

What is this supposed to test?  All those tests will break every time we change
the default version, which will add maintenance efforts.  It could be good to
have one test breaking when changing the version to remind us to add a test for
the new version, but not more.

Here I just want to verify that "installed" version is the expected version. But we do have the issue as you mentioned which will add maintenance efforts.

So I prefer to keep one test as now which can remind us to add a new version. As for others, just to check the count(*) to make sure installation is success.
Such as SELECT count(*) FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4'; What do you think?

How about tweaking your previous query so only the last execution fails when
pg_stat_statements default version is updated? Something like:

SELECT installed_version = default_version, installed_version
FROM pg_available_extensions
WHERE name = 'pg_stat_statements';

This way the same query can be reused for both older versions and current
version.

Also, can you register your patch for the next commitfest at
https://commitfest.postgresql.org/33/, to make sure it won't be forgotten?

Hi Julien,
Thanks a lot for the quick review. Please see my answer below in blue. Attached is the new patch.

------------------ Original ------------------
From: "Julien Rouhaud" <rjuju123@gmail.com&gt;;
Date:&nbsp;Tue, Mar 9, 2021 05:09 PM
To:&nbsp;"Erica Zhang"<ericazhangy@qq.com&gt;;
Cc:&nbsp;"pgsql-hackers"<pgsql-hackers@postgresql.org&gt;;
Subject:&nbsp;Re: Add some tests for pg_stat_statements compatibility verification under contrib

Hi,

On Tue, Mar 09, 2021 at 11:35:14AM +0800, Erica Zhang wrote:
&gt; Hi All,
&gt; On the master branch, it is possible to install multiple versions of pg_stat_statements with CREATE EXTENSION, but all the tests in sql/ on look at the latest version available, without testing past compatibility.
&gt;
&gt; Since we support to install lowest version 1.4 currently, add some tests to verify compatibility, upgrade from lower versions of pg_stat_statements.

The upgrade scripts are already tested as postgres will install 1.4 and perform
all upgrades to reach the default version.
Thanks for pointing that the upgrades paths are covered by upgrade scripts tests. So I don't need to verify the upgrade, I will test the installation of different versions directly, any concern?

But an additional thing being tested here is the ABI compatibility when there's
a mismatch between the library and the SQL definition, which seems like a
reasonable thing to test.

Looking at the patch:

+SELECT * FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4';

What is this supposed to test?&nbsp; All those tests will break every time we change
the default version, which will add maintenance efforts.&nbsp; It could be good to
have one test breaking when changing the version to remind us to add a test for
the new version, but not more.
Here I just want to verify that "installed" version is the expected version. But we do have the issue as you mentioned which will add maintenance efforts.

So I prefer to keep one test as now which can remind us to add a new version. As for others, just to check the count(*) to make sure installation is success.
Such as SELECT count(*) FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4'; What do you think?

Hi Julien,

------------------&nbsp;Original&nbsp;------------------
From: "Julien Rouhaud" <rjuju123@gmail.com&gt;;
Date:&nbsp;Wed, Mar 10, 2021 11:35 AM
To:&nbsp;"Erica Zhang"<ericazhangy@qq.com&gt;;
Cc:&nbsp;"pgsql-hackers"<pgsql-hackers@postgresql.org&gt;;
Subject:&nbsp;Re: Add some tests for pg_stat_statements compatibility verification under contrib

Hi Erica,

On Wed, Mar 10, 2021 at 11:14:52AM +0800, Erica Zhang wrote:
&gt; Hi Julien,
&gt; Thanks a lot for the quick review. Please see my answer below in blue. Attached is the new patch.

Thanks!

&gt;&gt; The upgrade scripts are already tested as postgres will install 1.4 and perform
&gt;&gt; all upgrades to reach the default version.
&gt; Thanks for pointing that the upgrades paths are covered by upgrade scripts tests. Since I don't need to test the upgrade, I will test the installation of different versions directly, any concern?

I think you should keep your previous approach.&nbsp; The result will be the same
but it will consume less resources for that which is always good.
Agreed!

&gt;&gt; +SELECT * FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4';
&gt;&gt;
&gt;&gt;
&gt;&gt; What is this supposed to test?&amp;nbsp; All those tests will break every time we change
&gt;&gt; the default version, which will add maintenance efforts.&amp;nbsp; It could be good to
&gt;&gt; have one test breaking when changing the version to remind us to add a test for
&gt;&gt; the new version, but not more.
&gt; Here I just want to verify that "installed" version is the expected version. But we do have the issue as you mentioned which will add maintenance efforts.
&gt;
&gt; So I prefer to keep one test as now which can remind us to add a new version. As for others, just to check the count(*) to make sure installation is success.
&gt; Such as SELECT count(*) FROM pg_available_extensions WHERE name = 'pg_stat_statements' and installed_version = '1.4'; What do you think?

How about tweaking your previous query so only the last execution fails when
pg_stat_statements default version is updated?&nbsp; Something like:

SELECT installed_version = default_version, installed_version
FROM pg_available_extensions
WHERE name = 'pg_stat_statements';

This way the same query can be reused for both older versions and current
version.
Yep, it's neater to use the query as you suggested. Thanks!

Also, can you register your patch for the next commitfest at
https://commitfest.postgresql.org/33/, to make sure it won't be forgotten?

On Mon, Mar 15, 2021 at 03:05:24PM +0800, Erica Zhang wrote:

This way the same query can be reused for both older versions and current
version.
Yep, it's neater to use the query as you suggested. Thanks!

Also, can you register your patch for the next commitfest at
https://commitfest.postgresql.org/33/, to make sure it won't be forgotten?

I was just looking at your patch, and I think that you should move all
the past compatibility tests into a separate test file, in a way
consistent to what we do in contrib/pageinspect/ for
oldextversions.sql. I would suggest to use the same file names, while
on it.
--
Michael

On Wed, Aug 25, 2021 at 04:16:08PM +0900, Michael Paquier wrote:

I was just looking at your patch, and I think that you should move all
the past compatibility tests into a separate test file, in a way
consistent to what we do in contrib/pageinspect/ for
oldextversions.sql. I would suggest to use the same file names, while
on it.

The current commit fest is ending, and it would be a waste to do
nothing here, so I have looked at what you proposed and reworked it.
The patch was blindly testing pg_stat_statements_reset() in all the
versions bumped with the same query on pg_stat_statements done each
time, which does not help in checking the actual parts of the code
that have changed, and there are two of them:
- pg_stat_statements_reset() execution got authorized for
pg_read_all_stats once in 1.6.
- pg_stat_statements() has been extended in 1.8, so we could just have
one query stressing this function in the tests for <= 1.7.

There is also no need for tests on 1.9, which is the latest version.
Tests for this one should be added once we bump the code to the next
version. At the end I finish with the attached, counting for the
back-and-forth game with pg_read_all_stats.
--
Michael

On Thu, Sep 30, 2021 at 11:12:21AM +0900, Michael Paquier wrote:

There is also no need for tests on 1.9, which is the latest version.
Tests for this one should be added once we bump the code to the next
version. At the end I finish with the attached, counting for the
back-and-forth game with pg_read_all_stats.

Done as of 2b0da03.
--
Michael