Bug Hunting Session
Bug 32960 - Query composer does not alias derived tables; breaks MySQL, PostgreSQL
Summary: Query composer does not alias derived tables; breaks MySQL, PostgreSQL
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
3.3.0 RC2
Hardware: All All
: high major
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks: 33240 32964
  Show dependency treegraph
 
Reported: 2011-01-10 05:50 UTC by Lionel Elie Mamane
Modified: 2011-01-20 01:58 UTC (History)
0 users

See Also:
Crash report or crash signature:


Attachments
patch for "invalid SQL" issue (1.53 KB, patch)
2011-01-18 05:40 UTC, Lionel Elie Mamane
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2011-01-10 05:50:03 UTC
When a report has the property "analyze SQL command" set to "No", the actual SQL sent to the database is something like:

SELECT * FROM ( ${QUERY} ) ...

Where "..." is for example an ORDER BY clause
This does not work with PostgreSQL (sdbc driver).

in http://archives.postgresql.org/pgsql-sql/2007-12/msg00002.php, the PostgreSQL developers say this is invalid ISO SQL; it needs to be something like:


SELECT * FROM ( ${QUERY} ) AS some_name ...

I suggest you just put something like "AS __libreoffice_tmp_${RANDOM_NUMBER}"
Comment 1 Lionel Elie Mamane 2011-01-12 22:28:43 UTC
I reproduced this bug in different circumstances:
 - MySQL, not only PostgreSQL
 - "analyze SQL command" set to "Yes"
 - The data source (property "Content") set to e.g. "SELECT foo, bar FROM qux WHERE baz=0" (the presence of a WHERE clause is of essence)

Then, when one executes the report, it fails with MySQL error message "Every derived table must have its own alias". That is because LibreOffice sends the following request to MySQL:

SELECT * FROM (SELECT foo, bar FROM qux WHERE baz=0) WHERE 0=1

Again, this should be:

SELECT * FROM (SELECT foo, bar FROM qux WHERE baz=0) AS SOME_NAME WHERE 0=1

As this bug happens not only with PostgreSQL, but also with MySQL, and more importantly since it breaks also the "analyze SQL command" case as soon as there is a WHERE clause in the query, I raised its severity.
Comment 2 Lionel Elie Mamane 2011-01-13 15:40:51 UTC
Reconfirmed with RC3 (package from "experimental" section of Debian)
Comment 3 Lionel Elie Mamane 2011-01-15 23:45:23 UTC
I get this bug only with gcj, not with OpenJDK. The cause for the different behaviour is in function fillOrderStatement of reportbuilder/java/com/sun/star/report/SDBCReportDataFactory.java:

            final XSingleSelectQueryComposer composer = getComposer(tools, command, commandType);
            if (composer != null)
            {
Comment 4 Lionel Elie Mamane 2011-01-16 00:02:45 UTC
I get this bug only with gcj, not with OpenJDK. The cause for the different
behaviour is in function fillOrderStatement of reportbuilder/java/com/sun/star/report/SDBCReportDataFactory.java:

  final XSingleSelectQueryComposer composer = getComposer(tools, command, commandType);
  if (composer != null)
  {
    // This branch is taken by OpenJDK
    (...)
  }
  else
  {
    // This branch is taken by gcj
    (...)
    statement = "SELECT * FROM (" + command + ")"
    (...)
  }

This probably means:

 - It should have lower priority / severity
 - This is not a regression: I probably tested OO.org 3.2 with OpenJDK...
 - Does not fit the release blocker criteria.

After this bug and bug#32964, are corrected, IMHO thee "analyze SQL: no" case (EscapeProcessing = false) should _always_ take the second branch (or do a similar thing from within composer): this allows to add an "order by" clause to an SQL command without parsing it, and thus to use a EscapeProcessing=false SQL command in a report that uses grouping. An alternative would be _not_ to add the ORDER BY clause at all, thus having the query still go through successfully, and rely on the user to put the ORDER BY clause that fits the grouping done by the report himself in the SQL command.
Comment 5 Michael Meeks 2011-01-17 02:49:05 UTC
ok - thanks for the analysis ! it is encouraging that this is gcj related. If the functionality changes depending on Java run-time, this could well be a gcj bug unrelated to LibreOffice anyway :-)

So - un-marking as a blocker; great job debugging it though, any chance you can roll the debugging a few frames further back to see why getComposer is returning NULL ?

Also - if this branch produces broken output, would we be better off with some sort of thrown exception there ? and/or is there a clean code fix for that branch ?

Thanks !
Comment 6 Lionel Elie Mamane 2011-01-18 05:20:55 UTC
(In reply to comment #5)

> So - un-marking as a blocker; great job debugging it though, any chance you can
> roll the debugging a few frames further back to see why getComposer is
> returning NULL ?

I haven't managed to get gdb to give me anything meaningful as far as Java code is concerned, nor to set a breakpoint in java code. I'd appreciate a pointer to a tutorial on how to debug Java code, generally or in OpenOffice.org/LibreOffice if that's any different.

http://gcc.gnu.org/java/gdb.html says gdb supports Java since 5.x days, but the documentation of 7.x is mum about Java; it is not listed as a supported language.

Also, my dev build suddenly refuses to use gcj for some reason, I have to investigate why.

> Also - if this branch produces broken output, would we be better off with some
> sort of thrown exception there ? and/or is there a clean code fix for that
> branch ?

This branch (or the same behaviour implemented within composer or somewhere else) has the potential, once bug #32964 is fixed, to allow a fix for bug #33240, so I wouldn't like to see it dropped completely.
Comment 7 Lionel Elie Mamane 2011-01-18 05:40:29 UTC
Created attachment 42160 [details]
patch for "invalid SQL" issue

So, there are two ways to fix this:
 1) Fix the fact that "composer" is NULL with gcj (which might be a gcj bug)
 2) Give the derived table (subquery) an alias (name).

IMHO the fact that we do the first should not keep us from doing the second, for added robustness.

The attached patch does the second.
Comment 8 Don't use this account, use tml@iki.fi 2011-01-20 01:58:30 UTC
Thanks for the patch, pushed to master. Resolving bug, please reopen if you find that the patch is not enough after all.