Maybe-issue on version check in tests

[jcarnu]

This line checks versions between json file and actual pg version being tested

https://github.com/tds-fdw/tds_fdw/blob/1c38147331b5795348247294bb0f4fbd366def14/tests/lib/tests.py#L54

It fails "on my machine" with an exception :

[ERROR] '<=' not supported between instances of 'list' and 'str'
Traceback (most recent call last):
  File "postgresql-tests.py", line 127, in <module>
    main()
  File "postgresql-tests.py", line 124, in main
    raise(e)
  File "postgresql-tests.py", line 110, in main
    args.debugging, args.unattended_debugging)
  File "/home/jc/dev/gh/tds_fdw/tests/lib/tests.py", line 125, in run_tests
    if check_ver(conn, req_ver['min'], req_ver['max'], dbtype):
  File "/home/jc/dev/gh/tds_fdw/tests/lib/tests.py", line 54, in check_ver
    if server_ver >= min_ver and (server_ver <= max_ver or min_ver == ''):
TypeError: '<=' not supported between instances of 'list' and 'str'

If I understand quite correctly this statement, you want to test if current version of PG will be tested ok on the current test.

The point is that if max_ver is '' (that is often), it is evaluated as empty string.

The other point is that max_ver should only be tested if defined max_ver is define, not min_ver.... as far I understand what you want to check.

The final point is that min_ver should also be tested as non empty before being tested against actual PG version. This should not happen often but it prevent any misuse.

if (min_ver ='' or server_ver >= min_ver) and (max_ver == '' or server_ver <= max_ver ):

Any thought @juliogonzalez ?

[juliogonzalez]

If I understand quite correctly this statement, you want to test if current version of PG will be tested ok on the current test.

It's valid for both PostgreSQL and MSSQL, but yes.

The tests have a mandatory min_ver required, and an optional (empty string in this case) max_ver required.

if server_ver >= min_ver and (server_ver <= max_ver or min_ver == ''): 

As you correctly guessed, that's a typo, it should be:

if server_ver >= min_ver and (server_ver <= max_ver or max_ver == ''): 

Sending a PR to fix it.

Let's not add the check for min_ver =='' here, as that's not a valid value.

To take care of this, we should have a JSON validator for the .json files that cancels the test if it is not valid.

[juliogonzalez]

BTW: If you don't want to specify a PostgreSQL min_ver, use 9.2 as that is the minimum version tds_fdw supports (not sure if now the support is broken now at master, but it was working not long ago).

I should probable create a doc about how to create tests :-)

[jcarnu]

Maybe would it be interesting to have a "default pg_min_ver" at the before-eol version of Pg (for the Pg one :-) ?)

This way you'll can provide the min version in one place?

This may be relevant only if you intend to stick to postgresql revision policy for testing.

I agree this may not be relevant if you plan to be backward compatible with earlier version.

My2¢

[juliogonzalez]

Let's say that we should try to stay backward compatible and avoid breaking stuff on purpose, when possible.

We need to keep in mind that while we provide instructions only for the official PostgreSQL packages, the concept "PostgreSQL X.Y is EoL" means the PostgreSQL development group declared it as such... but the GNU/Linux distributions out there are using such EoL versions, and are supported by the distributions themselves.

One example, from CentOS7:

[root@bef3f3cc8738 /]# yum info postgresql.x86_64
Loaded plugins: fastestmirror, ovl
Loading mirror speeds from cached hostfile
 * base: ftp.cica.es
 * extras: ftp.cica.es
 * updates: ftp.cica.es
Available Packages
Name        : postgresql
Arch        : x86_64
Version     : 9.2.24
Release     : 1.el7_5
Size        : 3.0 M
Repo        : base/7/x86_64
Summary     : PostgreSQL client programs
URL         : http://www.postgresql.org/
License     : PostgreSQL
Description : PostgreSQL is an advanced Object-Relational database management system (DBMS).
            : The base postgresql package contains the client programs that you'll need to
            : access a PostgreSQL DBMS server, as well as HTML documentation for the whole
            : system.  These client programs can be located on the same machine as the
            : PostgreSQL server, or on a remote machine that accesses a PostgreSQL server
            : over a network connection.  The PostgreSQL server can be found in the
            : postgresql-server sub-package.

CentOS7 has support until 2024.

CentOS8 (just released):

[root@12148df9468f /]# dnf info postgresql
Failed to set locale, defaulting to C
CentOS-8 - AppStream                                                                                                                                                                                                                           512 kB/s | 6.0 MB     00:12    
CentOS-8 - Base                                                                                                                                                                                                                                2.0 MB/s | 7.9 MB     00:04    
CentOS-8 - Extras                                                                                                                                                                                                                              620  B/s | 2.1 kB     00:03    
Last metadata expiration check: 0:00:01 ago on Fri Oct 18 16:15:52 2019.
Available Packages
Name         : postgresql
Version      : 10.6
Release      : 1.module_el8.0.0+15+f57f353b
Arch         : x86_64
Size         : 1.5 M
Source       : postgresql-10.6-1.module_el8.0.0+15+f57f353b.src.rpm
Repo         : AppStream
Summary      : PostgreSQL client programs
URL          : http://www.postgresql.org/
License      : PostgreSQL
Description  : PostgreSQL is an advanced Object-Relational database management system (DBMS).
             : The base postgresql package contains the client programs that you'll need to
             : access a PostgreSQL DBMS server, as well as HTML documentation for the whole
             : system.  These client programs can be located on the same machine as the
             : PostgreSQL server, or on a remote machine that accesses a PostgreSQL server
             : over a network connection.  The PostgreSQL server can be found in the
             : postgresql-server sub-package.

CentOS8 will stay with 10.6 until it goes EoL on 2029.

Same for the enterprise RHEL counterpart.

Needless to say, if we find out that 9.2 is broken already, then we need to push all the tests to 9.3, or the oldest version that still works.

[jcarnu]

Okaye, you are right and I was unclear :

My point was pure programming one, not dealing with PG release cycle rather than define a "fallback" min_version for pg (and why not mssql) somewhere in order not to have mistakes in tests.

Anyway, that was a small and un necessary enhancement :)

Sorry for the noise !

[juliogonzalez]

No, I agree it's a necessary enhancement.

We really should have a validator for the JSON files. And I should really create a doc about how to create new tests. I will create a card for that.