Use the timestamp as an identifier when editing a comment

[Remy Blank]

Currently, Ticket.modify_comment() uses the comment number stored in the oldvalue column of ticket_change to identify the comment that is edited. Unfortunately, it seems that some "comment adders" don't set this field correctly (e.g. ​XmlRpcPlugin). Also, databases from old versions of Trac (prior to the introduction of the threading feature) also don't have the oldvalue column set correctly. This prevents such comments from being edited.

The comment editing feature should be changed to use the comment timestamp as an identifier instead of the comment number.

[Remy Blank]

Mmh, using the comment time as an identifier is more difficult that I thought, because the comment number is used just about everywhere.

So another option would be to continue using the comment number, but to "interpolate" comment numbers for comments that don't have the oldvalue column set. Additionally, a database upgrade could be used to set the missing oldvalue fields. Finally, Ticket.save_change() could determine the comment number itself if the argument isn't provided.

[Remy Blank]

Fixed editing of comments where the comment number is not available in the oldvalue column of ticket_change in [9043]. The signature of Ticket.modify_comment() was also changed to take the comment date as an identifier instead of the comment number.

[Remy Blank]

[9043] introduced more robustness in the case where the comment number is not available in the oldvalue column. We could also make Ticket.save_changes() more robust, by having it find the cnum when it isn't provided:

trac/ticket/model.py

@@ -301 +301 @@
                            "VALUES (%s, %s, %s, %s, %s, %s)",
                            (self.id, when_ts, author, name, self._old[name],
                             self[name]))
+
+        # find cnum if it isn't provided
+        if not cnum:
+            cursor.execute("SELECT oldvalue FROM ticket_change "
+                           "WHERE ticket=%s AND field='comment' "
+                           "ORDER BY time DESC", (self.id,))
+            num = 0
+            for row in cursor:
+                try:
+                    num = int((row[0] or '').rsplit('.', 1)[-1]) + num
+                    break
+                except ValueError:
+                    num += 1
+            cnum = str(num + 1)
+
         # always save comment, even if empty (numbering support for timeline)
         cursor.execute("INSERT INTO ticket_change "
                        "(ticket,time,author,field,oldvalue,newvalue) "

This would ensure that the cnum is also inserted when plugins "forget" the argument. Thoughts?

[Christian Boos]

[9043] also introduced a bug apparently, see #8993.

[Remy Blank]

What about the proposed change in comment:4? Is it a good idea to generate a correct cnum if it isn't passed to save_changes()? Obviously, I'll have to adapt the patch after [9107].

[Remy Blank]

Automatically add cnum if not provided in save_changes().

[Remy Blank]

The patch above was refreshed for current trunk, and calculates cnum in save_changes() if it's not provided.

Thoughts?

[Christian Boos]

Seems fine. What about starting to use unique multiline """ ... """ strings for the SQL?

[Remy Blank]

Replying to cboos:

What about starting to use unique multiline """ ... """ strings for the SQL?

Ok, I'll do that one and then we can argue endlessly about the correct style (indentation, etc) to use ;-)

[Remy Blank]

Patch applied in [9158]. Ok with the SQL style?

[Christian Boos]

Nice! But as you expected, I'd have done it slightly differently ;-)

trac/ticket/model.py

@@ -283 +283 @@
         # find cnum if it isn't provided
         if not cnum:
             num = 0
-            cursor.execute("""\
+            cursor.execute("""
                 SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,'')
                 FROM ticket_change AS tc1
                   LEFT OUTER JOIN
                     (SELECT time,oldvalue FROM ticket_change
                      WHERE field='comment') AS tc2
                   ON (tc1.time = tc2.time)
-                WHERE ticket=%s ORDER BY tc1.time DESC""",
-                (self.id,))
+                WHERE ticket=%s ORDER BY tc1.time DESC
+                """, (self.id,))
             for ts, old in cursor:
                 # Use oldvalue if available, else count edits
                 try:

• I don't think we gain anything by avoiding the initial line break,
• if we don't move the """ at the beginning of the line, the vertical separation between the last line of the SQL and the query parameters is not immediately obvious

[Remy Blank]

Replying to cboos:

Nice! But as you expected, I'd have done it slightly differently ;-)

I would have been surprised if that wasn't the case. Or, to paraphrase the last book I read: "If two programmers in a group agree, that's a majority".

• if we don't move the """ at the beginning of the line, the vertical separation between the last line of the SQL and the query parameters is not immediately obvious

It is if your editor does syntax highlighting ;-)

Feel free to apply, I don't really mind. I'll adapt to anything (as long as I don't have to write Perl, that is).

[Christian Boos]

Replying to rblank:

Replying to cboos:

Nice! But as you expected, I'd have done it slightly differently ;-)

I would have been surprised if that wasn't the case. Or, to paraphrase the last book I read: "If two programmers in a group agree, that's a majority".

And in our group, it's even unanimity ;-)

• if we don't move the """ at the beginning of the line, the vertical separation between the last line of the SQL and the query parameters is not immediately obvious

It is if your editor does syntax highlighting ;-)

Feel free to apply, I don't really mind. I'll adapt to anything (as long as I don't have to write Perl, that is).

Oh yes, here-docs, that would be even better!

trac/ticket/model.py

@@ -283 +283 @@
         # find cnum if it isn't provided
         if not cnum:
             num = 0
-            cursor.execute("""\
+            cursor.execute(<<EOSQL, (self.id,))
                 SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,'')
                 FROM ticket_change AS tc1
                   LEFT OUTER JOIN
@@ -291 +291 @@
                      WHERE field='comment') AS tc2
                   ON (tc1.time = tc2.time)
                 WHERE ticket=%s ORDER BY tc1.time DESC""",
-                (self.id,))
+            EOSQL
             for ts, old in cursor:
                 # Use oldvalue if available, else count edits
                 try:

(we should switch to this as soon as the Grand-Unification-of-Scripting-Languages happen, I'll leave a reminder note for my kids ;-) ).

More seriously, the suggested style in comment:11 is very close to the one already adopted by the ​TracHacks:AnnouncerPlugin (​example), which is the source of inspiration for this.

Committed in r9161.

[Michael Ching <michaelc@…>]

I am trying to understand the SQL used to determine the cnum in the patch:

SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,'')
                    FROM ticket_change AS tc1
                      LEFT OUTER JOIN
                        (SELECT time,oldvalue FROM ticket_change
                         WHERE field='comment') AS tc2
                      ON (tc1.time = tc2.time)
                    WHERE ticket=? ORDER BY tc1.time DESC

If there are two transactions with the same timestamp but affecting different tickets, would this not be liable to return the comment values for all of the tickets? This in turn could result in the cnum being pulled from the incorrect ticket.

In addition, when ticket_change gets large, the outer join becomes very slow.

Adding "ticket=?" as a clause to the inner select seems like it would resolve both of these problems, but the statement could then be further simplified to something like the one in comment:4, e.g.:

SELECT time,COALESCE(oldvalue,'') FROM ticket_change
                         WHERE field='comment' AND ticket=? ORDER BY time DESC;

Please let me know if there is something which I am overlooking.

[Christian Boos]

Well spotted! For correction this could probably be fixed. Even if we now use microsecond timestamps, we might deal with older tickets with a second only time resolution (I wonder if this couldn't explain some strange numbering we've seen recently on some tickets … should have kept track of which).

If additionally the fix improves the performance, all the better! Thanks, we'll look into it.

[Remy Blank]

The current code looks… just horribly wrong. I can't for the life of me remember why I did it that way.

/me continues trying to understand

[Remy Blank]

Oh, right, the outer join is necessary to handle the case where a change doesn't have the 'comment' field. But yes, a ticket=? is missing (the same in _find_change()).

[Remy Blank]

So the simple query from comment:14 won't work in the case where a ticket change doesn't have a comment field (which shouldn't normally happen, but does in practice).

The query should return one row (time,oldvalue) for every ticket comment, where oldvalue is taken from the comment field if it exists, or an empty string otherwise. Adding the ticket=? to the inner query would make the current query correct, but performance is probably not stellar. I'm of course open to suggestions how this could be improved.

We could at least avoid the sub-query but I don't know if it would make a difference in performance:

SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,'')
FROM ticket_change AS tc1
LEFT OUTER JOIN ticket_change AS tc2
    ON tc2.ticket = ? AND tc1.time = tc2.time AND tc2.field = 'comment'
WHERE tc1.ticket = ? ORDER BY tc1.time DESC

[Remy Blank]

Suggested fix:

trac/ticket/model.py

@@ -297 +297 @@
                 cursor.execute("""
                     SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,'')
                     FROM ticket_change AS tc1
-                      LEFT OUTER JOIN
-                        (SELECT time,oldvalue FROM ticket_change
-                         WHERE field='comment') AS tc2
-                      ON (tc1.time = tc2.time)
-                    WHERE ticket=%s ORDER BY tc1.time DESC
-                    """, (self.id,))
+                    LEFT OUTER JOIN ticket_change AS tc2
+                    ON tc2.ticket=%s AND tc2.time=tc1.time
+                       AND tc2.field='comment'
+                    WHERE tc1.ticket=%s ORDER BY tc1.time DESC
+                    """, (self.id, self.id))
                 for ts, old in cursor:
                     # Use oldvalue if available, else count edits
                     try:
@@ -604 +603 @@
         # Fallback when comment number is not available in oldvalue
         num = 0
         cursor.execute("""
-            SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,''),
-               tc2.author,COALESCE(tc2.newvalue,'')
-               FROM ticket_change AS tc1
-                   LEFT OUTER JOIN
-                          (SELECT time,author,oldvalue,newvalue
-                           FROM ticket_change
-                           WHERE field='comment') AS tc2
-                        ON (tc1.time = tc2.time)
-               WHERE ticket=%s
-               ORDER BY tc1.time
-               """, (self.id,))
+            SELECT DISTINCT tc1.time,COALESCE(tc2.oldvalue,''),
+                            tc2.author,COALESCE(tc2.newvalue,'')
+            FROM ticket_change AS tc1
+            LEFT OUTER JOIN ticket_change AS tc2
+            ON tc2.ticket=%s AND tc2.time=tc1.time AND tc2.field='comment'
+            WHERE tc1.ticket=%s ORDER BY tc1.time
+            """, (self.id, self.id))
         for ts, old, author, comment in cursor:
             # Use oldvalue if available, else count edits
             try:

Performance should not be too bad, because the outer join should be using the (ticket, time, field) index.

[Michael Ching <michaelc@…>]

Thank you for the fix, we are testing it out.

I can confirm that in my initial testing, simply having the join use the ticket index resolves the performance issue. In our table (110k ticket, 1.6m ticket_change), the initial statement took up to 30 seconds to complete, with the new statement returning in less than 0.01s.

[Remy Blank]

I have applied the patch in [10630:10631] as a first step. If the performance is still bad, we can optimize from there.

The patch above is slightly wrong. If you would like to test it (i.e. if you can't update to current 0.12-stable), then please add a DISTINCT to the second SELECT. Or re-get the patch from comment:19, as I'll fix it shortly.

[Remy Blank]

This seems to be working fine now. Closing again.