1138417 - sql_group_concat() generates bad SQL code with PostgreSQL 8.x

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Vladimir]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150224134236

Steps to reproduce:

I have installed Bugzilla 5.0 rc2 with Postgres 8.4. Requiremets says: "You need PostgreSQL version 8.03.0000 or higher."

I get DB-errors while trying to see the bug.

Bugzilla 5.0 rc2 works fine with Postgres 9.4.


Actual results:

I try to see the bug (after creating) and catch  DB errors:

An error occurred while performing a database operation:

DBD::Pg::db selectall_arrayref failed: ERROR:  syntax error at or near "ORDER"
LINE 1: SELECT comment_id, ARRAY_TO_STRING(ARRAY_AGG(tag ORDER BY ta...
                                                         ^ [for Statement "SELECT comment_id, ARRAY_TO_STRING(ARRAY_AGG(tag ORDER BY tag), ',')
               FROM longdescs_tags
              WHERE  comment_id IN (2) 
              GROUP BY comment_id"] at Bugzilla/Comment.pm line 162.
	Bugzilla::Comment::preload('Bugzilla::Comment', 'ARRAY(0x5d97350)') called at Bugzilla/Bug.pm line 3630
	Bugzilla::Bug::comments('Bugzilla::Bug=HASH(0x5d97a28)') called at Bugzilla/Bug.pm line 535
	Bugzilla::Bug::_preload_referenced_bugs('Bugzilla::Bug=HASH(0x5d97a28)') called at Bugzilla/Bug.pm line 517
	Bugzilla::Bug::preload('Bugzilla::Bug', 'ARRAY(0x554ac28)') called at /var/bugzilla/show_bug.cgi line 94




Expected results:

Bugzilla 5.0 rc2 works fine with Postgres 9.4.

I think you need to change DB Requirements or use old SQL syntax.

Comment 1

[Frédéric Buclin]

We still support Pg 8.4.

Comment 2

[Frédéric Buclin]

The code which causes trouble is in Bugzilla/DB/Pg.pm, in sql_group_concat():

    if (vers_cmp($self->bz_server_version, 9) < 0) {
        # PostgreSQL 8.x doesn't support STRING_AGG
        return "ARRAY_TO_STRING(ARRAY_AGG($text$order_by), $separator)";
    }

You will note that this code is specific to Pg 8.x.

Comment 3

[Frédéric Buclin]

I can reproduce the issue with Pg 8.4.21.

Comment 4

[Frédéric Buclin]

The PostgreSQL documentation says that no 8.x release supports ORDER BY inside ARRAY_AGG(). This documentation also says that Pg 8.3 doesn't support ARRAY_AGG() at all. So the code for 8.x is totally bogus.

Comment 5

[:glob ✱]

mtyson - is this something you'll be able to look at?

Comment 6

[Matt Tyson]

I'm not sure this can be done in postgres 8.4 without knowing the name of the table.

EG, this will work, but note that I've hardcoded the table name.

return "ARRAY_TO_STRING((SELECT ARRAY_AGG($text) FROM (SELECT $text FROM longdescs_tags $order_by) as foobar), $separator)"

Comment 7

[Marko Tiikkaja]

(In reply to Matt Tyson from comment #6)
> I'm not sure this can be done in postgres 8.4 without knowing the name of
> the table.
> 
> EG, this will work, but note that I've hardcoded the table name.
> 
> return "ARRAY_TO_STRING((SELECT ARRAY_AGG($text) FROM (SELECT $text FROM
> longdescs_tags $order_by) as foobar), $separator)"

That's somewhat bogus; pedantically speaking the order of the subquery is not guaranteed to be preserved.

But there's no GROUP BY or other aggregates here so there's no need to use array_agg() in the first place.  Just do:

array_to_string(ARRAY(SELECT $text FROM longdescs_tags $order_by), $separator)

Comment 8

[Matt Tyson]

(In reply to Marko Tiikkaja from comment #7)
> That's somewhat bogus; pedantically speaking the order of the subquery is
> not guaranteed to be preserved.
> 
> But there's no GROUP BY or other aggregates here so there's no need to use
> array_agg() in the first place.  Just do:
> 
> array_to_string(ARRAY(SELECT $text FROM longdescs_tags $order_by),
> $separator)

That looks a bit better, but the real problem is the requirement of the table name in the query.

In the call to sql_group_concat we don't have that information.

Comment 9

[Frédéric Buclin]

Don't waste your time. Back out this patch from Bugzilla 5.0, and for Bugzilla 6.0, bump the min version to Pg 9.0 and remove the buggy code about Pg 8.x. Either that, or simply ignore ORDER BY for Pg 8.x.

Comment 10

[Simon Green]

Attachment: bug1138417-v1.patch

I have not tested this patch beyond checking it passes runtests.pl, as I don't have access to a Postgres 8 server. Basically it reverts to the 4.4 way of doing things if Pg < 9.0. It means the flags won't be ordered correct in buglist.cgi, but it was like that for 4.4 anyway :)

Comment 11

[Frédéric Buclin]

Comment on attachment 8588423 [details] [diff] [review]
bug1138417-v1.patch

>+++ b/Bugzilla/DB/Pg.pm

>+    # PostgreSQL 8.x doesn't support STRING_AGG
>+    if (vers_cmp($self->bz_server_version, 9) < 0) {
>+        my $sql = "ARRAY_ACCUM($text)";
>+        if ($sort) {
>+            $sql = "ARRAY_SORT($sql)";
>+        }
>+        return "ARRAY_TO_STRING($sql, $separator)";
>+    }

ARRAY_ACCUM and ARRAY_SORT aren't builtin functions either, see the original code in DB/Pg.pm. You must restore their definitions.

Comment 12

[Simon Green]

Attachment: patch for 5.0, v2

Comment 13

[Frédéric Buclin]

Comment on attachment 8588513 [details] [diff] [review]
patch for 5.0, v2

This patch must only land in 5.0. For master, you need a separate patch where you remove the Pg 8.x specific code entirely, and bump the min version of Pg to 9.0.

Comment 14

[Frédéric Buclin]

Comment on attachment 8588513 [details] [diff] [review]
patch for 5.0, v2

Tested with Pg 8.4.20 on CentOS 6.6 and with Pg 9.3.6 on Mageia 4. r=LpSolit

Comment 15

[Frédéric Buclin]

For 5.0 only.

Comment 16

[Frédéric Buclin]

Attachment: patch for 5.1, v1

This patch bumps the min version of Pg to 9.0, for master only.

Comment 17

[:glob ✱]

Comment on attachment 8589650 [details] [diff] [review]
patch for 5.1, v1

please create a new bug to track this change on trunk.

Comment 18

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   fe7e0d0..665c59e  5.0 -> 5.0