[Fdo-trac] [fdo-trac] #228: MySQL Defect : Can't retreive tablename in joined query

[FDO]

#228: MySQL Defect : Can't retreive tablename in joined query
----------------------------+-------------------------
 Reporter:  bscott          |        Owner:  danstoica
     Type:  defect          |       Status:  closed
 Priority:  major           |    Milestone:  3.4.0
Component:  MySQL Provider  |      Version:  3.2.0
 Severity:  4               |   Resolution:  fixed
 Keywords:                  |  External ID:
----------------------------+-------------------------
Changes (by jng):

 * resolution:   => fixed
 * status:  new => closed

Comment:

 In [changeset:"8399" 8399]:
 {{{#!CommitTicketReference repository="" revision="8399"
 Fix joined queries reporting indistinguishable column names

 A native FdoISQLCommand query that selects a same-named column from more
 than
 one table reports that one name for every one of them, and
 FdoISQLDataReader
 resolves a column name to the first column that has it, so every column
 after
 the first was unreachable by name. That is FDO ticket #228, which asked
 either
 for a table-qualified name from GetColumnName() or for some other way to
 tell
 the columns apart. A table-qualified name was not taken: it changes what a
 caller receives for every query, it means nothing for a select-list item
 that is
 an expression (the reporter's own ticket comment gives "table1.column1 +
 table2.column2" as the case that rules it out), and it needs a new table-
 name
 output on the rdbi_describe_slct contract that every driver would have to
 supply. The readers already had the disambiguation both interfaces need -
 a
 repeat of a select-list name gets a "$<n>" suffix, so a join reports
 SHARED_COL, SHARED_COL$1, SHARED_COL$2 - and two defects sat behind it, so
 the
 GenericRdbms path was broken for the second repeat and the King Oracle
 path did
 not disambiguate at all.

 FdoRdbmsSQLDataReader::GenerateUniqueName() wrote the digit of its suffix
 with
 "*(dest + propsize + 1 + idx) = buffer[idx++]", which reads and increments
 idx
 without a sequence point; GCC evaluates the right-hand side first, so the
 digit
 went one slot past the NUL terminator and the name did not change. A two-
 table
 join therefore reported SHARED_COL and a malformed SHARED_COL$, and
 because the
 name was never replaced, the retry loop that looks for a name the result
 set
 does not already use never terminated: a query with three same-named
 select-list
 items hung in ExecuteReader() indefinitely (the loop's counter was
 observed
 climbing past 25 with the name unchanged). The same expression in the
 sibling
 "name too long" branch also left a gap at dest[2]. Both branches now copy
 the
 digit and advance the index in separate statements.

 c_KgOraSQLDataReader, which is the King Oracle provider's own reader
 rather than
 a subclass of the GenericRdbms one, returned the OCI column name for every
 ordinal and nothing more, so its name-to-index lookup reached only the
 first
 column of a repeated name - the original symptom of the ticket, unfixed.

 Because the suffix is produced by the shared reader, the GenericRdbms fix
 covers every provider that reads a native query through it (MySQL,
 PostGIS,
 ODBC and SQL Server Spatial); King Oracle needed its own.

 Provider:
 * Providers/GenericRdbms/Src/Fdo/Other/FdoRdbmsSQLDataReader.cpp:
   GenerateUniqueName() copies each digit into dest with the index advanced
 in a
   second statement, in both the suffix and the "too long" branch, and the
   sequence-point hazard is noted in a comment. The generated names are
   unchanged - first repeat "$1", second "$2" - so no other caller of this
   reader is affected.
 * Providers/KingOracle/src/Provider/c_KgOraSQLDataReader.cpp,
 c_KgOraSQLDataReader.h: the
   constructor builds a new m_ColumnNames collection that holds the
 reported name
   of every result set column in ordinal order, via the new
   MakeUniqueColumnName(), which suffixes a name that a previous column
 already
   used (case-insensitively, matching the collection's own lookup). The
   recognized-type and SDO_GEOMETRY branches add that same name to
 m_SqlColumns,
   so the name a caller is given is the name that resolves to its own
 column.
   GetColumnName() returns the reported name and now range-checks its
 index;
   it previously passed the ordinal to OCI unvalidated. m_ColumnNames is an
   FdoPtr, so the new collection is released with the reader, and only
 private
   headers changed (the class lives under src/Provider, not the installed
   inc/KingOracle tree), so no shipped layout moves.

 Tests:
 * Providers/GenericRdbms/Src/UnitTest/Common/FdoSqlCmdTest.cpp,
 FdoSqlCmdTest.h:
   FdoSqlCmdTest::QueryJoinedColumns (new), in the shared provider suite:
 three
   tables are each given a key_col and a shared_col, one row is inserted
 into
   each, and a three-way join selects shared_col three times. It asserts
 the
   reported names are SHARED_COL, SHARED_COL$1 and SHARED_COL$2, that each
 name
   reads its own column through GetInt32, and that ExecuteReader() returns
 at
   all. It fails before the fix - with two tables the name is "SHARED_COL$"
 and
   with three the reader never returns. Registered in the base suite, so
 MySQL,
   PostGIS, ODBC and SQL Server Spatial all inherit it. The precondition
 uses
   DROP TABLE IF EXISTS inside a catch: a plain failed DROP aborts a
 PostgreSQL
   transaction and would poison every statement after it, while the clause
 itself
   is newer than some servers this suite targets.
 * Providers/GenericRdbms/Src/UnitTest/Odbc/OdbcFdoSqlCmdTest.h: a no-op
   override of QueryJoinedColumns for OdbcAccessFdoSqlCmdTest, alongside
 the
   stubs it already has for the other tests that fixture cannot run because
 it
   has no SQL-command support.
 * Providers/KingOracle/src/KgOraUnitTest/SelectTests.cpp, SelectTests.h:
   SelectTests::SelectJoinedColumns (new) for King Oracle: the same three-
 way
   join against Oracle XE 11g, asserting the same three names and that each
 reads
   its own value. It failed before the provider fix with the second column
   reported as SHARED_COL.
 * Providers/KingOracle/src/KgOraUnitTest/InsertUpdateDeleteTests.cpp:
   AddBatchRow (was MakeBatchRow returning a Detached raw pointer) now
 builds the
   row of a batch insert into an FdoPtr and adds it from there.
   FdoCollection::Add() takes a reference of its own and does not consume
 the
   caller's, so the reference the row factory made was left with no owner -
 11
   direct LeakSanitizer records of 32 bytes, one per row of the four
 InsertBatch
   tests, each anchoring its row's parameter values, their data values, the
   FdoGeometryValue and the FGF byte array behind it. This fixes a leak
   introduced with r8398's batch-insert tests and leaves their assertions
   unchanged.

 Verified in the baseline ubuntu22 Debug (ASan) build container against the
 dbs
 compose MySQL 5.6, PostGIS and Oracle XE 11g. FdoSqlCmdTest goes from OK
 (9) to
 OK (10) for MySQL and runs OK (9) for PostGIS; the full MySql and PostGis
 suites
 are OK (249) and OK (140) with no direct leak record and no record whose
 stack
 refers to the changed code (their LeakSanitizer totals are unchanged at
 46236
 bytes in 491 allocations and 105212 bytes in 942). KgOraUnitTest is OK
 (39) with
 SelectTests OK (3), up from OK (2), and its LeakSanitizer total goes from
 6284
 bytes in 132 allocations (11 direct, 121 indirect) to 0 bytes.
 UnitTestOdbc
 builds with the new override.

 Fixes #228
 }}}
-- 
Ticket URL: <https://trac.osgeo.org/fdo/ticket/228#comment:3>
FDO <http://fdo.osgeo.org/>
Feature Data Objects