Derby
  1. Derby
  2. DERBY-1909

ALTER TABLE DROP COLUMN needs to update GRANTed column privileges

    Details

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

      Description

      When ALTER TABLE DROP COLUMN is used to drop a column from a table, it needs to update the GRANTed column privileges on that table.

      The following behaviors need to be provided:

      • DROP COLUMN needs to automatically revoke any privileges which have been granted on this column
      • Privileges which have been granted on other columns in this table need to be adjusted due to changed column position numbers

      When this problem is fixed, the following additional cleanup steps should occur:

      • The check that disables DROP COLUMN in sqlAuthorization mode needs to be removed
      • There is a comment in AlterTableConstantAction that needs to be revised
      • The tests in lang/altertableDropColumn.sql should be moved into altertable.sql
      • altertableDropColumn.sql and altertableDropColumn.out should be deleted

      Search for this JIRA issue number in the code to find the relevant places to change.

      1. rewrite_syscolperms_v3.diff
        90 kB
        Bryan Pendleton
      2. rewrite_syscolperms_v2.diff
        89 kB
        Bryan Pendleton
      3. rewrite_syscolperms_v1.stat
        0.8 kB
        Bryan Pendleton
      4. rewrite_syscolperms_v1.diff
        89 kB
        Bryan Pendleton
      5. repro.sql
        0.3 kB
        Bryan Pendleton
      6. d1909_partial_incomplete.diff
        6 kB
        Bryan Pendleton

        Issue Links

          Activity

          Hide
          Bryan Pendleton added a comment -

          Committed rewrite_syscolperms_v3.diff to subversion as revision 503550.

          Show
          Bryan Pendleton added a comment - Committed rewrite_syscolperms_v3.diff to subversion as revision 503550.
          Hide
          Bryan Pendleton added a comment -

          Attached is rewrite_syscolperms_v3.diff. This patch is up to date with the head of trunk, and includes the change to EmptyDictionary.java (thanks Andrew!).

          I intend to commit this patch, unless I find any problems in my last re-run of derbyall & suites.All.

          If there are any review comments, please let me know.

          Show
          Bryan Pendleton added a comment - Attached is rewrite_syscolperms_v3.diff. This patch is up to date with the head of trunk, and includes the change to EmptyDictionary.java (thanks Andrew!). I intend to commit this patch, unless I find any problems in my last re-run of derbyall & suites.All. If there are any review comments, please let me know.
          Hide
          Bryan Pendleton added a comment -

          Attached is rewrite_syscolperms_v2.diff. This patch proposal differs from
          the v1 version only in that the code patch to DataDictionaryImpl.java
          for DERBY-1847 has been removed from this patch.

          This patch now applies cleanly against the head of trunk and is, I
          believe, ready for commit. derbyall and suites.All pass with this patch.
          Please review the patch as time permits.

          Show
          Bryan Pendleton added a comment - Attached is rewrite_syscolperms_v2.diff. This patch proposal differs from the v1 version only in that the code patch to DataDictionaryImpl.java for DERBY-1847 has been removed from this patch. This patch now applies cleanly against the head of trunk and is, I believe, ready for commit. derbyall and suites.All pass with this patch. Please review the patch as time permits.
          Hide
          Bryan Pendleton added a comment -

          Attached is rewrite_syscolperms_v1.diff, a proposed patch for this problem.

          The core of this proposed patch involves refactoring and reusing the
          DERBY-1847 method which knows how to rewrite SYSCOLPERMS rows
          to update the COLUMNS column. The DERBY-1847 version of that code
          only handled the case of adding a bit to the COLUMNS column; this patch
          extends that method to support removing a bit from the COLUMNS
          column as well, then calls the method from the AlterTable execution logic.

          The diff file is rather large because because it includes the removal of
          altertableDropColumn.sql and altertableDropColumn.out, and the
          merging of those files into altertable.sql and altertable.out, so the
          entire contents of those files are included in the diff twice each

          The rest of the diff is pretty small and straightforward and hopefully is
          not hard to review.

          The diff includes the proposed 2-line change from DERBY-1847, because
          that change is mandatory for this fix. Once the DERBY-1847 change is
          resolved, I'll revisit this diff to remove those two lines from DataDictionaryImpl.java

          The proposed patch has one particularly interesting behavior: in the case
          in which a column is dropped from a table, and there exists a user who
          was granted privileges on that column, and that column only, in that table,
          the diff processes their SYSCOLPERMS row and leaves a "empty" row; that
          is, a row in which the COLUMNS column of SYSCOLPERMS has all bits clear.
          From what I can tell, this does not cause any problems; the behavior is
          reasonable. However, it might be argued that it would be preferable to
          entirely delete the row in this case. Feedback from reviewers about this
          behavior and about how to address it (if necessary), would be much appreciated.

          Show
          Bryan Pendleton added a comment - Attached is rewrite_syscolperms_v1.diff, a proposed patch for this problem. The core of this proposed patch involves refactoring and reusing the DERBY-1847 method which knows how to rewrite SYSCOLPERMS rows to update the COLUMNS column. The DERBY-1847 version of that code only handled the case of adding a bit to the COLUMNS column; this patch extends that method to support removing a bit from the COLUMNS column as well, then calls the method from the AlterTable execution logic. The diff file is rather large because because it includes the removal of altertableDropColumn.sql and altertableDropColumn.out, and the merging of those files into altertable.sql and altertable.out, so the entire contents of those files are included in the diff twice each The rest of the diff is pretty small and straightforward and hopefully is not hard to review. The diff includes the proposed 2-line change from DERBY-1847 , because that change is mandatory for this fix. Once the DERBY-1847 change is resolved, I'll revisit this diff to remove those two lines from DataDictionaryImpl.java The proposed patch has one particularly interesting behavior: in the case in which a column is dropped from a table, and there exists a user who was granted privileges on that column, and that column only, in that table, the diff processes their SYSCOLPERMS row and leaves a "empty" row; that is, a row in which the COLUMNS column of SYSCOLPERMS has all bits clear. From what I can tell, this does not cause any problems; the behavior is reasonable. However, it might be argued that it would be preferable to entirely delete the row in this case. Feedback from reviewers about this behavior and about how to address it (if necessary), would be much appreciated.
          Hide
          Bryan Pendleton added a comment -

          I started working on a possible patch, but hit a bit of a confusion while trying
          to re-factor some code in DataDictionaryImpl.java. I don't want to lose the
          work I did, so I'm attaching d1909_partial_incomplete.diff to save that work.
          Hopefully I can pick up this work and continue it once I understand the DERBY-1847
          fix better.

          Show
          Bryan Pendleton added a comment - I started working on a possible patch, but hit a bit of a confusion while trying to re-factor some code in DataDictionaryImpl.java. I don't want to lose the work I did, so I'm attaching d1909_partial_incomplete.diff to save that work. Hopefully I can pick up this work and continue it once I understand the DERBY-1847 fix better.
          Hide
          Bryan Pendleton added a comment -

          repro.sql is a first start at a script demonstrating the problem

          Show
          Bryan Pendleton added a comment - repro.sql is a first start at a script demonstrating the problem
          Hide
          Bryan Pendleton added a comment -

          The techniques in DERBY-1847 may be useful as a building block for fixing this issue.

          Show
          Bryan Pendleton added a comment - The techniques in DERBY-1847 may be useful as a building block for fixing this issue.
          Hide
          Bryan Pendleton added a comment -

          This issue is a follow-on from DERBY-1489. DERBY-1489 implements most of the DROP COLUMN behaviors, but did not handle the interaction with GRANTed column privileges.

          Show
          Bryan Pendleton added a comment - This issue is a follow-on from DERBY-1489 . DERBY-1489 implements most of the DROP COLUMN behaviors, but did not handle the interaction with GRANTed column privileges.

            People

            • Assignee:
              Bryan Pendleton
              Reporter:
              Bryan Pendleton
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development