Closed Bug 1138417 Opened 6 years ago Closed 6 years ago

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

Categories

(Bugzilla :: Database, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: vladimir, Assigned: mail)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
We still support Pg 8.4.
Summary: Is Bugzilla v5.0.rc2 requires Postgres version > 9 ? → Bugzilla 5.0 fails with PostgreSQL 8.4
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.
Assignee: installation → database
Severity: normal → critical
Component: Installation & Upgrading → Database
Depends on: 936275
Flags: blocking5.0?
Keywords: regression
Summary: Bugzilla 5.0 fails with PostgreSQL 8.4 → sql_group_concat() generates bad SQL code with PostgreSQL 8.x
Target Milestone: --- → Bugzilla 5.0
I can reproduce the issue with Pg 8.4.21.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: blocking5.0? → blocking5.0+
mtyson - is this something you'll be able to look at?
Flags: needinfo?(mtyson)
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)"
Flags: needinfo?(mtyson)
(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)
(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.
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.
Assignee: database → simon
Attached patch bug1138417-v1.patch (obsolete) — Splinter Review
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 :)
Attachment #8588423 - Flags: review?(dylan)
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.
Attachment #8588423 - Flags: review?(dylan) → review-
Attachment #8588423 - Attachment is obsolete: true
Attachment #8588513 - Flags: review?(dylan)
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.
Attachment #8588513 - Attachment description: bug1138417-v2.patch → patch for 5.0, v2
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
Attachment #8588513 - Flags: review?(dylan) → review+
For 5.0 only.
Status: NEW → ASSIGNED
Flags: approval5.0?
Attached patch patch for 5.1, v1 (obsolete) — Splinter Review
This patch bumps the min version of Pg to 9.0, for master only.
Attachment #8589650 - Flags: review?(glob)
Comment on attachment 8589650 [details] [diff] [review]
patch for 5.1, v1

please create a new bug to track this change on trunk.
Attachment #8589650 - Flags: review?(glob)
Flags: approval5.0? → approval5.0+
Attachment #8589650 - Attachment is obsolete: true
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   fe7e0d0..665c59e  5.0 -> 5.0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.