Bug 114755 - SQL error: extra parentheses
Summary: SQL error: extra parentheses
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Julien Nabet
URL:
Whiteboard: target:6.1.0 target:6.0.0.2
Keywords:
Depends on:
Blocks: Database-Firebird-Default
  Show dependency treegraph
 
Reported: 2017-12-29 19:31 UTC by Terrence Enger
Modified: 2018-01-01 15:13 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
example database (3.83 KB, application/vnd.oasis.opendocument.database)
2017-12-29 19:31 UTC, Terrence Enger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Terrence Enger 2017-12-29 19:31:57 UTC
Created attachment 138737 [details]
example database

STR
(1) Download and open attached a02.fdb.  It is an embedded Firebird
    database with an empty table.
(2) In Database pane click <Tables>.
(3) In Tables pane, right-click Table1.
(4) In pop-up menu select Copy.
(5) Right-click on background of Tables pane.
(6) In pop-up menu, right-click Paste.
(7) In dialog "Copy table" click button <Next>>.
(8) In dialog "Apply columns", click button ">>". and then click
    button <Next>>.
(9) In dialog "Type formatting", click button <Create>.  Observe
    unexpected terminal message (first line rewrapped):

        warn:connectivity.firebird:12371:1:
            connectivity/source/drivers/firebird/Util.cxx:52:
            firebird_sdbc error:
        *Dynamic SQL Error
        *SQL error code = -104
        *Token unknown - line 1, column 46
        *(
        caused by
        'isc_dsql_prepare'

A backtrace reveals that the SQL statement is:

    INSERT INTO "Table12" ( "cardinal","words" )
    ( SELECT "cardinal", "words" FROM "Table1" )


Note that Table1 is empty.  When Table1 has data, there is an
additional terminal message:

    warn:connectivity.firebird:12526:1:
        connectivity/source/drivers/firebird/Util.cxx:52:
        firebird_sdbc error:
    *Attempt to reclose a closed cursor
    caused by
    'isc_dsql_free_statement: close cursor'

and message box:

    LibreOfficeDev Base
    An error occurred.  Do you want to contiue copying?
    <Yes>  <No>  <More>


These observations are from debian-buster and daily Linux dbgutil
bibisect repository version 2017-12-29.
Comment 1 Robert Großkopf 2017-12-30 08:58:56 UTC
Couldn't confirm with 
Version: 6.0.0.1
Build-ID: d2bec56d7865f05a1003dc88449f2b0fdd85309a

Also couldn't confirm with
Version: 5.4.4.2
Build-ID: 2524958677847fb3bb44820e40380acbe820f960

Both tested on OpenSUSE 42.2 64bit rpm Linux.

Downloaded daily build
Version: 6.0.0.1.0+
Build ID: 4393ced78656c083b01b66d8987ff956e90b39ab
CPU threads: 4; OS: Linux 4.4; UI render: default; VCL: kde4; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:libreoffice-6-0, Time: 2017-12-30_00:57:19
Locale: de-DE (de_DE.UTF-8); Calc: group

Also no error. Table has been pasted. Only thing I recognized: Pasting of the table is very slow.
Comment 2 Julien Nabet 2017-12-30 16:21:34 UTC
On pc Debian x86-64 with master sources updated yesterday, I see the error messages in console but the table is copied.
Comment 3 Julien Nabet 2017-12-30 18:55:38 UTC
Let's put this one to NEW since it's been reproduced on tdf#114711
Comment 4 Julien Nabet 2017-12-30 20:42:14 UTC
I retrieved a backtrace from location indicated by console logs and got this:
#0  0x00007fffc4b9610e in connectivity::firebird::StatusVectorToString(long const (&) [20], rtl::OUString const&) (rStatusVector=..., rCause="isc_dsql_prepare")
    at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/Util.cxx:54
#1  0x00007fffc4b962fb in connectivity::firebird::evaluateStatusVector(long const (&) [20], rtl::OUString const&, com::sun::star::uno::Reference<com::sun::star::uno::XInterface> const&) (rStatusVector=..., rCause="isc_dsql_prepare", _rxContext=uno::Reference to (connectivity::firebird::OStatement *) 0x5555580dea40)
    at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/Util.cxx:64
#2  0x00007fffc4b8a5c6 in connectivity::firebird::OStatementCommonBase::prepareAndDescribeStatement(rtl::OUString const&, XSQLDA*&, XSQLDA*) (this=0x5555580dea40, sql="INSERT INTO \"Table12\" ( \"cardinal\",\"words\" ) ( SELECT \"cardinal\", \"words\" FROM \"Table1\" )", pOutSqlda=@0x5555580debe0: 0x5555580dcfa0, pInSqlda=0x0)
    at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/StatementCommonBase.cxx:163
#3  0x00007fffc4b88f46 in connectivity::firebird::OStatement::executeQuery(rtl::OUString const&) (this=0x5555580dea40, sql="INSERT INTO \"Table12\" ( \"cardinal\",\"words\" ) ( SELECT \"cardinal\", \"words\" FROM \"Table1\" )") at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/Statement.cxx:112
#4  0x00007fffc4b892bb in connectivity::firebird::OStatement::execute(rtl::OUString const&) (this=0x5555580dea40, sql="INSERT INTO \"Table12\" ( \"cardinal\",\"words\" ) ( SELECT \"cardinal\", \"words\" FROM \"Table1\" )") at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/Statement.cxx:148
#5  0x00007fffc4b88dcc in connectivity::firebird::OStatement::executeUpdate(rtl::OUString const&) (this=0x5555580dea40, sql="INSERT INTO \"Table12\" ( \"cardinal\",\"words\" ) ( SELECT \"cardinal\", \"words\" FROM \"Table1\" )") at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/Statement.cxx:96
#6  0x00007fffcae09623 in OStatement::executeUpdate(rtl::OUString const&) (this=0x5555580ddff0, _rSQL="INSERT INTO \"Table12\" ( \"cardinal\",\"words\" ) ( SELECT \"cardinal\", \"words\" FROM \"Table1\" )") at /home/julien/lo/libreoffice/dbaccess/source/core/api/statement.cxx:478
#7  0x00007fffc835d28e in dbaui::CopyTableWizard::impl_doCopy_nothrow() (this=0x5555580a3cf0) at /home/julien/lo/libreoffice/dbaccess/source/ui/uno/copytablewizard.cxx:1368

The request is generated from CopyTableWizard::impl_getServerSideCopyStatement_throw 
1442 const OUString sComposedTableName = ::dbtools::composeTableName( xDestMetaData, _xTable, ::dbtools::EComposeRule::InDataManipulation, true );
1443 OUString sSql("INSERT INTO " + sComposedTableName + " ( " + sColumns.makeStringAndClear() + " ) ( " + m_pSourceObject->getSelectStatement() + " )");

see https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/uno/copytablewizard.cxx#1425

Lionel: here are 2 questions:
1) I haven't found SQL ref about INSERT INTO SELECT but when I read about it on to https://www.w3schools.com/sql/sql_insert_into_select.asp or on other websites, I never see parenthesis around SELECT block.
Do you think it'd be ok to remove them?

2) Now about the table used for INSERT block, the sql can't succeed since the new table doesn't exist yet at this moment.
However, if we use the same table as the SELECT block, it won't work as soon as there's primary/unique index.
Any thoughts?
Comment 5 Terrence Enger 2017-12-31 00:31:17 UTC
Julien,

I accused the extra parentheses on the grounds that neither of
    https://www.firebirdsql.org/file/documentation/reference_manuals/fblangref25-en/html/fblangref25-dml-select.html
    https://www.firebirdsql.org/file/documentation/reference_manuals/fblangref25-en/html/fblangref25-dml-insert.html
show parentheses around the embedded SELECT.  I have verified via
Tools > SQL that Firebird accepts the INSERT without the extra
parentheses.

That, BTW, is how bug 114702 came to be. <sigh />
Comment 6 Lionel Elie Mamane 2017-12-31 13:42:11 UTC
(In reply to Julien Nabet from comment #4)
> Lionel: here are 2 questions:
> 1) I haven't found SQL ref about INSERT INTO SELECT (...)
>    I never see parenthesis around SELECT block.
> Do you think it'd be ok to remove them?

Yes, removing them should be fine. Both versions should work in most DBMSs (at least those that support subqueries). The version without parentheses is actually "more basic SQL", while the version with parentheses is a subquery. Just hoping it will not break another DBMS.

> 2) Now about the table used for INSERT block, the sql can't succeed since
> the new table doesn't exist yet at this moment.

That's weird. The code should definitely first do the CREATE TABLE and then the INSERT.
Comment 7 Lionel Elie Mamane 2017-12-31 13:50:05 UTC
The table creation should happen there
https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/misc/WCopyTable.cxx#1233
when that function is called from
https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/uno/copytablewizard.cxx#1324

If it doesn't, maybe the Firebird SDBC driver doesn't implement the necessary API calls? If that's the reason, that warrants a wishlist bug that blocks bug 51780.
Comment 8 Julien Nabet 2017-12-31 14:15:45 UTC
(In reply to Terrence Enger from comment #5)
> Julien,
> 
> I accused the extra parentheses on the grounds that neither of
>    
> https://www.firebirdsql.org/file/documentation/reference_manuals/fblangref25-
> en/html/fblangref25-dml-select.html
>    
> https://www.firebirdsql.org/file/documentation/reference_manuals/fblangref25-
> en/html/fblangref25-dml-insert.html
> show parentheses around the embedded SELECT.  I have verified via
> Tools > SQL that Firebird accepts the INSERT without the extra
> parentheses.
> 
> That, BTW, is how bug 114702 came to be. <sigh />

Yes but the content of dbaccess/source/ui/uno/copytablewizard.cxx must work for every DB not just for Firebird.
Anyway, I also read the Lionel's comment and will propose a patch to just remove these parenthesis.
Comment 9 Julien Nabet 2017-12-31 14:22:43 UTC
(In reply to Lionel Elie Mamane from comment #7)
> The table creation should happen there
> https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/misc/
> WCopyTable.cxx#1233
> when that function is called from
> https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/uno/
> copytablewizard.cxx#1324
> 
> If it doesn't, maybe the Firebird SDBC driver doesn't implement the
> necessary API calls? If that's the reason, that warrants a wishlist bug that
> blocks bug 51780.

Thank you for your detailed feedback Lionel.
Dumb me, I was testing the sql retrieved from gdb, of course table couldn't exist.
I've just tested after having removed parenthesis from SELECT block, I go no more error from Firebird in console.
Just a lot of these:
warn:legacy.osl:27488:27488:comphelper/source/property/propertycontainerhelper.cxx:186: OPropertyContainerHelper::implPushBackProperty: name already exists!
warn:legacy.osl:27488:27488:comphelper/source/property/propertycontainerhelper.cxx:187: OPropertyContainerHelper::implPushBackProperty: handle already exists!

and twice:
warn:legacy.osl:27488:27488:cppuhelper/source/propshlp.cxx:990: Property array is not sorted

but that's another story.
Comment 10 Julien Nabet 2017-12-31 14:27:06 UTC
I submitted a patch to review here: https://gerrit.libreoffice.org/#/c/47218/1
Comment 11 Commit Notification 2017-12-31 15:34:49 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b3ea7ed747991696b9f02ede14c57f333c28a41b

tdf#114755: Remove extra parenthesis

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2018-01-01 01:01:24 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2d1a732a2afabb6946d389f01dc491f0f3b6a571&h=libreoffice-6-0

tdf#114755: Remove extra parenthesis

It will be available in 6.0.0.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Julien Nabet 2018-01-01 09:47:04 UTC
Terrence: could you give a new try with an empty and a not empty table once there's a build including the patch?
(knowing that it could also help for tdf#114711)
Comment 14 Terrence Enger 2018-01-01 15:13:18 UTC
I observe the problem fixed in daily Linux dbgutil bibisect repository
2018-01-01.  I am setting status VERIFIED FIXED.

The program also copied a table with 4 rows without problem.  So,
tdf#114755 seems to be fixed as well.

Thank you, Julien.