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

RESOLVED FIXED in Bugzilla 5.0

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vladimir, Assigned: mail)

Tracking

({regression})

Dependency tree / graph
Bug Flags:
approval5.0 +
blocking5.0 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
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

4 years ago
We still support Pg 8.4.
Summary: Is Bugzilla v5.0.rc2 requires Postgres version > 9 ? → Bugzilla 5.0 fails with PostgreSQL 8.4

Comment 2

4 years ago
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

Comment 3

4 years ago
I can reproduce the issue with Pg 8.4.21.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

4 years ago
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)

Comment 6

4 years ago
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)

Comment 7

4 years ago
(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

4 years ago
(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

4 years ago
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

Updated

4 years ago
Assignee: database → simon
Assignee

Comment 10

4 years ago
Posted 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-
Assignee

Comment 12

4 years ago
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?
Posted 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+

Updated

4 years ago
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: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.