Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-11416

Primary key on phpbb_notifications table too small

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-dev
    • Fix Version/s: 3.1.0-a1
    • Component/s: Notification System
    • Labels:
      None

      Description

      The phpbb_notifications table seems to hold per user_id data and notification_id is auto-generated and never reused. The type mediumint(8) used for notification_id may be too small for this use case.

        Activity

        Hide
        EXreaction EXreaction [X] (Inactive) added a comment -

        What should be used instead?

        Show
        EXreaction EXreaction [X] (Inactive) added a comment - What should be used instead?
        Hide
        bantu Andreas Fischer added a comment -

        That is a good question. I am not familiar enough with the notification system to answer this question. Upgrading from 3 byte medium integer to 4 byte integers would be rather easy, but I am not sure whether that is sufficient.

        Show
        bantu Andreas Fischer added a comment - That is a good question. I am not familiar enough with the notification system to answer this question. Upgrading from 3 byte medium integer to 4 byte integers would be rather easy, but I am not sure whether that is sufficient.
        Hide
        bantu Andreas Fischer added a comment -

        Since nobody can come up with perfect migrations, it seems okay to allow developers to change migration files as long as they have not been part of a release. So my proposed fix here would be changing the schema to use 4 byte integers (int(11) on MySQL) which should be sufficient for probably all larger boards.

        Show
        bantu Andreas Fischer added a comment - Since nobody can come up with perfect migrations, it seems okay to allow developers to change migration files as long as they have not been part of a release. So my proposed fix here would be changing the schema to use 4 byte integers (int(11) on MySQL) which should be sufficient for probably all larger boards.
        Hide
        EXreaction EXreaction [X] (Inactive) added a comment -

        Can the column be updated in this case with db tools? If it can I believe it would be better to use a new migration file for it.

        Show
        EXreaction EXreaction [X] (Inactive) added a comment - Can the column be updated in this case with db tools? If it can I believe it would be better to use a new migration file for it.
        Hide
        bantu Andreas Fischer added a comment - - edited

        I don't know. We either have to update the existing migration (because of the other problems anyway) or we have to add primary key support (which I am not going to do for 3.1-A1). So changing the migration seems okay to me.

        Show
        bantu Andreas Fischer added a comment - - edited I don't know. We either have to update the existing migration (because of the other problems anyway) or we have to add primary key support (which I am not going to do for 3.1-A1). So changing the migration seems okay to me.
        Hide
        EXreaction EXreaction [X] (Inactive) added a comment -

        I don't think that issue affects us here since we're just changing the column type.

        Show
        EXreaction EXreaction [X] (Inactive) added a comment - I don't think that issue affects us here since we're just changing the column type.
        Hide
        bantu Andreas Fischer added a comment -

        It probably doesn't but there is no point in adding another migration if the existing one is broken and needs to be fixed anyway. The existing migration is broken as per http://tracker.phpbb.com/browse/PHPBB3-11411 and http://tracker.phpbb.com/browse/PHPBB3-11420 and others.

        Show
        bantu Andreas Fischer added a comment - It probably doesn't but there is no point in adding another migration if the existing one is broken and needs to be fixed anyway. The existing migration is broken as per http://tracker.phpbb.com/browse/PHPBB3-11411 and http://tracker.phpbb.com/browse/PHPBB3-11420 and others.
        Hide
        EXreaction EXreaction [X] (Inactive) added a comment -

        Those all can be corrected without the need to alter existing migrations (in the case of altering primary keys it's just a bit of extra work to get around our inability to do so).

        One of the key points of migrations really is to ensure that updates are done systematically for everyone--due to this I do not think we should allow any altering of existing migrations for any reason.

        If we want to allow alteration of existing migrations we should have in place specific rules as to when, where, and for what reason they can be altered to ensure problems do not arise.

        Show
        EXreaction EXreaction [X] (Inactive) added a comment - Those all can be corrected without the need to alter existing migrations (in the case of altering primary keys it's just a bit of extra work to get around our inability to do so). One of the key points of migrations really is to ensure that updates are done systematically for everyone--due to this I do not think we should allow any altering of existing migrations for any reason. If we want to allow alteration of existing migrations we should have in place specific rules as to when, where, and for what reason they can be altered to ensure problems do not arise.

          People

          • Assignee:
            EXreaction EXreaction [X] (Inactive)
            Reporter:
            bantu Andreas Fischer
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development