Re: Fix proposal for comparaison bugs in PostgreSQL::Version

On 2022-06-28 Tu 16:53, Jehan-Guillaume de Rorthais wrote:
> Hi,
>
> I found a comparaison bug when using the PostgreSQL::Version module. See:
>
> $ perl -I. -MPostgreSQL::Version -le '
> my $v = PostgreSQL::Version->new("9.6");
>
> print "not 9.6 > 9.0" unless $v > 9.0;
> print "not 9.6 < 9.0" unless $v < 9.0;
> print "9.6 <= 9.0" if $v <= 9.0;
> print "9.6 >= 9.0" if $v >= 9.0;'
> not 9.6 > 9.0
> not 9.6 < 9.0
> 9.6 <= 9.0
> 9.6 >= 9.0
>
> When using < or >, 9.6 is neither greater or lesser than 9.0.
> When using <= or >=, 9.6 is equally greater and lesser than 9.0.
> The bug does not show up if you compare with "9.0" instead of 9.0.
> This bug is triggered with devel versions, eg. 14beta1 <=> 14.
>
> The bug appears when both objects have a different number of digit in the
> internal array representation:
>
> $ perl -I. -MPostgreSQL::Version -MData::Dumper -le '
> print Dumper(PostgreSQL::Version->new("9.0")->{num});
> print Dumper(PostgreSQL::Version->new(9.0)->{num});
> print Dumper(PostgreSQL::Version->new(14)->{num});
> print Dumper(PostgreSQL::Version->new("14beta1")->{num});'
> $VAR1 = [ '9', '0' ];
> $VAR1 = [ '9' ];
> $VAR1 = [ '14' ];
> $VAR1 = [ '14', -1 ];
>
> Because of this, The following loop in "_version_cmp" is wrong because we are
> comparing two versions with different size of 'num' array:
>
> for (my $idx = 0;; $idx++)
> {
> return 0 unless (defined $an->[$idx] && defined $bn->[$idx]);
> return $an->[$idx] <=> $bn->[$idx]
> if ($an->[$idx] <=> $bn->[$idx]);
> }
>
>
> If we want to keep this internal array representation, the only fix I can think
> of would be to always use a 4 element array defaulted to 0. Previous examples
> would be:
>
> $VAR1 = [ 9, 0, 0, 0 ];
> $VAR1 = [ 9, 0, 0, 0 ];
> $VAR1 = [ 14, 0, 0, 0 ];
> $VAR1 = [ 14, 0, 0, -1 ];
>
> A better fix would be to store the version internally as version_num that are
> trivial to compute and compare. Please, find in attachment an implementation of
> this.
>
> The patch is a bit bigger because it improved the devel version to support
> rc/beta/alpha comparison like 14rc2 > 14rc1.
>
> Moreover, it adds a bunch of TAP tests to check various use cases.

Nice catch, but this looks like massive overkill. I think we can very
simply fix the test in just a few lines of code, instead of a 190 line
fix and a 130 line TAP test.

It was never intended to be able to compare markers like rc1 vs rc2, and
I don't see any need for it. If you can show me a sane use case I'll
have another look, but right now it seems quite unnecessary.

Here's my proposed fix.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com