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

Direct post links open the wrong page of viewtopic when multiple posts are posted in the same second

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.8
    • Fix Version/s: 3.0.10-RC1
    • Component/s: Search, Viewing posts
    • Labels:
      None

      Description

      Assume you have a direct link to a post, viewtopic.php?p=10#p10 and you have 10 posts per page in the settings.
      If post 10 and 11 in that topic are posted the same second (rare, I know, but has happened), then the above direct link would take you to page 2 instead of page 1 where the post is.

      This is because the determining of the right page counts per post time. The fix would be to also check the post id. Only counting per post id would also work but might give the wrong result if someone is messing with the database and changing post time. IMO checking for both is the best solution.

      You'll get a pull request as soon as I have learned how to do that.

        Issue Links

          Activity

          Hide
          nickvergessen Joas Schilling added a comment -

          I proposed shifting post times into the future in case of conflicts to guarantee their uniqueness. This obviously creates an issue if many posts are inserted with the same timestamp. As a compromise a limit may be imposed on how far a post time can be shifted, say 5 seconds (which is equal to 5 attempts to shift a post).

          When a post is made, after it is inserted a query is run adjusting its time forward by one second if there are other posts with its time in the same topic. This is done at most 5 times, if that does not help the original time + 5 second offset is used (this is to prevent newer posts from appearing before older posts).

          I personally would say this is not acceptable.

          I will remove the second commit (changing the index-stuff), than the initial bug is fixed for the highest part of boards being affected. The rest is a will not fix.

          Show
          nickvergessen Joas Schilling added a comment - I proposed shifting post times into the future in case of conflicts to guarantee their uniqueness. This obviously creates an issue if many posts are inserted with the same timestamp. As a compromise a limit may be imposed on how far a post time can be shifted, say 5 seconds (which is equal to 5 attempts to shift a post). When a post is made, after it is inserted a query is run adjusting its time forward by one second if there are other posts with its time in the same topic. This is done at most 5 times, if that does not help the original time + 5 second offset is used (this is to prevent newer posts from appearing before older posts). I personally would say this is not acceptable. I will remove the second commit (changing the index-stuff), than the initial bug is fixed for the highest part of boards being affected. The rest is a will not fix.
          Hide
          nickvergessen Joas Schilling added a comment -

          Removed the index change.

          Show
          nickvergessen Joas Schilling added a comment - Removed the index change.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          With the first commit only, without the index change, viewtopic does a full table scan which is not acceptable either. Have you tested this on a board with 100,000+ posts?

          Show
          Oleg Oleg [X] (Inactive) added a comment - With the first commit only, without the index change, viewtopic does a full table scan which is not acceptable either. Have you tested this on a board with 100,000+ posts?
          Hide
          nickvergessen Joas Schilling added a comment -

          It uses the same index it does without the patch (for me) on MySQL.

          Show
          nickvergessen Joas Schilling added a comment - It uses the same index it does without the patch (for me) on MySQL.
          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          Postgres query plans (with PHPBB3-10199 applied):

          before:

          SELECT COUNT(p.post_id) AS prev_posts
          FROM phpbb_posts p
          WHERE p.topic_id = 8
          AND p.post_time <= 1311484802

          Aggregate (cost=116.49..116.50 rows=1 width=4)
          -> Index Scan using phpbb_posts_topic_id on phpbb_posts p (cost=0.00..114.75 rows=694 width=4)
          Index Cond: (topic_id = 8)
          Filter: (post_time <= 1311484802)

          Elapsed: 0.00167s

          after:

          SELECT COUNT(p.post_id) AS prev_posts
          FROM phpbb_posts p
          WHERE p.topic_id = 8
          AND p.post_time <= 1311484801 AND (p.post_time < 1311484801 OR (p.post_time = 1311484801 AND p.post_id <= 2295))

          Aggregate (cost=123.30..123.31 rows=1 width=4)
          -> Index Scan using phpbb_posts_topic_id on phpbb_posts p (cost=0.00..122.06 rows=494 width=4)
          Index Cond: (topic_id = 8)
          Filter: ((post_time <= 1311484801) AND ((post_time < 1311484801) OR ((post_time = 1311484801) AND (post_id <= 2295))))

          Elapsed: 0.00182s

          The queries are essentially planned identically. The after query takes longer because it performs more checks. Note that the index is not used in full.

          In mysql the index is not used in full either before or after, and no additional information is obtainable as to how the query is actually planned. So I'm going to assume it works the same as postgres.

          Note, therefore, that viewtopic right now (without the fix) and with the fix scans through all posts in a topic when viewtopic is given the post argument (and it needs to figure out which page of results to show based on which post is being displayed).

          Based on this I recommend applying the patch which adds extra checks only and does not alter schema. If PHPBB3-10199 is merged first the patch provided here will need to be updated, and the end result might be the patch link originally posted in PHPBB3-10199.

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited Postgres query plans (with PHPBB3-10199 applied): before: SELECT COUNT(p.post_id) AS prev_posts FROM phpbb_posts p WHERE p.topic_id = 8 AND p.post_time <= 1311484802 Aggregate (cost=116.49..116.50 rows=1 width=4) -> Index Scan using phpbb_posts_topic_id on phpbb_posts p (cost=0.00..114.75 rows=694 width=4) Index Cond: (topic_id = 8) Filter: (post_time <= 1311484802) Elapsed: 0.00167s after: SELECT COUNT(p.post_id) AS prev_posts FROM phpbb_posts p WHERE p.topic_id = 8 AND p.post_time <= 1311484801 AND (p.post_time < 1311484801 OR (p.post_time = 1311484801 AND p.post_id <= 2295)) Aggregate (cost=123.30..123.31 rows=1 width=4) -> Index Scan using phpbb_posts_topic_id on phpbb_posts p (cost=0.00..122.06 rows=494 width=4) Index Cond: (topic_id = 8) Filter: ((post_time <= 1311484801) AND ((post_time < 1311484801) OR ((post_time = 1311484801) AND (post_id <= 2295)))) Elapsed: 0.00182s The queries are essentially planned identically. The after query takes longer because it performs more checks. Note that the index is not used in full. In mysql the index is not used in full either before or after, and no additional information is obtainable as to how the query is actually planned. So I'm going to assume it works the same as postgres. Note, therefore, that viewtopic right now (without the fix) and with the fix scans through all posts in a topic when viewtopic is given the post argument (and it needs to figure out which page of results to show based on which post is being displayed). Based on this I recommend applying the patch which adds extra checks only and does not alter schema. If PHPBB3-10199 is merged first the patch provided here will need to be updated, and the end result might be the patch link originally posted in PHPBB3-10199.

            People

            • Assignee:
              Oleg Oleg [X] (Inactive)
              Reporter:
              tumba25 Jari Kanerva
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development