Derby
  1. Derby
  2. DERBY-3327

SQL roles: Implement authorization stack (and SQL session context to hold it)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.5.1.1
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Security

      Description

      The current LanguageConnectionContext keeps the user authorization identifier for an SQL session.
      The lcc is shared context also for nested connections (opened from stored procedures).
      So far, for roles, the current role has been stored in the lcc also. However, SQL requires that
      authorization identifers be pushed on a "authorization stack" when calling a stored procedure, cf.
      SQL 2003, vol 2, section 4.34.1.1 and 4.27.3 and 10.4 GR 5h and i.
      This allows a caller to keep its current role after a call even if changed by the stored procedure.

      This issue will implement the current role name part ("cell") of the authorization stack.

      The authorization stack will be implemented as part of the SQL session context.
      The patch will also implement the pushing of the current unqualified schema name part of
      the SQL session context, cf. 10.4 GR 5a (DERBY-1331).

      1. releaseNote.html
        3 kB
        Dag H. Wanvik
      2. DERBY-3327-6.stat
        0.1 kB
        Dag H. Wanvik
      3. DERBY-3327-6.diff
        0.7 kB
        Dag H. Wanvik
      4. derby-3327-5a-extracted_initial_schema_patch.diff
        3 kB
        Kristian Waagan
      5. DERBY-3327-4-full-e-10_4.stat
        2 kB
        Dag H. Wanvik
      6. DERBY-3327-4-full-e-10_4.diff
        86 kB
        Dag H. Wanvik
      7. DERBY-3327-4-full-e.stat
        2 kB
        Dag H. Wanvik
      8. DERBY-3327-4-full-e.diff
        86 kB
        Dag H. Wanvik
      9. DERBY-3327-4-full-d.stat
        2 kB
        Dag H. Wanvik
      10. DERBY-3327-4-full-d.diff
        86 kB
        Dag H. Wanvik
      11. DERBY-3327-4-full-c.stat
        2 kB
        Dag H. Wanvik
      12. DERBY-3327-4-full-c.diff
        85 kB
        Dag H. Wanvik
      13. DERBY-3327-4-full-b.stat
        2 kB
        Dag H. Wanvik
      14. DERBY-3327-4-full-b.diff
        85 kB
        Dag H. Wanvik
      15. DERBY-3327-4-full.stat
        2 kB
        Dag H. Wanvik
      16. DERBY-3327-4-full.diff
        85 kB
        Dag H. Wanvik
      17. DERBY-3327-3.stat
        0.8 kB
        Dag H. Wanvik
      18. DERBY-3327-3.diff
        27 kB
        Dag H. Wanvik
      19. DERBY-3327-2.stat
        0.8 kB
        Dag H. Wanvik
      20. DERBY-3327-2.diff
        28 kB
        Dag H. Wanvik
      21. DERBY-3327-1.stat
        0.8 kB
        Dag H. Wanvik
      22. DERBY-3327-1.diff
        28 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment - - edited

          This patch implements an authorization stack for roles. The basic idea
          is as follows: For the top level, the current role is kept in the
          lcc. For dynamic call contexts (while in nested connections opened
          from a stored procedure/function), the current role is kept in the
          activation of the calling statement. The lcc keeps a stack of call
          activations to help initialize the activations so they know their
          calling activation.

          If several nested connections are opened inside a stored procedure
          they share the authorization context.

          If a dynamic result set is passed out referring to the current role
          inside a stored procedure, the correct (nested) value will result,
          since the activation of the call is still live.

          RolesTest has also been extended to test these semantics.
          The patch also fixes a bug in SetRoleConstantAction.

          For review only at this point. I have run regression tests without any
          errors. Any feedback is welcome!

          Show
          Dag H. Wanvik added a comment - - edited This patch implements an authorization stack for roles. The basic idea is as follows: For the top level, the current role is kept in the lcc. For dynamic call contexts (while in nested connections opened from a stored procedure/function), the current role is kept in the activation of the calling statement. The lcc keeps a stack of call activations to help initialize the activations so they know their calling activation. If several nested connections are opened inside a stored procedure they share the authorization context. If a dynamic result set is passed out referring to the current role inside a stored procedure, the correct (nested) value will result, since the activation of the call is still live. RolesTest has also been extended to test these semantics. The patch also fixes a bug in SetRoleConstantAction. For review only at this point. I have run regression tests without any errors. Any feedback is welcome!
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Dag. One item puzzled me: Is there a reason that getCurrentRole() and setCurrentRole() in BaseActivation are manipulating a field called nestedCurrentRole rather than a field called currentRole?

          Show
          Rick Hillegas added a comment - Thanks for the patch, Dag. One item puzzled me: Is there a reason that getCurrentRole() and setCurrentRole() in BaseActivation are manipulating a field called nestedCurrentRole rather than a field called currentRole?
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this, Rick!
          Re your question:
          Not really, just to emphaszie that this field will only ever hold the current role of a stored procedure scope; never
          the outer scope. But since it can be confusing, I will change it to currentRole and leave the it to the comments
          to explain this fact.

          Btw, I have vacillated a bit over whether to call the field holding the current role (a string)
          currentRole, currentRoleName or currentRoleId. The standard uses the term "authorization identifier"
          (generic for both user and role), but also plain "role name". But in SQL section 5.4 I see the syntax is specified as

          <authorization identifier> ::=
          <role name>

          <user identifier>

          so I will rename fields and arguments whereever appropriate to currentRoleName and roleName at some point,
          I think. Also,the current code uses "authorizationId" for user identifier in many places which can also be
          confusing now that roles are introduced. This should probably be cleaned up as well.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this, Rick! Re your question: Not really, just to emphaszie that this field will only ever hold the current role of a stored procedure scope; never the outer scope. But since it can be confusing, I will change it to currentRole and leave the it to the comments to explain this fact. Btw, I have vacillated a bit over whether to call the field holding the current role (a string) currentRole, currentRoleName or currentRoleId. The standard uses the term "authorization identifier" (generic for both user and role), but also plain "role name". But in SQL section 5.4 I see the syntax is specified as <authorization identifier> ::= <role name> <user identifier> so I will rename fields and arguments whereever appropriate to currentRoleName and roleName at some point, I think. Also,the current code uses "authorizationId" for user identifier in many places which can also be confusing now that roles are introduced. This should probably be cleaned up as well.
          Hide
          Dag H. Wanvik added a comment -

          Renamed nestedCurrentRole to currentRole as Rick suggested.
          Fixed up a few comments.

          Show
          Dag H. Wanvik added a comment - Renamed nestedCurrentRole to currentRole as Rick suggested. Fixed up a few comments.
          Hide
          Knut Anders Hatlen added a comment -

          What about changing the names of the methods instead? I don't think it's a bad idea to spell out the scope of the role, and I only read Rick's comment as saying that it would be good if the names matched.

          java.util.Stack is a synchronized collection, by the way, so you may want to consider using ArrayList instead. You have already added (push,pop)Caller() methods, so the stack abstraction will be preserved even with a different data structure.

          Show
          Knut Anders Hatlen added a comment - What about changing the names of the methods instead? I don't think it's a bad idea to spell out the scope of the role, and I only read Rick's comment as saying that it would be good if the names matched. java.util.Stack is a synchronized collection, by the way, so you may want to consider using ArrayList instead. You have already added (push,pop)Caller() methods, so the stack abstraction will be preserved even with a different data structure.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this, Knut!

          a) naming: Sounds like a good idea. You OK with that, Rick?

          b) Yes, Stack is a bit heavy being based on Vector. I was looking at
          Deque which is the recommended stack abstraction,
          (cf. comment at top of http://java.sun.com/javase/6/docs/api/java/util/Stack.html:

          "...A more complete and consistent set of LIFO stack operations is
          provided by the Deque interface and its implementations, which
          should be used in preference to this class."

          but it is not available in 1.4, so ArrayList, although less
          descriptive, is sufficient and probably the better choice here, I
          agree.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this, Knut! a) naming: Sounds like a good idea. You OK with that, Rick? b) Yes, Stack is a bit heavy being based on Vector. I was looking at Deque which is the recommended stack abstraction, (cf. comment at top of http://java.sun.com/javase/6/docs/api/java/util/Stack.html: "...A more complete and consistent set of LIFO stack operations is provided by the Deque interface and its implementations, which should be used in preference to this class." but it is not available in 1.4, so ArrayList, although less descriptive, is sufficient and probably the better choice here, I agree.
          Hide
          Dag H. Wanvik added a comment -

          New version of the patch, DERBY-3327-3, which addresses both a) and b) above.
          Running regression tests over again.

          Show
          Dag H. Wanvik added a comment - New version of the patch, DERBY-3327 -3, which addresses both a) and b) above. Running regression tests over again.
          Hide
          Rick Hillegas added a comment -

          Thanks for the new patch, Dag. Now that the method names and variable names agree, it's easier for me to read the code.

          What happens if you call popCallers() too many times? Will it fail gracefully or could this method use a little defensive logic?

          Thanks!

          Show
          Rick Hillegas added a comment - Thanks for the new patch, Dag. Now that the method names and variable names agree, it's easier for me to read the code. What happens if you call popCallers() too many times? Will it fail gracefully or could this method use a little defensive logic? Thanks!
          Hide
          Dag H. Wanvik added a comment -

          It will give an IndexOutOfBoundsException, but it should never happen unless there is a
          coding error, so I chose not to add any logic. Better to make it fail early, I think?

          Show
          Dag H. Wanvik added a comment - It will give an IndexOutOfBoundsException, but it should never happen unless there is a coding error, so I chose not to add any logic. Better to make it fail early, I think?
          Hide
          Rick Hillegas added a comment -

          Thanks, Dag. I agree that since this is an internal error (triggered by a bug in Derby) that the IndexOutOfBoundsException is good enough. Cheers-Rick

          Show
          Rick Hillegas added a comment - Thanks, Dag. I agree that since this is an internal error (triggered by a bug in Derby) that the IndexOutOfBoundsException is good enough. Cheers-Rick
          Hide
          Daniel John Debrunner added a comment -

          One high level comment and some specific comments on the patch:

          high-level
          --------------

          It seems somewhat strange that this is implemented as a stack of activations and because of that we end up needing to store role information in two different places, in the LCC if it's the top-level and the activation otherwise. Since the activation represents an individual statement is its job to hold SQL context information?

          Looking closer at the SQL standard (4.34.1.1 and 4.27.3) I'm not sure the patch is implementing exactly what is described there.
          One portion is the SQL-session context, that comes into being with the routine invocation. Then within the SQL-session context is an authorization stack.
          The patch seems to be morphing the two into one, executing a routine creates just a role stack.

          DERBY-1331 is an outstanding issue to handle SET SCHEMA within routines correctly, which I think falls into the SQL-session context logic, creating a morphed stack here might make it harder to fix that.

          Would it make more sense to have objects that followed the model described by the SQL specification, even if currently they are not fully utilized?

          Something like:
          SQLSessionContext - pushed for a routine call, stack maintained in the lcc
          has a field for schema (future for DERBY-1331)
          has a field for authorization stack (stack of AuthorizationCell)

          AuthorizationCell (see 4.34.1.1)
          field for user identifier
          field for role name

          Maybe the existing StatementContext class is already fulfilling the purpose of SQLSessionContext?????

          specific patch comments
          ----------------------------------

          For the field 'callers' in GenericLanguageConnectionContext.java I think it's holding a stack of Activations. Could you state this in its javadoc.
          (oh for Java 5

          In the javadoc comment for 'callers' it says:

          For the top level, the current role is kept here, cf. 'currentRole'.

          Since this is the javadoc for callers, the here applies to callers, but I think you really mean the current class. Would this be correct?

          For the top level, the current role is kept in this.currentRole.

          In BaseActivation can the comments for the field callActivation be javadoc comments, so that IDE's will pick them up.
          Can comments be added for nestedCurrentRole.

          Even with the comments for BaseActivation.callActivation I became a little confused. Looking elsewhere I can see that callActivation is the activation of the enclosing CALL statement, that would have been useful in the javadoc. I'm somewhat confused by the need for BaseActivation.callActivation and the stack in the LCC. It seems that the stack of activations is being kept twice, once in LCC and once through the activations.

          I wonder about the cost of this stack maintenance, for SQL routines that are defined as NO SQL it might be a significant overhead. E.g. SIN() function. Avoiding the context manipulation for those routines could be a follow on, but I'd be interested in the overhead with this change.

          Show
          Daniel John Debrunner added a comment - One high level comment and some specific comments on the patch: high-level -------------- It seems somewhat strange that this is implemented as a stack of activations and because of that we end up needing to store role information in two different places, in the LCC if it's the top-level and the activation otherwise. Since the activation represents an individual statement is its job to hold SQL context information? Looking closer at the SQL standard (4.34.1.1 and 4.27.3) I'm not sure the patch is implementing exactly what is described there. One portion is the SQL-session context, that comes into being with the routine invocation. Then within the SQL-session context is an authorization stack. The patch seems to be morphing the two into one, executing a routine creates just a role stack. DERBY-1331 is an outstanding issue to handle SET SCHEMA within routines correctly, which I think falls into the SQL-session context logic, creating a morphed stack here might make it harder to fix that. Would it make more sense to have objects that followed the model described by the SQL specification, even if currently they are not fully utilized? Something like: SQLSessionContext - pushed for a routine call, stack maintained in the lcc has a field for schema (future for DERBY-1331 ) has a field for authorization stack (stack of AuthorizationCell) AuthorizationCell (see 4.34.1.1) field for user identifier field for role name Maybe the existing StatementContext class is already fulfilling the purpose of SQLSessionContext????? specific patch comments ---------------------------------- For the field 'callers' in GenericLanguageConnectionContext.java I think it's holding a stack of Activations. Could you state this in its javadoc. (oh for Java 5 In the javadoc comment for 'callers' it says: For the top level, the current role is kept here, cf. 'currentRole'. Since this is the javadoc for callers, the here applies to callers, but I think you really mean the current class. Would this be correct? For the top level, the current role is kept in this.currentRole. In BaseActivation can the comments for the field callActivation be javadoc comments, so that IDE's will pick them up. Can comments be added for nestedCurrentRole. Even with the comments for BaseActivation.callActivation I became a little confused. Looking elsewhere I can see that callActivation is the activation of the enclosing CALL statement, that would have been useful in the javadoc. I'm somewhat confused by the need for BaseActivation.callActivation and the stack in the LCC. It seems that the stack of activations is being kept twice, once in LCC and once through the activations. I wonder about the cost of this stack maintenance, for SQL routines that are defined as NO SQL it might be a significant overhead. E.g. SIN() function. Avoiding the context manipulation for those routines could be a follow on, but I'd be interested in the overhead with this change.
          Hide
          Dag H. Wanvik added a comment -

          Re-ran regression tests then committed DERBY-3327-3 as svn 614071, resolving.

          Show
          Dag H. Wanvik added a comment - Re-ran regression tests then committed DERBY-3327 -3 as svn 614071, resolving.
          Hide
          Dag H. Wanvik added a comment -

          Sorry, Dan, I belatedly (I committed the patch) noticed you had further comments on this issue, thanks for looking at it!
          I will reopen this issue and consider them.

          Show
          Dag H. Wanvik added a comment - Sorry, Dan, I belatedly (I committed the patch) noticed you had further comments on this issue, thanks for looking at it! I will reopen this issue and consider them.
          Hide
          Daniel John Debrunner added a comment -

          np - one other item is that according to section 10.4 routine invocation the order is:

          GR2) Evaluate arguments
          GR5) Push new SQL-session context (RSC)
          GR7/8) Execute the routine

          The order in the patch is:
          A) Push new authorization context
          B) Evaluate arguments
          C) Execute the routine

          To match the order of the spec the new context would need to be part of the generated code.
          Not sure if it makes any difference or not, but something to consider.

          Ok - one other item ...
          I don't think the patch addresses functions, since the lcc.pushCaller() is only set from CallStatementResultSet.

          Performing the context setup in generated code would solve both of these issues.

          Show
          Daniel John Debrunner added a comment - np - one other item is that according to section 10.4 routine invocation the order is: GR2) Evaluate arguments GR5) Push new SQL-session context (RSC) GR7/8) Execute the routine The order in the patch is: A) Push new authorization context B) Evaluate arguments C) Execute the routine To match the order of the spec the new context would need to be part of the generated code. Not sure if it makes any difference or not, but something to consider. Ok - one other item ... I don't think the patch addresses functions, since the lcc.pushCaller() is only set from CallStatementResultSet. Performing the context setup in generated code would solve both of these issues.
          Hide
          Dag H. Wanvik added a comment -

          I'll answer in two segments, the first on the specific patch comments,
          then on the larger issues.

          • Specific patch comments

          > For the field 'callers' in GenericLanguageConnectionContext.java I
          > think it's holding a stack of Activations. Could you state this in
          > its javadoc. (oh for Java 5

          Yes, I thought I did say it ("keeps track of which, if any, stored
          procedure activations are active"), but I will make it
          clearer. Generics would have been nice here, yes

          > In the javadoc comment for 'callers' it says:
          >
          > For the top level, the current role is kept here, cf. 'currentRole'.
          >
          > Since this is the javadoc for callers, the here applies to callers,
          > but I think you really mean the current class. Would this be
          > correct?

          You are right. I have moved this comment to where it belongs, with
          "currentRole".

          > In BaseActivation can the comments for the field callActivation be
          > javadoc comments, so that IDE's will pick them up.

          Agree, will do.

          > Can comments be added for nestedCurrentRole.

          Agree, will do.

          > Even with the comments for BaseActivation.callActivation I became a
          > little confused. Looking elsewhere I can see that callActivation is
          > the activation of the enclosing CALL statement, that would have been
          > useful in the javadoc.

          Adding an explicit statement about that.

          > I'm somewhat confused by the need for BaseActivation.callActivation
          > and the stack in the LCC. It seems that the stack of activations is
          > being kept twice, once in LCC and once through the activations.

          It's probably redundant, I will try again see if I can get rid of it.

          > I wonder about the cost of this stack maintenance, for SQL routines
          > that are defined as NO SQL it might be a significant
          > overhead. E.g. SIN() function. Avoiding the context manipulation for
          > those routines could be a follow on, but I'd be interested in the
          > overhead with this change.

          Right. I'll run some tests. Moving the pushing to code generation (as
          you also) should help us get rid of the overhead for NO SQL functions,
          too.

          > np - one other item is that according to section 10.4 routine
          > invocation the order is:
          >
          > GR2) Evaluate arguments
          > GR5) Push new SQL-session context (RSC)
          > GR7/8) Execute the routine
          >
          > The order in the patch is:
          > A) Push new authorization context
          > B) Evaluate arguments
          > C) Execute the routine
          >
          > To match the order of the spec the new context would need to be part
          > of the generated code. Not sure if it makes any difference or not,
          > but something to consider.

          I think as long as we run with invoker's right it does not matter, but
          if/when we decide to add the ability to run with definer's rights it
          would, since for example an argument refering CURRENT_USER would then
          evaluate differently depending on whether the authorization stack was
          pushed or not, so it would be good to do it in the right order,

          > I don't think the patch addresses functions, since the
          > lcc.pushCaller() is only set from CallStatementResultSet.

          Yes, thanks. I had added a test (buggy!) which blind sided me there
          for a while.

          • Larger issues

          > It seems somewhat strange that this is implemented as a stack of
          > activations and because of that we end up needing to store role
          > information in two different places, in the LCC if it's the
          > top-level and the activation otherwise. Since the activation
          > represents an individual statement is its job to hold SQL context
          > information?

          It is a bit awkward, and it may be the wrong choice. I would have
          preferred an explicit representation of the new session context (SQL
          10.4's "RSC", perhaps called SQLSessionContext object, as you
          suggest), but I did not find one I looked at ConnectionContext and
          StatementContext but these classes seems to be available only at the
          JDBC level. At execution time, it seems we have to rely on what's
          reachable from the activation (lcc, transaction controller, result
          set). Somehow, the activation needs to know its dynamic nesting so
          that it can access the correct dynamic context. DERBY-1331 would seem
          to indicate we need to go this route at some point anyway, so I'll
          have a go.

          >
          > Looking closer at the SQL standard (4.34.1.1 and 4.27.3) I'm not
          > sure the patch is implementing exactly what is described there. One
          > portion is the SQL-session context, that comes into being with the
          > routine invocation. Then within the SQL-session context is an
          > authorization stack. The patch seems to be morphing the two into
          > one, executing a routine creates just a role stack.

          Right, although interpreting the standard literally (GR 5.h: "copying
          the authorization stack") seems to be inefficient, so some morphing
          may be useful

          > Would it make more sense to have objects that followed the model
          > described by the SQL specification, even if currently they are not
          > fully utilized?

          Yes, I think so.

          >
          > Something like:
          > SQLSessionContext - pushed for a routine call, stack maintained in the lcc
          > has a field for schema (future for DERBY-1331)
          > has a field for authorization stack (stack of AuthorizationCell)
          >
          > AuthorizationCell (see 4.34.1.1)
          > field for user identifier
          > field for role name
          >
          >
          > Maybe the existing StatementContext class is already fulfilling the
          > purpose of SQLSessionContext?????

          It does not seem to be reachable from the activation; being confined
          to the JDBC level? A stack of StatementContext is kept in the lcc, but
          the activation does not know which one represents it, AFAICT. I'll
          have another look, perhaps it could be extended with the information
          needed an allow references from the activation? I think it would be
          better to add new SQLSessionContext, to keep the separation between
          the JDBC and SQL level intact?

          Anyway, I'll first fix up the current solution with your comments and
          then try to work on a more general solution.

          Show
          Dag H. Wanvik added a comment - I'll answer in two segments, the first on the specific patch comments, then on the larger issues. Specific patch comments > For the field 'callers' in GenericLanguageConnectionContext.java I > think it's holding a stack of Activations. Could you state this in > its javadoc. (oh for Java 5 Yes, I thought I did say it ("keeps track of which, if any, stored procedure activations are active"), but I will make it clearer. Generics would have been nice here, yes > In the javadoc comment for 'callers' it says: > > For the top level, the current role is kept here, cf. 'currentRole'. > > Since this is the javadoc for callers, the here applies to callers, > but I think you really mean the current class. Would this be > correct? You are right. I have moved this comment to where it belongs, with "currentRole". > In BaseActivation can the comments for the field callActivation be > javadoc comments, so that IDE's will pick them up. Agree, will do. > Can comments be added for nestedCurrentRole. Agree, will do. > Even with the comments for BaseActivation.callActivation I became a > little confused. Looking elsewhere I can see that callActivation is > the activation of the enclosing CALL statement, that would have been > useful in the javadoc. Adding an explicit statement about that. > I'm somewhat confused by the need for BaseActivation.callActivation > and the stack in the LCC. It seems that the stack of activations is > being kept twice, once in LCC and once through the activations. It's probably redundant, I will try again see if I can get rid of it. > I wonder about the cost of this stack maintenance, for SQL routines > that are defined as NO SQL it might be a significant > overhead. E.g. SIN() function. Avoiding the context manipulation for > those routines could be a follow on, but I'd be interested in the > overhead with this change. Right. I'll run some tests. Moving the pushing to code generation (as you also) should help us get rid of the overhead for NO SQL functions, too. > np - one other item is that according to section 10.4 routine > invocation the order is: > > GR2) Evaluate arguments > GR5) Push new SQL-session context (RSC) > GR7/8) Execute the routine > > The order in the patch is: > A) Push new authorization context > B) Evaluate arguments > C) Execute the routine > > To match the order of the spec the new context would need to be part > of the generated code. Not sure if it makes any difference or not, > but something to consider. I think as long as we run with invoker's right it does not matter, but if/when we decide to add the ability to run with definer's rights it would, since for example an argument refering CURRENT_USER would then evaluate differently depending on whether the authorization stack was pushed or not, so it would be good to do it in the right order, > I don't think the patch addresses functions, since the > lcc.pushCaller() is only set from CallStatementResultSet. Yes, thanks. I had added a test (buggy!) which blind sided me there for a while. Larger issues > It seems somewhat strange that this is implemented as a stack of > activations and because of that we end up needing to store role > information in two different places, in the LCC if it's the > top-level and the activation otherwise. Since the activation > represents an individual statement is its job to hold SQL context > information? It is a bit awkward, and it may be the wrong choice. I would have preferred an explicit representation of the new session context (SQL 10.4's "RSC", perhaps called SQLSessionContext object, as you suggest), but I did not find one I looked at ConnectionContext and StatementContext but these classes seems to be available only at the JDBC level. At execution time, it seems we have to rely on what's reachable from the activation (lcc, transaction controller, result set). Somehow, the activation needs to know its dynamic nesting so that it can access the correct dynamic context. DERBY-1331 would seem to indicate we need to go this route at some point anyway, so I'll have a go. > > Looking closer at the SQL standard (4.34.1.1 and 4.27.3) I'm not > sure the patch is implementing exactly what is described there. One > portion is the SQL-session context, that comes into being with the > routine invocation. Then within the SQL-session context is an > authorization stack. The patch seems to be morphing the two into > one, executing a routine creates just a role stack. Right, although interpreting the standard literally (GR 5.h: "copying the authorization stack") seems to be inefficient, so some morphing may be useful > Would it make more sense to have objects that followed the model > described by the SQL specification, even if currently they are not > fully utilized? Yes, I think so. > > Something like: > SQLSessionContext - pushed for a routine call, stack maintained in the lcc > has a field for schema (future for DERBY-1331 ) > has a field for authorization stack (stack of AuthorizationCell) > > AuthorizationCell (see 4.34.1.1) > field for user identifier > field for role name > > > Maybe the existing StatementContext class is already fulfilling the > purpose of SQLSessionContext????? It does not seem to be reachable from the activation; being confined to the JDBC level? A stack of StatementContext is kept in the lcc, but the activation does not know which one represents it, AFAICT. I'll have another look, perhaps it could be extended with the information needed an allow references from the activation? I think it would be better to add new SQLSessionContext, to keep the separation between the JDBC and SQL level intact? Anyway, I'll first fix up the current solution with your comments and then try to work on a more general solution.
          Hide
          Daniel John Debrunner added a comment -

          Thanks for the detailed reply

          StatementContexts are not for the JDBC layer, they are really for the SQL layer. The current statement context can be obtained using lcc.getStatementContext(). If an activation (or its result set) is being executed in any way then it is the current statement context. Derby is only actively executing a single statement at any time for a connection.

          Another thought is that the context manager exists to handle pushing and popping of contexts, so should this existing paradigm be used to manage new contexts such as a SQLSessionContext or authentication stack. Then error cleanup would be handled consistently, rather than introducing a new mechanism with a different paradigm.

          The statement context may not match the SQLSessionContext in all cases (e.g. it maybe does for a procedure call but not for a function call).

          Show
          Daniel John Debrunner added a comment - Thanks for the detailed reply StatementContexts are not for the JDBC layer, they are really for the SQL layer. The current statement context can be obtained using lcc.getStatementContext(). If an activation (or its result set) is being executed in any way then it is the current statement context. Derby is only actively executing a single statement at any time for a connection. Another thought is that the context manager exists to handle pushing and popping of contexts, so should this existing paradigm be used to manage new contexts such as a SQLSessionContext or authentication stack. Then error cleanup would be handled consistently, rather than introducing a new mechanism with a different paradigm. The statement context may not match the SQLSessionContext in all cases (e.g. it maybe does for a procedure call but not for a function call).
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the information, it is very useful!

          When you suggest the StatementContext may be sufficient to hold the SQL session context, I presume you mean the StatementContext of the calling statement? It has (about) the same lifetime as the procedure invocation - but see question below. So the activation and/or statementContext of, say, a nested "VALUES CURRENT_USER" would reference the parent (CALL) statement context to get at the nested SQL session context? If you meant the statement context of the "VALUES CURRENT_USER", it has to get its session state from somewhere? (As I understand it, the lifetime of the statement context is limited to prepare and/or execution of a single statement.)

          What about the following case: I think the activations of dynamic result sets may need the dynamic context after the procedure has returned, say, if the statement yielding the result set references CURRENT_ROLE and that this is evaluated lazily only when the cursor is accessed (rs.next()). The statement context for the CALL has been popped (and reset) at that time (from GEnericPreparedStatement#execute), right? Would it be ok with the current model for it to stay live until the dynamic result set us closed? If not, perhaps a separate SQLConnectionContext (which could stay around longer) is better?

          Also, since the lcc.getStatementContext returns the current top context, it would not yield the right one if invoked by a dynamic result set of an activation after procedure return, I think?

          Sorry if I am misconstruing reality here; trying to understand more

          Show
          Dag H. Wanvik added a comment - Thanks for the information, it is very useful! When you suggest the StatementContext may be sufficient to hold the SQL session context, I presume you mean the StatementContext of the calling statement? It has (about) the same lifetime as the procedure invocation - but see question below. So the activation and/or statementContext of, say, a nested "VALUES CURRENT_USER" would reference the parent (CALL) statement context to get at the nested SQL session context? If you meant the statement context of the "VALUES CURRENT_USER", it has to get its session state from somewhere? (As I understand it, the lifetime of the statement context is limited to prepare and/or execution of a single statement.) What about the following case: I think the activations of dynamic result sets may need the dynamic context after the procedure has returned, say, if the statement yielding the result set references CURRENT_ROLE and that this is evaluated lazily only when the cursor is accessed (rs.next()). The statement context for the CALL has been popped (and reset) at that time (from GEnericPreparedStatement#execute), right? Would it be ok with the current model for it to stay live until the dynamic result set us closed? If not, perhaps a separate SQLConnectionContext (which could stay around longer) is better? Also, since the lcc.getStatementContext returns the current top context, it would not yield the right one if invoked by a dynamic result set of an activation after procedure return, I think? Sorry if I am misconstruing reality here; trying to understand more
          Hide
          Daniel John Debrunner added a comment -

          > (As I understand it, the lifetime of the statement context is limited to prepare and/or execution of a single statement.)

          No, the statement context is set up during the ResultSet.next() or any positioning call as well. Code that is executing or fetching rows will have the most recently pushed statement context associated with it. This is so that a statement level exception error handling affects the currently executing statement.

          With a dynamic result I believe that when it is executing (e.g. a next() call) then the most recently pushed statement context will be associated with that result set and the activation/statement that created it.

          So I think I mean only that any statement would always just refer to its StatementContext, which is the most recently pushed one since the statement must be being executed.

          Show
          Daniel John Debrunner added a comment - > (As I understand it, the lifetime of the statement context is limited to prepare and/or execution of a single statement.) No, the statement context is set up during the ResultSet.next() or any positioning call as well. Code that is executing or fetching rows will have the most recently pushed statement context associated with it. This is so that a statement level exception error handling affects the currently executing statement. With a dynamic result I believe that when it is executing (e.g. a next() call) then the most recently pushed statement context will be associated with that result set and the activation/statement that created it. So I think I mean only that any statement would always just refer to its StatementContext, which is the most recently pushed one since the statement must be being executed.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for helping me understand this better!.

          So, I can trust the correct statement context to be available at
          execution time. But it still seems not to be sufficient? The
          statement context is initialized when pushed on the basis of read-only
          information from EmbedStatement (which is a JDBC level object if I am
          not mistaken again whenever we are preparing/executing/using the
          result set of a statement, but it does not have the lifetime of the
          procedure so it can't hold the mutable state of current user, role and
          schema?

          Those would have to be stored in a data structure that would persist
          throughout the lifetime of the procedure, I think. First I looked the
          statement context of the caller. But using the debugger I see that the
          statement context of the call of not pushed back when the a next() is
          performed outside the caller on the returned dynamic result set, so
          even the statement context of the caller seems to have too short a
          lifetime to hold the dynamic state of the nested scope.

          So unless I am still missing something here, I will try to work
          towards a new context object to hold the nested SQL session state.

          Show
          Dag H. Wanvik added a comment - Thanks for helping me understand this better!. So, I can trust the correct statement context to be available at execution time. But it still seems not to be sufficient? The statement context is initialized when pushed on the basis of read-only information from EmbedStatement (which is a JDBC level object if I am not mistaken again whenever we are preparing/executing/using the result set of a statement, but it does not have the lifetime of the procedure so it can't hold the mutable state of current user, role and schema? Those would have to be stored in a data structure that would persist throughout the lifetime of the procedure, I think. First I looked the statement context of the caller. But using the debugger I see that the statement context of the call of not pushed back when the a next() is performed outside the caller on the returned dynamic result set, so even the statement context of the caller seems to have too short a lifetime to hold the dynamic state of the nested scope. So unless I am still missing something here, I will try to work towards a new context object to hold the nested SQL session state.
          Hide
          Daniel John Debrunner added a comment -

          Just to be clear, I'm not recommending the StatementContext, just trying to ensure that some understanding of it exists before another context is added. The wrong approach would be to blindly add a new context just because StatementContext is not understood. I think there is probably some overlap between the StatementContext and a SQLSessionContext, but maybe StatementContext does not quite match the role of SQLSessionContext, but then what role is StatementContext implementing in the model defined by the SQL standard? Discussion like this is good to get an understanding.

          Maybe Activation is acting as the SQLSession context, as in your first patch?

          I think in the last comment you raise an interesting point: For a procedure that sets a role R and returns dynamic result sets, the role in force during the processing of those dynamic result sets needs to be R, even though the execute of the CALL returned (and thus R was popped from some stack).

          Show
          Daniel John Debrunner added a comment - Just to be clear, I'm not recommending the StatementContext, just trying to ensure that some understanding of it exists before another context is added. The wrong approach would be to blindly add a new context just because StatementContext is not understood. I think there is probably some overlap between the StatementContext and a SQLSessionContext, but maybe StatementContext does not quite match the role of SQLSessionContext, but then what role is StatementContext implementing in the model defined by the SQL standard? Discussion like this is good to get an understanding. Maybe Activation is acting as the SQLSession context, as in your first patch? I think in the last comment you raise an interesting point: For a procedure that sets a role R and returns dynamic result sets, the role in force during the processing of those dynamic result sets needs to be R, even though the execute of the CALL returned (and thus R was popped from some stack).
          Hide
          Daniel John Debrunner added a comment -

          I see the point about dynamic result sets was already noted in the functional spec.

          Looking closely at SET ROLE I see it doesn't require a authorization stack (within a SQLSessionContext) because it only modifies the current role, it never pushes a new cell onto the authorization stack.

          So I'm coming to thinking that the initial patch is basically correct, with the exception that there is no need to have a stack of activations or to link an activation to its caller. It seems to me the current stacking of StatementContexts has the required information already, upon execution the role of the activation needs to be taken from the role of the activation of currently executing statement (the most recently pushed StatementContext) or the lcc if there is no currently statement being executed.

          Show
          Daniel John Debrunner added a comment - I see the point about dynamic result sets was already noted in the functional spec. Looking closely at SET ROLE I see it doesn't require a authorization stack (within a SQLSessionContext) because it only modifies the current role, it never pushes a new cell onto the authorization stack. So I'm coming to thinking that the initial patch is basically correct, with the exception that there is no need to have a stack of activations or to link an activation to its caller. It seems to me the current stacking of StatementContexts has the required information already, upon execution the role of the activation needs to be taken from the role of the activation of currently executing statement (the most recently pushed StatementContext) or the lcc if there is no currently statement being executed.
          Hide
          Dag H. Wanvik added a comment -

          > The wrong approach would be to blindly add a new
          > context just because StatementContext is not understood. I think there
          > is probably some overlap between the StatementContext and a
          > SQLSessionContext, but maybe StatementContext does not quite match the
          > role of SQLSessionContext, but then what role is StatementContext
          > implementing in the model defined by the SQL standard? Discussion like
          > this is good to get an understanding.

          I absolutely agree, I am happy to get your feedback on this, Dan!
          I will try to find out where Statement context fits in by reading up
          a bit in the standard.

          > Maybe Activation is acting as the SQLSession context, as in your first
          > patch?
          > I think in the last comment you raise an interesting point: For a
          > procedure that sets a role R and returns dynamic result sets, the role
          > in force during the processing of those dynamic result sets needs to
          > be R, even though the execute of the CALL returned (and thus R was
          > popped from some stack).

          I don't really grasp fully the design rationales involved in the
          existing code here, I readily admit that. I used the activation of the
          caller because it seems to have the correct lifetime to handle also
          the dynamic result case (making R available after the call has
          returned).

          What I don't like about that solution is that the asymmetry (lcc for
          outer, caller activation for inner scopes); it would be nice to have a
          SQLSessionContext uniformly available somehow from both the root
          connection scope and inside the procedures...

          Show
          Dag H. Wanvik added a comment - > The wrong approach would be to blindly add a new > context just because StatementContext is not understood. I think there > is probably some overlap between the StatementContext and a > SQLSessionContext, but maybe StatementContext does not quite match the > role of SQLSessionContext, but then what role is StatementContext > implementing in the model defined by the SQL standard? Discussion like > this is good to get an understanding. I absolutely agree, I am happy to get your feedback on this, Dan! I will try to find out where Statement context fits in by reading up a bit in the standard. > Maybe Activation is acting as the SQLSession context, as in your first > patch? > I think in the last comment you raise an interesting point: For a > procedure that sets a role R and returns dynamic result sets, the role > in force during the processing of those dynamic result sets needs to > be R, even though the execute of the CALL returned (and thus R was > popped from some stack). I don't really grasp fully the design rationales involved in the existing code here, I readily admit that. I used the activation of the caller because it seems to have the correct lifetime to handle also the dynamic result case (making R available after the call has returned). What I don't like about that solution is that the asymmetry (lcc for outer, caller activation for inner scopes); it would be nice to have a SQLSessionContext uniformly available somehow from both the root connection scope and inside the procedures...
          Hide
          Dag H. Wanvik added a comment -

          > Looking closely at SET ROLE I see it doesn't require a authorization
          > stack (within a SQLSessionContext) because it only modifies the
          > current role, it never pushes a new cell onto the authorization stack.

          Yes, thats how I understand it also, it just needs access to the top element
          somehow, but does not need to see a stack.

          > So I'm coming to thinking that the initial patch is basically correct,
          > with the exception that there is no need to have a stack of
          > activations or to link an activation to its caller. It seems to me the
          > current stacking of StatementContexts has the required information
          > already, upon execution the role of the activation needs to be taken
          > from the role of the activation of currently executing statement (the
          > most recently pushed StatementContext) or the lcc if there is no
          > currently statement being executed.

          Thanks, I will investigate this.

          Show
          Dag H. Wanvik added a comment - > Looking closely at SET ROLE I see it doesn't require a authorization > stack (within a SQLSessionContext) because it only modifies the > current role, it never pushes a new cell onto the authorization stack. Yes, thats how I understand it also, it just needs access to the top element somehow, but does not need to see a stack. > So I'm coming to thinking that the initial patch is basically correct, > with the exception that there is no need to have a stack of > activations or to link an activation to its caller. It seems to me the > current stacking of StatementContexts has the required information > already, upon execution the role of the activation needs to be taken > from the role of the activation of currently executing statement (the > most recently pushed StatementContext) or the lcc if there is no > currently statement being executed. Thanks, I will investigate this.
          Hide
          Dag H. Wanvik added a comment -

          A new version of a patch for this issue, DERBY-3327-4-full. It builds
          on the idea of the previous patch of using the activation to hold a
          references to the SQL session context at execution time. This makes
          the context available even after the original statement context has
          gone (needed for dynamic result sets). The SQL session context has
          been factored out into a separate class. The patch also makes the SQL
          session context available at compile time via the statement
          context. This is needed for the solution to work for current default
          schema (in addition to current role). The current patch also solves
          DERBY-1331.

          Code is generated for routines (which can contain SQL) to push the SQL
          session context. A new test, SQLSessionContextTest, has been factored
          out from RolesTest which tests the stack semantics for both roles and
          current default schema.

          Depending on feed-back, I may unbundle the changes for default current
          schema in this patch and retain only the current role part to reduce
          the risk; but I include it in this version of the patch as
          proof-of-concept for review.

          I am not totally happy with schema side of the patch; some usages of
          getCurrentSchemaName at execution time break the pattern in that they
          do not (and in deed need not) use the activation argument variant, in
          SpaceTable#getConglomInfo and inside
          SystemProcedures#

          {INSTALL|REPLACE|REMOVE}

          _JAR.
          I have noted this in the code as a caveat.

          All regression tests pass.

          Detailed patch comment:

          A java/engine/org/apache/derby/iapi/sql/execute/SQLSessionContext.java
          A java/engine/org/apache/derby/impl/sql/execute/SQLSessionContextImpl.java

          The new interface and implementation class which encapsulates the SQL
          session context

          M java/engine/org/apache/derby/impl/sql/compile/SpecialFunctionNode.java

          Code changes for CURRENT_ROLE and CURRENT SCHEMA.

          M java/engine/org/apache/derby/impl/sql/compile/StaticMethodCallNode.java

          Code generation to push SQL session context. There is no separate
          stack; the statement context stack is used. Also an activation still has a
          reference to the calling activation.

          M java/engine/org/apache/derby/iapi/sql/conn/StatementContext.java
          M java/engine/org/apache/derby/impl/sql/conn/GenericStatementContext.java

          New methods to set and get and a field to hold the SQL session context
          and associated activation, if any.

          M java/engine/org/apache/derby/iapi/sql/conn/LanguageConnectionContext.java
          M java/engine/org/apache/derby/impl/sql/conn/GenericLanguageConnectionContext.java

          The exisisting fields "sd" (current default schema), currentRole and
          the callers stack have been removed. Instead there is now a field
          topLevelSSC which hold the top level SQL session context and
          cachedInitialDefaultSchemaDescr, which is used to avoid having to
          compute the initial default schema for the session more than once.

          The methods get/setDefaultSchema and getCurrentSchemaName now have two
          variants one for use at compile-time and another for use at execution
          time (activation argument). A new method, resetSchemaUsages, is used
          to set refrences to a dropped schema back to the default value for all
          occurences on the SQL session state stack.

          A new method, setupNestedSessionContext, is used to push the SQL
          session context at routine invocation time, cf. StaticMethodCallNode.

          M java/engine/org/apache/derby/iapi/sql/Activation.java
          M java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java
          M java/engine/org/apache/derby/impl/sql/GenericActivationHolder.java

          Now hold both the calling activation if any (as earlier), and the SQL
          session context. A new interface method to retrieve the SQL session
          context has been added.

          M java/engine/org/apache/derby/impl/sql/execute/SetSchemaConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/DropSchemaConstantAction.java

          Uses the new execution time variants so the correct SQL session
          context can be used.

          M java/engine/org/apache/derby/impl/sql/execute/CallStatementResultSet.java

          Changed back to original; the changes having move to code generation,
          see StaticMethodCallNode.

          M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/BasicNoPutResultSetImpl.java

          Associates the current activation with the current statement context.

          M java/engine/org/apache/derby/impl/sql/GenericPreparedStatement.java

          Sets up activation with caller reference, same semantics, but now uses
          the statement context instead of the callers stack in lcc.

          M java/engine/org/apache/derby/iapi/sql/dictionary/SchemaDescriptor.java

          The drop method now takes an activation argument, and now calls
          lcc#resetSchemaUsages to clean up.

          A java/testing/org/apache/derbyTesting/functionTests/tests/lang/SQLSessionContextTest.java

          New test to check that stack semantics hold for roles end schema.

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java

          Slimmed down.

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java

          Added SQLSessionContextTest.

          Show
          Dag H. Wanvik added a comment - A new version of a patch for this issue, DERBY-3327 -4-full. It builds on the idea of the previous patch of using the activation to hold a references to the SQL session context at execution time. This makes the context available even after the original statement context has gone (needed for dynamic result sets). The SQL session context has been factored out into a separate class. The patch also makes the SQL session context available at compile time via the statement context. This is needed for the solution to work for current default schema (in addition to current role). The current patch also solves DERBY-1331 . Code is generated for routines (which can contain SQL) to push the SQL session context. A new test, SQLSessionContextTest, has been factored out from RolesTest which tests the stack semantics for both roles and current default schema. Depending on feed-back, I may unbundle the changes for default current schema in this patch and retain only the current role part to reduce the risk; but I include it in this version of the patch as proof-of-concept for review. I am not totally happy with schema side of the patch; some usages of getCurrentSchemaName at execution time break the pattern in that they do not (and in deed need not) use the activation argument variant, in SpaceTable#getConglomInfo and inside SystemProcedures# {INSTALL|REPLACE|REMOVE} _JAR. I have noted this in the code as a caveat. All regression tests pass. Detailed patch comment: A java/engine/org/apache/derby/iapi/sql/execute/SQLSessionContext.java A java/engine/org/apache/derby/impl/sql/execute/SQLSessionContextImpl.java The new interface and implementation class which encapsulates the SQL session context M java/engine/org/apache/derby/impl/sql/compile/SpecialFunctionNode.java Code changes for CURRENT_ROLE and CURRENT SCHEMA. M java/engine/org/apache/derby/impl/sql/compile/StaticMethodCallNode.java Code generation to push SQL session context. There is no separate stack; the statement context stack is used. Also an activation still has a reference to the calling activation. M java/engine/org/apache/derby/iapi/sql/conn/StatementContext.java M java/engine/org/apache/derby/impl/sql/conn/GenericStatementContext.java New methods to set and get and a field to hold the SQL session context and associated activation, if any. M java/engine/org/apache/derby/iapi/sql/conn/LanguageConnectionContext.java M java/engine/org/apache/derby/impl/sql/conn/GenericLanguageConnectionContext.java The exisisting fields "sd" (current default schema), currentRole and the callers stack have been removed. Instead there is now a field topLevelSSC which hold the top level SQL session context and cachedInitialDefaultSchemaDescr, which is used to avoid having to compute the initial default schema for the session more than once. The methods get/setDefaultSchema and getCurrentSchemaName now have two variants one for use at compile-time and another for use at execution time (activation argument). A new method, resetSchemaUsages, is used to set refrences to a dropped schema back to the default value for all occurences on the SQL session state stack. A new method, setupNestedSessionContext, is used to push the SQL session context at routine invocation time, cf. StaticMethodCallNode. M java/engine/org/apache/derby/iapi/sql/Activation.java M java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java M java/engine/org/apache/derby/impl/sql/GenericActivationHolder.java Now hold both the calling activation if any (as earlier), and the SQL session context. A new interface method to retrieve the SQL session context has been added. M java/engine/org/apache/derby/impl/sql/execute/SetSchemaConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/DropSchemaConstantAction.java Uses the new execution time variants so the correct SQL session context can be used. M java/engine/org/apache/derby/impl/sql/execute/CallStatementResultSet.java Changed back to original; the changes having move to code generation, see StaticMethodCallNode. M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java M java/engine/org/apache/derby/impl/sql/execute/BasicNoPutResultSetImpl.java Associates the current activation with the current statement context. M java/engine/org/apache/derby/impl/sql/GenericPreparedStatement.java Sets up activation with caller reference, same semantics, but now uses the statement context instead of the callers stack in lcc. M java/engine/org/apache/derby/iapi/sql/dictionary/SchemaDescriptor.java The drop method now takes an activation argument, and now calls lcc#resetSchemaUsages to clean up. A java/testing/org/apache/derbyTesting/functionTests/tests/lang/SQLSessionContextTest.java New test to check that stack semantics hold for roles end schema. M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java Slimmed down. M java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java Added SQLSessionContextTest.
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Dag. As usual, I don't have much to add that's substantive. Just a couple points:

          o I understand your misgivings about the management of the current schema. The existing code is pretty organic at this point.

          o There are a couple places in the LCC where you lookup the session context based on the Activation. It seems to me that a method like the following might be a useful abstraction:

          public SQLSessionContext getCurrentSQLSessionContext( Activation caller )
          {
          if (caller == null )

          { return getTopLevelSQLSessionContext(); }

          else

          { return caller.getNestedSQLSessionContext(); }

          }

          Show
          Rick Hillegas added a comment - Thanks for the patch, Dag. As usual, I don't have much to add that's substantive. Just a couple points: o I understand your misgivings about the management of the current schema. The existing code is pretty organic at this point. o There are a couple places in the LCC where you lookup the session context based on the Activation. It seems to me that a method like the following might be a useful abstraction: public SQLSessionContext getCurrentSQLSessionContext( Activation caller ) { if (caller == null ) { return getTopLevelSQLSessionContext(); } else { return caller.getNestedSQLSessionContext(); } }
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this, Rick! As for for the abstraction suggestion,
          I definitely agree. Uploading DERBY-3327-4-full-b, which replaces
          DERBY-3327-4-full (and earlier revs), which incorporates your suggestion!

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this, Rick! As for for the abstraction suggestion, I definitely agree. Uploading DERBY-3327 -4-full-b, which replaces DERBY-3327 -4-full (and earlier revs), which incorporates your suggestion!
          Hide
          Dag H. Wanvik added a comment -

          Uploaded DERBY-3327-4-c, which replaces earlier revs. This version cleans up the modularization a bit
          by moving SQLSessionContext

          {Impl}

          ? to sql.conn rather than sql.execute since it is not only used at execution time,
          and also removes the need to import the *Impl outside of its package by providing a create method in lcc.

          Show
          Dag H. Wanvik added a comment - Uploaded DERBY-3327 -4-c, which replaces earlier revs. This version cleans up the modularization a bit by moving SQLSessionContext {Impl} ? to sql.conn rather than sql.execute since it is not only used at execution time, and also removes the need to import the *Impl outside of its package by providing a create method in lcc.
          Hide
          Dag H. Wanvik added a comment -

          Marking with Existing application impact and Release note needed, since the latest revision of the patch, in its solution for to DERBY-1331,
          changes the semantics of SET SCHEMA if performed in a nested connection (inside a routine).

          Show
          Dag H. Wanvik added a comment - Marking with Existing application impact and Release note needed, since the latest revision of the patch, in its solution for to DERBY-1331 , changes the semantics of SET SCHEMA if performed in a nested connection (inside a routine).
          Hide
          Dag H. Wanvik added a comment -

          Ran some performance tests to see if the new code for getting current schema
          had any substantial impact on performance, but it seems not (a loop executing
          CURRENT SCHEMA inside a stored procedure saw a 3-5% performance hit, which is
          not unreasonable since access is no longer just a simple field access in lcc but
          incurs 3 extra method calls).

          If there are no further comments, I will commit this patch in a few days.

          Show
          Dag H. Wanvik added a comment - Ran some performance tests to see if the new code for getting current schema had any substantial impact on performance, but it seems not (a loop executing CURRENT SCHEMA inside a stored procedure saw a 3-5% performance hit, which is not unreasonable since access is no longer just a simple field access in lcc but incurs 3 extra method calls). If there are no further comments, I will commit this patch in a few days.
          Hide
          Dyre Tjeldvoll added a comment -

          Pushing out to 10.5

          Show
          Dyre Tjeldvoll added a comment - Pushing out to 10.5
          Hide
          Dag H. Wanvik added a comment -

          Uploaded DERBY-3327-4-full-d, which replaces *-c.

          Added a new test case to check that the current schema
          in pushed sql context stack frames is reset to the session's
          default schema when a routine drops the schema (this reverting
          is what happens to the current schema before this patch also).
          I could not find anything in the standard about this, but it seems
          the right behavior.

          Cleaned up some naming and javadocs. Regression test ran OK.

          Show
          Dag H. Wanvik added a comment - Uploaded DERBY-3327 -4-full-d, which replaces *-c. Added a new test case to check that the current schema in pushed sql context stack frames is reset to the session's default schema when a routine drops the schema (this reverting is what happens to the current schema before this patch also). I could not find anything in the standard about this, but it seems the right behavior. Cleaned up some naming and javadocs. Regression test ran OK.
          Hide
          Rick Hillegas added a comment -

          Thanks Dag,

          This sounds like reasonable and non-disruptive behavior to me.

          Show
          Rick Hillegas added a comment - Thanks Dag, This sounds like reasonable and non-disruptive behavior to me.
          Hide
          Dag H. Wanvik added a comment -

          Uploaded DERBY-3327-4-full-e which replaces *-d.

          No significant changes, just polishing:

          • Abstracted out some common code in GenericLanguageConnectionContext (
            new parameterless getCurrentSQLSessionContext).
          • More Javadoc improvements.

          Regression tests ran OK. Will commit this version soon.

          Show
          Dag H. Wanvik added a comment - Uploaded DERBY-3327 -4-full-e which replaces *-d. No significant changes, just polishing: Abstracted out some common code in GenericLanguageConnectionContext ( new parameterless getCurrentSQLSessionContext). More Javadoc improvements. Regression tests ran OK. Will commit this version soon.
          Hide
          Dag H. Wanvik added a comment -

          DERBY-3327-4-full-e committed as svn 643920.

          Show
          Dag H. Wanvik added a comment - DERBY-3327 -4-full-e committed as svn 643920.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a release note for the changed behavior of SET SCHEMA.

          Show
          Dag H. Wanvik added a comment - Attaching a release note for the changed behavior of SET SCHEMA.
          Hide
          Kristian Waagan added a comment -

          Is it feasible to backport the last fix/commit to the 10.4 branch?
          If not, I will pull out the fix for DERBY-3690 from 10.4 as it depends on this one.

          I ran regression tests on 10.4 with the patch 'derby-3327-4-full-e.diff' applied, and the only errors I got was in SQLSessionContextTest because roles are disabled. The errors happened during setup, so I do now know yet if the tests pass if we enable roles again (for testing only).

          Show
          Kristian Waagan added a comment - Is it feasible to backport the last fix/commit to the 10.4 branch? If not, I will pull out the fix for DERBY-3690 from 10.4 as it depends on this one. I ran regression tests on 10.4 with the patch 'derby-3327-4-full-e.diff' applied, and the only errors I got was in SQLSessionContextTest because roles are disabled. The errors happened during setup, so I do now know yet if the tests pass if we enable roles again (for testing only).
          Hide
          Dag H. Wanvik added a comment -

          Uploading a 10.4 version of this patch, DERBY-3327-4-full-e-10_4. The only difference
          from DERBY-3327-4-full-e, is that the test is changed by commenting out the role test cases, since
          roles is not a feature of 10.4. I verified that SQLSessionContextTest ran OK with the change on the
          head of 10.4.

          To your question, Kristian, I don't think there is a problem applying this to 10.4.

          Show
          Dag H. Wanvik added a comment - Uploading a 10.4 version of this patch, DERBY-3327 -4-full-e-10_4. The only difference from DERBY-3327 -4-full-e, is that the test is changed by commenting out the role test cases, since roles is not a feature of 10.4. I verified that SQLSessionContextTest ran OK with the change on the head of 10.4. To your question, Kristian, I don't think there is a problem applying this to 10.4.
          Hide
          Dag H. Wanvik added a comment -

          I ran the regression tests on 10.4 with the modified patch. The only errors I saw were related to MBeans, cf
          http://www.nabble.com/MBeans-errors-on-10.4-head-td17526725.html.

          Kristian, do you know exactly what changed behavior makes DERBY-3690 work? (The patch is supposed to change
          the existing Derby behavior only if one changes the current schema inside stored routines cf attached releaseNote.html)
          I would feel more comfortable committing it to the 10.4 branch if we knew...

          Show
          Dag H. Wanvik added a comment - I ran the regression tests on 10.4 with the modified patch. The only errors I saw were related to MBeans, cf http://www.nabble.com/MBeans-errors-on-10.4-head-td17526725.html . Kristian, do you know exactly what changed behavior makes DERBY-3690 work? (The patch is supposed to change the existing Derby behavior only if one changes the current schema inside stored routines cf attached releaseNote.html) I would feel more comfortable committing it to the 10.4 branch if we knew...
          Hide
          Kristian Waagan added a comment -

          Dag,

          I had a look and extracted the parts of your patch that I needed to run the failing tests successfully (see 'derby-3327-5a-extracted_initial_schema_patch.diff').
          Seems to me the changed behavior is whether the data dictionary is queried or not when setting / returning the default schema.

          I haven't verified the patch excerpt, but I'm running the regression tests now.

          Show
          Kristian Waagan added a comment - Dag, I had a look and extracted the parts of your patch that I needed to run the failing tests successfully (see 'derby-3327-5a-extracted_initial_schema_patch.diff'). Seems to me the changed behavior is whether the data dictionary is queried or not when setting / returning the default schema. I haven't verified the patch excerpt, but I'm running the regression tests now.
          Hide
          Dag H. Wanvik added a comment -

          The extracted patch looks good to me for the 10.4 branch.

          +1

          Show
          Dag H. Wanvik added a comment - The extracted patch looks good to me for the 10.4 branch. +1
          Hide
          Kristian Waagan added a comment -

          Regression tests on 10.4 ran without any failures:
          jars/sane/derby.jar] 10.4.1.4 - (661686M)
          suites.All: OK (10405 tests)
          derbyall: 100% Pass (274 tests passed)

          I'm considering committing the 5a patch to the 10.4 branch to resolve DERBY-3692. The other option is to pull out DERBY-3690.
          The code in patch 5a is extracted from 4e_full, and the only thing changed is the handling of the initial schema descriptor.

          Show
          Kristian Waagan added a comment - Regression tests on 10.4 ran without any failures: jars/sane/derby.jar] 10.4.1.4 - (661686M) suites.All: OK (10405 tests) derbyall: 100% Pass (274 tests passed) I'm considering committing the 5a patch to the 10.4 branch to resolve DERBY-3692 . The other option is to pull out DERBY-3690 . The code in patch 5a is extracted from 4e_full, and the only thing changed is the handling of the initial schema descriptor.
          Hide
          Kristian Waagan added a comment -

          Committed 'derby-3327-5a-extracted_initial_schema_patch.diff' to 10.4 with revision 662416.
          It is my understanding that nothing else of this issue will be backported to 10.4.

          Thanks for the feedback Dag!

          Show
          Kristian Waagan added a comment - Committed 'derby-3327-5a-extracted_initial_schema_patch.diff' to 10.4 with revision 662416. It is my understanding that nothing else of this issue will be backported to 10.4. Thanks for the feedback Dag!
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading DERBY-3327-6, which removes a sanity check from
          GenericLanguageConnectionContext.

          In GenericLanguageConnectionContext#getInitialDefaultSchemaDescriptor there was
          a sanity check that the initial default schema value is always computed before
          it is ever used by getDefaultSchema; that proved to not be the case in one code
          path, cf. indicative check for null in DataDictionaryImpl#getSchemaDescriptor
          near the end of that method.

          When chasing the recursion in the printing of the lock table for DERBY-3678 I
          noticed that the assert error was triggered, which removed the recursion at the
          expense of the lock table being incompetely filled in as below (notice default
          values, e.g. for CONTAINER_ID column);

          STATE TABLETYPE / LOCKOBJ INDEXNAME / CONTAINER_ID / (MODE for LATCH only) TABLENAME / CONGLOM_ID

          ------------------------------------------------------------------------------------------------------

              • The following row is the victim ***
                151 |ROW |S |0 |(1,18) |WAIT |org.apache.derby.impl.services.locks.A|CONTAINERID |CONGLOMID |
                :

          (The assert error was silently swallowed in TimeOut#buildLockTableString, see
          the comment "//just don't do anything".

          If this is a valid code path or not I have yet to determine, but I remove the
          sanity check for now; so the sane build will not work better than the insane.

          Regressions ran OK, modulo wisconsin, which i believe to be unrelated.

          Show
          Dag H. Wanvik added a comment - - edited Uploading DERBY-3327 -6, which removes a sanity check from GenericLanguageConnectionContext. In GenericLanguageConnectionContext#getInitialDefaultSchemaDescriptor there was a sanity check that the initial default schema value is always computed before it is ever used by getDefaultSchema; that proved to not be the case in one code path, cf. indicative check for null in DataDictionaryImpl#getSchemaDescriptor near the end of that method. When chasing the recursion in the printing of the lock table for DERBY-3678 I noticed that the assert error was triggered, which removed the recursion at the expense of the lock table being incompetely filled in as below (notice default values, e.g. for CONTAINER_ID column); STATE TABLETYPE / LOCKOBJ INDEXNAME / CONTAINER_ID / (MODE for LATCH only) TABLENAME / CONGLOM_ID ------------------------------------------------------------------------------------------------------ The following row is the victim *** 151 |ROW |S |0 |(1,18) |WAIT |org.apache.derby.impl.services.locks.A|CONTAINERID |CONGLOMID | : (The assert error was silently swallowed in TimeOut#buildLockTableString, see the comment "//just don't do anything". If this is a valid code path or not I have yet to determine, but I remove the sanity check for now; so the sane build will not work better than the insane. Regressions ran OK, modulo wisconsin, which i believe to be unrelated.
          Hide
          Dag H. Wanvik added a comment -

          Committed DERBY-3327-6 as svn 662911.

          Show
          Dag H. Wanvik added a comment - Committed DERBY-3327 -6 as svn 662911.
          Hide
          Dag H. Wanvik added a comment -

          A bug in this machinery was uncovered and fixed as part of DERBY-4551, cf linked issue.

          Show
          Dag H. Wanvik added a comment - A bug in this machinery was uncovered and fixed as part of DERBY-4551 , cf linked issue.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development