Derby
  1. Derby
  2. DERBY-2152

Support diagnostic vti tables that take parameters, such as SpaceTable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: SQL
    • Labels:
      None

      Description

      Expand the work of DERBY-571 to support the remaining diagnostic tables that take parameters.

      Syntax would use the table constructor, like (not sure if an 'AS' clause will be required:

      select * from TABLE(SYSCS_DIAG.SPACE_TABLE(?, ?))

      Diagnostic VTIs that could be handled this way are:

      ErrorLogReader(String log file name)
      SpaceTable(String tableName)
      SpaceTable(String schemaName, String tableName)
      StatementDuration(String inputFileName)

      This is the second stage mentioned in DERBY-571

      1. d2152_engine_v1.patch
        7 kB
        A B
      2. d2152_engine_v2.patch
        11 kB
        A B
      3. d2152_junit_jdk16_fix.patch
        1 kB
        A B
      4. d2152_testing_v1.patch
        29 kB
        A B
      5. d2152_testing_v2.patch
        31 kB
        A B
      6. d2152_v1.stat
        0.7 kB
        A B
      7. d2152_v2.stat
        0.9 kB
        A B
      8. d2152_vtiMappingCleanup_v1.patch
        16 kB
        A B
      9. d2152_vtiMappingCleanup_v1.stat
        0.6 kB
        A B
      10. junit-errors.txt
        6 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          36d 2h 28m 1 A B 10/Jan/07 21:29
          Resolved Resolved Closed Closed
          4d 18h 49m 1 A B 15/Jan/07 16:18
          Gavin made changes -
          Workflow jira [ 12391587 ] Default workflow, editable Closed status [ 12797099 ]
          Gavin made changes -
          Link This issue depends upon DERBY-1520 [ DERBY-1520 ]
          Gavin made changes -
          Link This issue depends on DERBY-1520 [ DERBY-1520 ]
          Rick Hillegas made changes -
          Link This issue is related to DERBY-5554 [ DERBY-5554 ]
          Hide
          Laura Stewart added a comment -

          There is a documentation fissue that needs to incorporate the updates made by Derby2152.
          The documentation issue is Derby1520.

          Show
          Laura Stewart added a comment - There is a documentation fissue that needs to incorporate the updates made by Derby2152. The documentation issue is Derby1520.
          Laura Stewart made changes -
          Link This issue depends on DERBY-1520 [ DERBY-1520 ]
          Laura Stewart made changes -
          Link This issue blocks DERBY-1520 [ DERBY-1520 ]
          Hide
          Laura Stewart added a comment -

          The documentation issue for Derby2152 is Derby1520.

          Show
          Laura Stewart added a comment - The documentation issue for Derby2152 is Derby1520.
          Laura Stewart made changes -
          Link This issue blocks DERBY-1520 [ DERBY-1520 ]
          A B made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          A B added a comment -

          The patch for this issue has been committed and I haven't seen any fallout, so I'm marking it as closed.

          Show
          A B added a comment - The patch for this issue has been committed and I haven't seen any fallout, so I'm marking it as closed.
          A B made changes -
          Fix Version/s 10.3.0.0 [ 12310800 ]
          Derby Info [Patch Available]
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Hide
          A B added a comment -

          Thank you for taking a look, Dan. I committed the cleanup patch with svn #494993. I'm resolving the issue now and will close it in a couple of days if nothing further comes up.

          Show
          A B added a comment - Thank you for taking a look, Dan. I committed the cleanup patch with svn #494993. I'm resolving the issue now and will close it in a couple of days if nothing further comes up.
          Hide
          Daniel John Debrunner added a comment -

          Cleanup looks good.

          Show
          Daniel John Debrunner added a comment - Cleanup looks good.
          Hide
          A B added a comment -

          I haven't heard anything regarding the latest patch for this issue (d2152_vtiMappingCleanup_v1.patch), so I'm inclined to commit it. If anyone is planning to review, please let me know; otherwise I'll go ahead and commit it tomorrow (Wednesday) PST.

          Show
          A B added a comment - I haven't heard anything regarding the latest patch for this issue (d2152_vtiMappingCleanup_v1.patch), so I'm inclined to commit it. If anyone is planning to review, please let me know; otherwise I'll go ahead and commit it tomorrow (Wednesday) PST.
          A B made changes -
          Derby Info [Patch Available]
          Hide
          A B added a comment -

          Attached d2152_vtiMappingCleanup_v1.patch, which does some reorganization of the code added for this issue and for DERBY-571 to make them more consistent with each other. In particular, the patch addresses the following review comment from Dan:

          > The lack of consistency between getVTIClassForTableFunction() and
          > getVTIClassForTable() seems strange, though I can see why you did
          > it that way. Once the code has been committed maybe some cleanup
          > could be done, could be the old code to match the new code, or
          > some other common ground. The differences are:
          >
          > - getVTIClassFor*() methods take different argument styles to
          > represent the same information
          >
          > - one resolves the vti class outside the NewInvocationNode, one inside.

          In an attempt to resolve these differences, the patch does the following:

          1. Creates a new "init()" method for NewInvocationNode that is specifically targeted for mapping VTI "tables" and VTI "table functions" to their corresponding class names. The new init() method takes both a TableDescriptor and a TableName, exactly one of which must be null. If the TableDescriptor is null then we will resolve the TableName to a VTI "table function"; if the TableName is null then we will resolve the TableDescriptor to a VTI "table". Thus VTI classes are now consistently resolved inside the NewInvocationNode class.

          2. Updates NodeFactory.mapTableAsVTI() to make use of the new "init()" method in NewInvocationNode. mapTableAsVTI() used to take as a parameter the resolved VTI class name; now it just passes the received TableDescriptor to the new init() method in NewInvocationNode and the latter resolves the VTI class.

          3. Combines the getVTIClassForTable() and getVTIClassForTableFunction() methods of DataDictionary into a new method, "getVTIClass()", that takes a TableDescriptor and a boolean value. The boolean indicates the kind of mapping to do-"table" or "table function"-while the TableDescriptor holds the schema and name information used for resolving the class. This resolves the inconsistency between the argument styles used in the old getVTIClassFor*() methods.

          I ran suites.All on Windows 2000 with ibm142 and there were no failures. I also ran derbyall on Red Hat Linux with ibm142 and all was clean. And as a sanity check I ran SysDiagVTIMappingTest against jdk16 (after building JDBC 4.0) and the test ran without problem.

          Review comments/suggestions are appreciated, as always.

          Show
          A B added a comment - Attached d2152_vtiMappingCleanup_v1.patch, which does some reorganization of the code added for this issue and for DERBY-571 to make them more consistent with each other. In particular, the patch addresses the following review comment from Dan: > The lack of consistency between getVTIClassForTableFunction() and > getVTIClassForTable() seems strange, though I can see why you did > it that way. Once the code has been committed maybe some cleanup > could be done, could be the old code to match the new code, or > some other common ground. The differences are: > > - getVTIClassFor*() methods take different argument styles to > represent the same information > > - one resolves the vti class outside the NewInvocationNode, one inside. In an attempt to resolve these differences, the patch does the following: 1. Creates a new "init()" method for NewInvocationNode that is specifically targeted for mapping VTI "tables" and VTI "table functions" to their corresponding class names. The new init() method takes both a TableDescriptor and a TableName, exactly one of which must be null. If the TableDescriptor is null then we will resolve the TableName to a VTI "table function"; if the TableName is null then we will resolve the TableDescriptor to a VTI "table". Thus VTI classes are now consistently resolved inside the NewInvocationNode class. 2. Updates NodeFactory.mapTableAsVTI() to make use of the new "init()" method in NewInvocationNode. mapTableAsVTI() used to take as a parameter the resolved VTI class name; now it just passes the received TableDescriptor to the new init() method in NewInvocationNode and the latter resolves the VTI class. 3. Combines the getVTIClassForTable() and getVTIClassForTableFunction() methods of DataDictionary into a new method, "getVTIClass()", that takes a TableDescriptor and a boolean value. The boolean indicates the kind of mapping to do- "table" or "table function" -while the TableDescriptor holds the schema and name information used for resolving the class. This resolves the inconsistency between the argument styles used in the old getVTIClassFor*() methods. I ran suites.All on Windows 2000 with ibm142 and there were no failures. I also ran derbyall on Red Hat Linux with ibm142 and all was clean. And as a sanity check I ran SysDiagVTIMappingTest against jdk16 (after building JDBC 4.0) and the test ran without problem. Review comments/suggestions are appreciated, as always.
          A B made changes -
          Attachment d2152_vtiMappingCleanup_v1.patch [ 12348282 ]
          Attachment d2152_vtiMappingCleanup_v1.stat [ 12348283 ]
          Hide
          Knut Anders Hatlen added a comment -

          A B wrote:

          > But that said, I don't understand why I'm not seeing the failure on
          > my local machine, and I don't understand why Knut Anders was seeing
          > it with jdk15...?

          I rechecked my script which runs all the test suites and then I
          remembered that I changed it to use jdk6 for the JUnit tests some time
          ago. But that still doesn't explain why you don't see the failure on
          jdk6. Did you build the JDBC 4.0 drivers? I don't think you will see
          these failures if you run with the JDBC 3.0 drivers on jdk6.

          Your patch fixes the failure in my environment. Thank you! I have
          committed it to trunk with revision 489597.

          Show
          Knut Anders Hatlen added a comment - A B wrote: > But that said, I don't understand why I'm not seeing the failure on > my local machine, and I don't understand why Knut Anders was seeing > it with jdk15...? I rechecked my script which runs all the test suites and then I remembered that I changed it to use jdk6 for the JUnit tests some time ago. But that still doesn't explain why you don't see the failure on jdk6. Did you build the JDBC 4.0 drivers? I don't think you will see these failures if you run with the JDBC 3.0 drivers on jdk6. Your patch fixes the failure in my environment. Thank you! I have committed it to trunk with revision 489597.
          A B made changes -
          Attachment d2152_junit_jdk16_fix.patch [ 12347697 ]
          Hide
          A B added a comment -

          > I got three failures in SysDiagVTIMappingTest when I ran the All suite on a clean trunk.

          It looks like these same failures also occurred in the nightly test results for Sun 1.6:

          http://dbtg.thresher.com/derby/test/Daily/jvm1.6/testing/testlog/sol/489139-_diff.txt

          This looks like a nested exception problem: the ASSERT statement checks for SQLSTATE 42X62, but that's actually a nested SQLSTATE; the top-level error has SQLSTATE 38000.

          My first reaction was that we're seeing a case of DERBY-1440, which seems particularly likely because the failures only occur when running against the Derby Client. But that said:

          1) When I run the test locally with jdk16, I don't see the failures (which surprised me).
          2) DERBY-1440 only talks about jdk16; it does not mention 1.5. But Knut Anders reported the failures for 1.5, which is odd.

          I'm attaching a patch, d2152_junit_jdk16_fix.patch, that should resolve the failure for jdk16 (and thus in the nightlies). But that said, I don't understand why I'm not seeing the failure on my local machine, and I don't understand why Knut Anders was seeing it with jdk15...?

          Note: I'm currently on vacation so I'm not able to investigate this further. If anyone has a chance to track this down and s/he determines that the attached patch should in fact be committed, then please commit it! I would commit it myself, but since I don't understand the two points mentioned above and since I am not actively monitoring the lists, I don't want to make things worse...

          Show
          A B added a comment - > I got three failures in SysDiagVTIMappingTest when I ran the All suite on a clean trunk. It looks like these same failures also occurred in the nightly test results for Sun 1.6: http://dbtg.thresher.com/derby/test/Daily/jvm1.6/testing/testlog/sol/489139-_diff.txt This looks like a nested exception problem: the ASSERT statement checks for SQLSTATE 42X62, but that's actually a nested SQLSTATE; the top-level error has SQLSTATE 38000. My first reaction was that we're seeing a case of DERBY-1440 , which seems particularly likely because the failures only occur when running against the Derby Client. But that said: 1) When I run the test locally with jdk16, I don't see the failures (which surprised me). 2) DERBY-1440 only talks about jdk16; it does not mention 1.5. But Knut Anders reported the failures for 1.5, which is odd. I'm attaching a patch, d2152_junit_jdk16_fix.patch, that should resolve the failure for jdk16 (and thus in the nightlies). But that said, I don't understand why I'm not seeing the failure on my local machine, and I don't understand why Knut Anders was seeing it with jdk15...? Note: I'm currently on vacation so I'm not able to investigate this further. If anyone has a chance to track this down and s/he determines that the attached patch should in fact be committed, then please commit it! I would commit it myself, but since I don't understand the two points mentioned above and since I am not actively monitoring the lists, I don't want to make things worse...
          Knut Anders Hatlen made changes -
          Attachment junit-errors.txt [ 12347565 ]
          Hide
          Knut Anders Hatlen added a comment -

          I got three failures in SysDiagVTIMappingTest when I ran the All suite on a clean trunk. The stack traces are attached in junit-errors.txt. I saw the errors only once. The tests were run on Solaris 10 with Sun's JVM 1.5.0_06. I reran the tests many times, but never saw the errors again. A timing issue, perhaps?

          Show
          Knut Anders Hatlen added a comment - I got three failures in SysDiagVTIMappingTest when I ran the All suite on a clean trunk. The stack traces are attached in junit-errors.txt. I saw the errors only once. The tests were run on Solaris 10 with Sun's JVM 1.5.0_06. I reran the tests many times, but never saw the errors again. A timing issue, perhaps?
          A B made changes -
          Derby Info [Patch Available]
          Hide
          A B added a comment -

          > I think the schema name for the function will always be passed in,
          > even if it isn't explicitly set in the SQL statement.

          Yes, you're right. In cases where the schema isn't explicitly specified the code will simply pass the current schema name to the DataDictionary. In NewInvocation.java:

          + /* If no schema was specified then we want to default to the
          + * current schema; that's what the following line does.
          + */
          + String funcSchema =
          + getSchemaDescriptor(funcName.getSchemaName()).getSchemaName();

          So I removed the words "if specified" from the javadoc comment. Thank you for pointing this out.

          > Once the code has been committed maybe some cleanup could be done,
          > could be the old code to match the new code, or some other common
          > ground.

          Good idea, thank you for bringing this up. I committed the _v2 patches (after updating the javadoc as mentioned above) with svn revision #488827. I'll look at creating a follow-up patch to make the two getVTIClassFor* methods more consistent with each other. That said, I'm leaving town tomorrow so I probably won't be posting any follow-up patches until January...

          Thank you for the review!

          Show
          A B added a comment - > I think the schema name for the function will always be passed in, > even if it isn't explicitly set in the SQL statement. Yes, you're right. In cases where the schema isn't explicitly specified the code will simply pass the current schema name to the DataDictionary. In NewInvocation.java: + /* If no schema was specified then we want to default to the + * current schema; that's what the following line does. + */ + String funcSchema = + getSchemaDescriptor(funcName.getSchemaName()).getSchemaName(); So I removed the words "if specified" from the javadoc comment. Thank you for pointing this out. > Once the code has been committed maybe some cleanup could be done, > could be the old code to match the new code, or some other common > ground. Good idea, thank you for bringing this up. I committed the _v2 patches (after updating the javadoc as mentioned above) with svn revision #488827. I'll look at creating a follow-up patch to make the two getVTIClassFor* methods more consistent with each other. That said, I'm leaving town tomorrow so I probably won't be posting any follow-up patches until January... Thank you for the review!
          Hide
          Daniel John Debrunner added a comment -

          The separation looks good.
          The javadoc comments for getVTIClassForTableFunction says
          @param funcSchema Schema part of the function name if specified
          but I think the schema name for the function will always be passed in, even if it isn't explicitly set in the SQL statement.

          The lack of consistency between getVTIClassForTableFunction() and getVTIClassForTable() seems strange, though I can see why you did it that way. Once the code has been committed maybe some cleanup could be done, could be the old code to match the new code, or some other common ground. The differences are:

          • getVTIClassFor*() methods take different argument styles to represent the same information
          • one resolves the vti class outside the NewInvocationNode, one inside.
          Show
          Daniel John Debrunner added a comment - The separation looks good. The javadoc comments for getVTIClassForTableFunction says @param funcSchema Schema part of the function name if specified but I think the schema name for the function will always be passed in, even if it isn't explicitly set in the SQL statement. The lack of consistency between getVTIClassForTableFunction() and getVTIClassForTable() seems strange, though I can see why you did it that way. Once the code has been committed maybe some cleanup could be done, could be the old code to match the new code, or some other common ground. The differences are: getVTIClassFor*() methods take different argument styles to represent the same information one resolves the vti class outside the NewInvocationNode, one inside.
          A B made changes -
          Derby Info [Patch Available]
          A B made changes -
          Attachment d2152_testing_v2.patch [ 12347455 ]
          Attachment d2152_v2.stat [ 12347456 ]
          Attachment d2152_engine_v2.patch [ 12347454 ]
          Hide
          A B added a comment -

          Posting a second version of the patch, d2152_engine_v2.patch and d2152_testing_v2.patch, to address Dan's review comments. In particular this version of the patch separates VTI "tables" (DERBY-571) from VTI "table functions". So the following statement:

          select * from syscs_diag.space_table

          will now throw error 42X05 ("table not found"). That's also what will happen for the other table functions defined in this issue (namely, statement_duration and error_log_reader). Similarly, the following statement:

          select * from table (syscs_diag.lock_table())

          will now throw error 42Y03 ("not recognized as a function or procedure"). That's also what will happen for the other VTI tables defined in DERBY-571 (namely, statement_cache, transaction_table, and error_messages).

          To accomplish this separation the _v2 patches have the following changes from the _v1 patches:

          • Renamed the "getVTIClass()" method in DataDictionary to "getVTIClassForTable()"
          • Added new method, "getVTIClassForTableFunction()", to the DataDictionary
            interface and implementation classes. The new method takes two strings:
            one for the schema name of the table function and one for the function name
            itself (ex. "space_table"). Also pushed the logic that checks the schema
            name (i.e. to make sure it is "SYSCS_DIAG") down into the new DataDictionary
            method, as mentioned in my previous comment.
          • Changed NewInvocation.java to call the new getVTIClassForTableFunction()
            method.
          • Changed the error thrown by NewInvocation for an invalid table function name
            to be error 42Y03 ("not recognized as a function or procedure") instead of
            a syntax error.
          • Added new test cases to the JUnit class to make sure that the correct errors
            are thrown if VTI tables are used in the TABLE constructor (42Y03) or if VTI
            table functions are used as base tables (42X05). Also updated the expected
            SQLSTATEs for existing test cases as appropriate.

          I ran deryall as a sanity check on Red Hat Linux with ibm142 and there were no failures.

          I think this patch accomplishes the separation of "tables" from "table functions" that Dan mentioned, but of course I could still be missing something. Any review comments one way or the other would be appreciated.

          Show
          A B added a comment - Posting a second version of the patch, d2152_engine_v2.patch and d2152_testing_v2.patch, to address Dan's review comments. In particular this version of the patch separates VTI "tables" ( DERBY-571 ) from VTI "table functions". So the following statement: select * from syscs_diag.space_table will now throw error 42X05 ("table not found"). That's also what will happen for the other table functions defined in this issue (namely, statement_duration and error_log_reader). Similarly, the following statement: select * from table (syscs_diag.lock_table()) will now throw error 42Y03 ("not recognized as a function or procedure"). That's also what will happen for the other VTI tables defined in DERBY-571 (namely, statement_cache, transaction_table, and error_messages). To accomplish this separation the _v2 patches have the following changes from the _v1 patches: Renamed the "getVTIClass()" method in DataDictionary to "getVTIClassForTable()" Added new method, "getVTIClassForTableFunction()", to the DataDictionary interface and implementation classes. The new method takes two strings: one for the schema name of the table function and one for the function name itself (ex. "space_table"). Also pushed the logic that checks the schema name (i.e. to make sure it is "SYSCS_DIAG") down into the new DataDictionary method, as mentioned in my previous comment. Changed NewInvocation.java to call the new getVTIClassForTableFunction() method. Changed the error thrown by NewInvocation for an invalid table function name to be error 42Y03 ("not recognized as a function or procedure") instead of a syntax error. Added new test cases to the JUnit class to make sure that the correct errors are thrown if VTI tables are used in the TABLE constructor (42Y03) or if VTI table functions are used as base tables (42X05). Also updated the expected SQLSTATEs for existing test cases as appropriate. I ran deryall as a sanity check on Red Hat Linux with ibm142 and there were no failures. I think this patch accomplishes the separation of "tables" from "table functions" that Dan mentioned, but of course I could still be missing something. Any review comments one way or the other would be appreciated.
          Hide
          James Synge added a comment -

          How interesting (i.e. convenient for me ) that you're working on this at the same
          time that I'm working on DERBY-47 (IN optimization). I'm experimenting with
          transforming a query such as:

          SELECT * FROM tableA WHERE columnB IN (constC, ?, ?, constD)

          into

          SELECT * FROM tableA WHERE columnB IN (SELECT vti.column1 FROM new ArgsToRowsVTI(SomeArgs))

          An issue I've not yet resolved is when/how to pass the arguments to the ArgsToRowsVTI instance.
          It needs to have one column with the same type as columnB (known at compile/bind time).
          And it needs to have N rows (4 in the example above), with the values in the original IN list
          (including parameter markers).

          I'm interested in any thoughts regarding when and how to pass the arguments. For example, I could
          imagine that InListOperatorNode.preprocess will return a SubqueryNode, such that the original
          query show above would become:

          SELECT * FROM tableA WHERE columnB IN (SELECT vti.column1 FROM new ArgsToRowsVTI(typeOfColumnB, constC, ?, ?, constD))

          Any thoughts/advice?

          Show
          James Synge added a comment - How interesting (i.e. convenient for me ) that you're working on this at the same time that I'm working on DERBY-47 (IN optimization). I'm experimenting with transforming a query such as: SELECT * FROM tableA WHERE columnB IN (constC, ?, ?, constD) into SELECT * FROM tableA WHERE columnB IN (SELECT vti.column1 FROM new ArgsToRowsVTI(SomeArgs)) An issue I've not yet resolved is when/how to pass the arguments to the ArgsToRowsVTI instance. It needs to have one column with the same type as columnB (known at compile/bind time). And it needs to have N rows (4 in the example above), with the values in the original IN list (including parameter markers). I'm interested in any thoughts regarding when and how to pass the arguments. For example, I could imagine that InListOperatorNode.preprocess will return a SubqueryNode, such that the original query show above would become: SELECT * FROM tableA WHERE columnB IN (SELECT vti.column1 FROM new ArgsToRowsVTI(typeOfColumnB, constC, ?, ?, constD)) Any thoughts/advice?
          Hide
          A B added a comment -

          > I guess I'd thought about having separate resolution for the two different types,
          > tables that map to vtis and table functions that map to vtis.

          Hmm...okay, I'll have to ponder this a bit to see if this separation is a better approach. I'm not quite clear on what exactly that means just yet...

          > From looking at the patch I now see that the SYSCS_DIAG is not part of the
          > syntax grammar, but matches the existing use for the diagnostic tables.

          Right, I think my Jira comments were misleading in that regard (sorry). But this did prompt me to push the relevant code further down into DataDictionary, which I think makes things a bit cleaner. So I'll include those changes in subsequent patches.

          I'll wait another day or so before posting a followup patch (probably on Monday or Tuesday) in case there is more discussion re: the other issues I mentioned in my previous comment.

          Thank you for the quick feedback!

          Show
          A B added a comment - > I guess I'd thought about having separate resolution for the two different types, > tables that map to vtis and table functions that map to vtis. Hmm...okay, I'll have to ponder this a bit to see if this separation is a better approach. I'm not quite clear on what exactly that means just yet... > From looking at the patch I now see that the SYSCS_DIAG is not part of the > syntax grammar, but matches the existing use for the diagnostic tables. Right, I think my Jira comments were misleading in that regard (sorry). But this did prompt me to push the relevant code further down into DataDictionary, which I think makes things a bit cleaner. So I'll include those changes in subsequent patches. I'll wait another day or so before posting a followup patch (probably on Monday or Tuesday) in case there is more discussion re: the other issues I mentioned in my previous comment. Thank you for the quick feedback!
          Hide
          Daniel John Debrunner added a comment -

          On issue 1 I would say that the no-argument lock table should not be supported as a TABLE(function). I guess I'd thought about having separate resolution for the two different types, tables that map to vtis and table functions that map to vtis.
          I think with the way you have combined it this select will kind of be accepted (at least the name will be bound)

          SELECT * FROM SYSCS_DIAG.SPACE_TABLE

          but I think this should fail with a table not found exception.

          From looking at the patch I now see that the SYSCS_DIAG is not part of the syntax grammar, but matches the existing use for the diagnostic tables.

          Show
          Daniel John Debrunner added a comment - On issue 1 I would say that the no-argument lock table should not be supported as a TABLE(function). I guess I'd thought about having separate resolution for the two different types, tables that map to vtis and table functions that map to vtis. I think with the way you have combined it this select will kind of be accepted (at least the name will be bound) SELECT * FROM SYSCS_DIAG.SPACE_TABLE but I think this should fail with a table not found exception. From looking at the patch I now see that the SYSCS_DIAG is not part of the syntax grammar, but matches the existing use for the diagnostic tables.
          Hide
          Daniel John Debrunner added a comment -

          Is there any reason to make SYSCS_DIAG part of the syntax, much more flexible for it to be a standard schema name that is passed into the bind code and then onto the data dictionary?

          Show
          Daniel John Debrunner added a comment - Is there any reason to make SYSCS_DIAG part of the syntax, much more flexible for it to be a standard schema name that is passed into the bind code and then onto the data dictionary?
          A B made changes -
          Assignee A B [ army ]
          A B made changes -
          Field Original Value New Value
          Attachment d2152_engine_v1.patch [ 12347315 ]
          Attachment d2152_testing_v1.patch [ 12347316 ]
          Attachment d2152_v1.stat [ 12347317 ]
          Hide
          A B added a comment -

          Attaching a first attempt at a solution for this issue. There are two patches involved:

          1. d2152_engine_v1.patch: Changes to the Derby engine code to allow
          diagnostic VTI table mappings for VTIs that take parameters.

          2. d2152_testing_v1.patch: Addition of a new JUnit test class
          for testing the new VTI mappings.

          The engine patch pretty much does what is written in the description for this issue: namely, it allows the user to query the Derby diagnostic VTIs by using the following syntax:

          SELECT <rcList> from TABLE (SYSCS_DIAG.<vti-table-name> (<arg list>) )
          [ AS ] corrlationName

          Note that:

          a. We only support VTI table names in the SYSCS_DIAG schema
          b. The correlation name is required, though use of the "AS" keyword
          is optional (section 7.6 of the SQL 2003 spec, "<table primary>").
          c. The argument list can be empty.

          The VTI table names that have been added are as follows:

          SYSCS_DIAG.SPACE_TABLE maps to org.apache.derby.diag.SpaceTable
          SYSCS_DIAG.ERROR_LOG_READER maps to org.apache.derby.diag.ErrorLogReader
          SYSCS_DIAG.STATEMENT_DURATION maps to org.apache.derby.diag.StatementDuration

          Given all of that, the following are some areas/issues/questions for which I am hoping to get feedback. If anyone out there can review and/or provide insight/opinions on these, I'd apprecate it.

          1. Since the argument list can be empty, a user now as two different ways
          to invoke diagnostic VTIs that take zero parameters. Ex:

          A. As of DERBY-571: SELECT * FROM SYSCS_DIAG.LOCK_TABLE
          B. With my patch: SELECT * FROM TABLE (SYSCS_DIAG.LOCK_TABLE()) x

          Is it okay to have both of these syntaxes? Note that, as it turns
          out, the only diagnostic VTI that does not allow zero arguments
          is SpaceTable; all of the other VTIs will have two different syntax
          variations that work.

          2. What error should we throw if the user specifies a table name that
          is not a valid VTI table-mapped name? Ex.

          select * from TABLE (APP.T1()) x

          With the _v1 patch we'll throw a syntax error as follows:

          ERROR 42X01: Syntax error: APP.T1

          But that does not seem particularly helpful to me. Should I add a new
          error message that gives more detail, or is this okay? One other thing to
          note about this: in some cases, this error could be misleading. Ex:

          set schema APP
          select * from TABLE (SPACE_TABLE('APP', 'T1')) x

          In this case we will throw a syntax error--but strictly speaking, this
          is not a syntax error. Rather, it's an error caused by the fact that
          SPACE_TABLE is not in the "APP" schema. So if, for example, I did the
          following:

          set schema SYSCS_DIAG
          select * from TABLE (SPACE_TABLE('APP', 'T1')) x

          the SELECT statement now works fine. Since the same statement works
          in one case and fails in the other, I don't think it's technically
          a syntax error. Should I bother creating a new error message, then?

          3. When writing the JUnit test I took a look at some of the negative test
          cases in store/TransactionTable.sql. I noticed that the following
          statement actually works:

          call SYSCS_UTIL.SYSCS_INPLACE_COMPRESS_TABLE(
          'SYSCS_DIAG', 'TRANSACTION_TABLE', 1, 1, 1);

          That statement works even without my changes. But on the other
          hand, the COMPRESS_TABLE call (i.e. without the "INPLACE") throws
          an error (42X62) as I would expect. Is this inconsistency a known
          issue, or should I file a separate Jira for this? As things are
          now, the above call will "work" with all VTI table names (those from
          DERBY-571 and the ones I've added with my _v1 patch)--and again,
          please note that this is not the result of my changes.

          4. As a result of my changes there is one master update that is required.
          In lang/valuesclause.sql there is a slight diff in the error message
          for three queries. With my patch the error message now shows a token
          where previously no token was given. Ex:

          select * from table target;
          < ERROR 42X01: Syntax error: Encountered "" at line 3, column 13.
          > ERROR 42X01: Syntax error: Encountered "target" at line 3, column 13.

          I assume this is okay, but if anyone feels otherwise, please let me know.

          5. The new JUnit test that I've added currently runs without a security
          manager. The reason is that two of the VTIs--ErrorLogReader and
          StatementDuration--take as an (optional) argument the name of a log file
          as input. I created a small test log file that I use for the tests, but
          when I tried running the JUnit test with a security manager, there was
          a failure when the VTI tried to read the log file. My guess is that
          this is a separate bug--perhaps a privileged block missing somewhere in
          the VTI code?--but I would appreciate a second opinion. For now I just
          have the test running with the security manager disabled; this can be
          changed when the permissions problem is resolved.

          And of course, any other comments/suggestions would be much appreciated, as well.

          Show
          A B added a comment - Attaching a first attempt at a solution for this issue. There are two patches involved: 1. d2152_engine_v1.patch: Changes to the Derby engine code to allow diagnostic VTI table mappings for VTIs that take parameters. 2. d2152_testing_v1.patch: Addition of a new JUnit test class for testing the new VTI mappings. The engine patch pretty much does what is written in the description for this issue: namely, it allows the user to query the Derby diagnostic VTIs by using the following syntax: SELECT <rcList> from TABLE (SYSCS_DIAG.<vti-table-name> (<arg list>) ) [ AS ] corrlationName Note that: a. We only support VTI table names in the SYSCS_DIAG schema b. The correlation name is required, though use of the "AS" keyword is optional (section 7.6 of the SQL 2003 spec, "<table primary>"). c. The argument list can be empty. The VTI table names that have been added are as follows: SYSCS_DIAG.SPACE_TABLE maps to org.apache.derby.diag.SpaceTable SYSCS_DIAG.ERROR_LOG_READER maps to org.apache.derby.diag.ErrorLogReader SYSCS_DIAG.STATEMENT_DURATION maps to org.apache.derby.diag.StatementDuration Given all of that, the following are some areas/issues/questions for which I am hoping to get feedback. If anyone out there can review and/or provide insight/opinions on these, I'd apprecate it. 1. Since the argument list can be empty, a user now as two different ways to invoke diagnostic VTIs that take zero parameters. Ex: A. As of DERBY-571 : SELECT * FROM SYSCS_DIAG.LOCK_TABLE B. With my patch: SELECT * FROM TABLE (SYSCS_DIAG.LOCK_TABLE()) x Is it okay to have both of these syntaxes? Note that, as it turns out, the only diagnostic VTI that does not allow zero arguments is SpaceTable; all of the other VTIs will have two different syntax variations that work. 2. What error should we throw if the user specifies a table name that is not a valid VTI table-mapped name? Ex. select * from TABLE (APP.T1()) x With the _v1 patch we'll throw a syntax error as follows: ERROR 42X01: Syntax error: APP.T1 But that does not seem particularly helpful to me. Should I add a new error message that gives more detail, or is this okay? One other thing to note about this: in some cases, this error could be misleading. Ex: set schema APP select * from TABLE (SPACE_TABLE('APP', 'T1')) x In this case we will throw a syntax error--but strictly speaking, this is not a syntax error. Rather, it's an error caused by the fact that SPACE_TABLE is not in the "APP" schema. So if, for example, I did the following: set schema SYSCS_DIAG select * from TABLE (SPACE_TABLE('APP', 'T1')) x the SELECT statement now works fine. Since the same statement works in one case and fails in the other, I don't think it's technically a syntax error. Should I bother creating a new error message, then? 3. When writing the JUnit test I took a look at some of the negative test cases in store/TransactionTable.sql. I noticed that the following statement actually works : call SYSCS_UTIL.SYSCS_INPLACE_COMPRESS_TABLE( 'SYSCS_DIAG', 'TRANSACTION_TABLE', 1, 1, 1); That statement works even without my changes. But on the other hand, the COMPRESS_TABLE call (i.e. without the "INPLACE") throws an error (42X62) as I would expect. Is this inconsistency a known issue, or should I file a separate Jira for this? As things are now, the above call will "work" with all VTI table names (those from DERBY-571 and the ones I've added with my _v1 patch)--and again, please note that this is not the result of my changes. 4. As a result of my changes there is one master update that is required. In lang/valuesclause.sql there is a slight diff in the error message for three queries. With my patch the error message now shows a token where previously no token was given. Ex: select * from table target; < ERROR 42X01: Syntax error: Encountered "" at line 3, column 13. > ERROR 42X01: Syntax error: Encountered "target" at line 3, column 13. I assume this is okay, but if anyone feels otherwise, please let me know. 5. The new JUnit test that I've added currently runs without a security manager. The reason is that two of the VTIs--ErrorLogReader and StatementDuration--take as an (optional) argument the name of a log file as input. I created a small test log file that I use for the tests, but when I tried running the JUnit test with a security manager, there was a failure when the VTI tried to read the log file. My guess is that this is a separate bug--perhaps a privileged block missing somewhere in the VTI code?--but I would appreciate a second opinion. For now I just have the test running with the security manager disabled; this can be changed when the permissions problem is resolved. And of course, any other comments/suggestions would be much appreciated, as well.
          Daniel John Debrunner created issue -

            People

            • Assignee:
              A B
              Reporter:
              Daniel John Debrunner
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development