Fossil Forum

`/vdiff?branch=' page finds incorrect diff base version
Login

`/vdiff?branch=' page finds incorrect diff base version

`/vdiff?branch=' page finds incorrect diff base version

(1) By Florian Balmer (florian.balmer) on 2024-06-13 15:22:24 [source]

On both fossil-scm.org and sqlite.org, where the active Fossil instance version is [b58b30512e] (by the time of this writing around 2024-06-13T15:12:00Z), there's the following bug in the Fossil web UI:

When viewing the timeline of any sub-branch originating from trunk (/timeline?R=BRANCH) and then clicking the Diff submenu button, the loaded diff page (/vdiff?branch=BRANCH) always takes the initial trunk commit as the left side of the comparison.

Example (with diffs hidden to prevent the loading of a huge page):

Things seem to work fine for nested sub-branches not originating directly from trunk:

Backing out the latest SQLite changes (that is, building one version behind tip) fixes the problem.

I haven't looked into the details yet, and probably won't understand them anyway, but this may be one of the WHERE EXISTS test cases you were looking for?

(2.1) By Daniel Dumitriu (danield) on 2024-06-13 23:51:33 edited from 2.0 in reply to 1 [link] [source]

This query returns the wrong answer (1 instead of 113592) with the latest SQLite changes:

WITH RECURSIVE 
  ancestor(rid, mtime) AS (
    SELECT 113580, mtime FROM event WHERE objid=113580 
    UNION 
    SELECT plink.pid, event.mtime
      FROM ancestor, plink, event
     WHERE plink.cid=ancestor.rid
       AND event.objid=plink.pid
     ORDER BY mtime DESC
  )
  SELECT ancestor.rid FROM ancestor
   WHERE EXISTS(SELECT 1 FROM tagxref
                 WHERE tagid=8 AND tagxref.rid=ancestor.rid
                   AND value='trunk' AND tagtype>0)
  LIMIT 1;

Yes, there is a WHERE EXISTS in there.

Edit: Of course, rids are local to repository.

(3) By Daniel Dumitriu (danield) on 2024-06-13 23:56:27 in reply to 2.1 [link] [source]

You can reproduce it in CLI with fossil whatis merge-in:BRANCH - at least for some branches.

(4) By Richard Hipp (drh) on 2024-06-14 17:13:39 in reply to 3 [link] [source]

Status Report:

I checked in an interim hack to deal with this issue. That check-in is not a final solution but just a temporary "ducktape and bailing wire" fix to get things going again.

The problem here turns out to not be a bug in the new EXISTS-to-JOIN optimization of SQLite, but rather a defect in the Fossil SQL statement that only came to light due to the EXISTS-to-JOIN optimization.

The defective query really does need an ORDER BY clause before the LIMIT 1. Remember that without an ORDER BY clause, SQLite is free to return the rows in any order that it wants. Prior versions of SQLite were returning the rows in the desired order naturally, so the query has been working up until now. But the EXISTS-to-JOIN optimization thought it could find a faster solution by returning the rows in a different order. That is what caused the error.

Inserting the required ORDER BY creates a big performance regression, however, because SQLite does not yet know how to reorder the order of tables in the join in order to avoid the need for a big sort operation to implement that ORDER BY.

The desired long-term fix will be:

  1. Teach SQLite that the output from a recursive CTE with an ORDER BY is in the specified order, and that if the outer query has a similar ORDER BY, it can omit its own ORDER BY by relaying on the return order of rows from the recursive CTE.

  2. Take out the "+" hack from the query in Fossil and add a proper and logically necessary ORDER BY clause. That ORDER BY will get optimized out by step 1 above, so that it will not cause a performance regression as it does now. But the presence of the ORDER BY will prevent the EXISTS-to-JOIN optimization from changing the output row order such that the wrong answer is returned.