225221 - longdescs table needs a primary key

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Christopher Hicks]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030225
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030225

Previously discussed [e-mail] on mailing list.

My currently preferred solution is pretty simple:

alter table longdescs add cid int(10) not null auto_increment primary key;

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jouni Heikniemi]

That would cause horrendously unreadable comment numbers. I dare not even guess
what sort numbers would we be going on this particular installation, but I'd
suffer seriously if I had to enter "comment 125192519" to reference a bug
comment... Bugzilla has enough too big numbers already ;-)

CCing people who took part in the mailing list discussion.

Comment 2

[Joel Peshkin]

Just adding a primary key doesn't mean the UI has to use it as the sole way to
access a record.  The UI could still permit a comment to be referred to as
comment 1 and even generart a URL using the #c1 anchor while the DB maintains a
primary key.

Comment 3

[Christopher Hicks]

The ideas in comment 1 and comment 2 are good.  There's no reason we can't
include anchors for the human readable value #c1, #c2, etc. and the more
interesting for automation unique primary key - #c=1234, #c=34567 and so on.

As for what "appears" for the user, while most folks would be utterly
uninterested in knowing the big number PK value, it could be made a popup for
those who do want to know.

Comment 4

[Gervase Markham [:gerv]]

Remind us again why it needs a primary key? Does it make searches faster?

If there is some sort of internal comment number, it should not be exposed
anywhere in the UI. It would be just too confusing.

Gerv

Comment 5

[Christopher Hicks]

Why do we need this?
--------------------

- The database gods said every table shall have a primary key or it isn't a real
table.  This is wisdom I've found to be worth heeding.

- The database gods were expecting things like I experienced when I tried to
refine my bugzilla-based billing system.  It's a pain to refer to something if
you don't have some way of knowing that you're referring to one thing.  I wanted
a table that linked a given chunk of time expended to a given invoice.  It
should be a simple two-column table, but when I looked at longdescs it had no PK.

- MysqlTool is my favorite if somewhat abandoned web GUI for tinkering with
MySQL databases.  It is handy for tweaking records in longdescs to move comments
back in time for instance.  Yet when you try to do this it doesn't give you the
option because there's no unique key for it to refer to to be confidant it's
updating one record at a time.  I suspect any effort to make comments editable
would suffer without some primary key.  Bug 540 for instance.

- bbaetz made a comment on the mailing list about this being desirable for
making it easier to display comment #'s.  I'm not sure exactly clear what
performance bottleneck he is referring to honestly.  However, anything that's
significantly interacting with the longdescs table should find a performance
increase with the availability of the primary key.  Instead of queries needing
three fields to have a hope of a unique identifier, there would be only field.

Comment 6

[Joel Peshkin]

The lack of a repeatable key (as opposed to enumerating comments at run-time)
makes processing things like comment privacy a pain.

A better reason, though, is that Windows ODBC programs like M$ACCESS get
confused when there is no primary key.

Comment 7

[Bradley Baetz (:bbaetz)]

By 'comment number' I was thinking of a per-bug comment, so the pk is (bug_id,
comment_id). This avoids the daylight saving problem, too.

Comment 8

[Christopher Hicks]

To bbaetz:

(1) Per bug comment ids aren't necessary to solve the dst sort order problem. 
Unique ids across all of the longdescs records will sort just as well as per bug
ids.

(2) Unique ids across the entire table are necessary to solve various problems
already mentioned.

(3) Per bug ids are much harder to implement and maintain.  It's much easier to
have FK's be single fields under nearly all circumstances.

Do you still feel a two-field PK is the best way?  If so, can you explain why
better?

P.S.: I can't find the bug you're referring to as for dst causing comments sort
order issues.

Comment 9

[Christopher Hicks]

Attachment: simple approach to fixing

I was hoping to get more consensus before submitting a patch, but here's an
initial attempt.  Comments welcomed.

Comment 10

[Vlad Dascalu]

> Comments welcomed.

Can you use cvs diff -u (Unified) for making the diff? It's much easier to read.

>     $dbh->do("ALTER TABLE longdescs ADD comment_id mediumint not null
auto_increment primary key;");

Can you split that around 72 chars? :)

If you upload a new patch with those, request review on it from a Bugzilla
Reviewer ( http://bugzilla.org/who_we_are.html ). If you want a fast reply,
kiko, jouni, gerv are suitable names. :)

Comment 11

[Christopher Hicks]

Attachment: simple fix with cleanups

Followed jocuri's suggestions.

Comment 12

[Christopher Hicks]

Comment on attachment 135731 [details] [diff] [review]
simple fix with cleanups

I think this takes care of it.

Comment 13

[Christian Reis]

Comment on attachment 135731 [details] [diff] [review]
simple fix with cleanups

Christopher, the DST issue Bradley mentioned is bug 176979. I agree with you
that having a primary key and ordering by it is enough to order comments
correctly when DST strikes, but let's ask bbaetz to comment on the change.

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 135731 [details] [diff] [review]
simple fix with cleanups

The conversion code isn't sufficient - you need to set the id on all of the
existing bits, and do it in  order (ie order by bug_when, and assign them one
by one, then update the current value for the auto_increment field)

That allows us to select the comments ordering by this number, and so on.

Comment 15

[Christopher Hicks]

Considering that generally comments are already going to be in the database in
order this seems like a significant waste of effort.  I'll do patch 3 tomorrow.

Comment 16

[Jouni Heikniemi]

>Considering that generally comments are already going to be in the database in
>order this seems like a significant waste of effort.

In what cases (apart from the DST issue) could the longdescs currently be out of
order in the db?

Point being, if we already show some old comments in wrong order, I don't think
we're going to have to fix it now if it makes the conversion code significantly
bulkier and more complex.

Comment 17

[Christopher Hicks]

I doubt there are going to be that many cases when longdescs is naturally out of
order - DST issue or not.  Based on running bugzilla on MySQL 3.23 and 4.0 it
seems that records are added in order to the end.  Deletions and updates seem to
be the only way someone can "easily" get longdescs and out of order.  Since
longdescs has lacked a primary key and thus has been effectively unmodifiable
with standard tools so I doubt almost any people will have data that is
naturally out of order.

Regardless of what exists preconverstion, worrying about the effects of such
modifications and pedantically getting the PK's in useful order during
conversion to PK doesn't address the bigger long term issue of what happens when
people start modifying things.  If I change the date in the database of a
comment does it make more sense for it still be to comment 10 or should it now
be comment 5 (or whatever)?  Should I reorder the PK's again then?

Comment 18

[Christopher Hicks]

2 examples of where things can normally happen "out of order":

(1) Timestamping comments correctly when adding nightly cvs summaries to
relevant bugs.

(2) Using an e-mails date when doing an e-mail interface into bugzilla.

Comment 19

[Jouni Heikniemi]

To me, it sounds very dubious whether it's a good idea to insert emailed/cvs
updated comments above already existing comments anyway. It would potentially
make discussion _very_ hard to follow. I'd expect things to be shown in the
order in which they're entered.

Comment 20

[Gervase Markham [:gerv]]

Indeed; and the (related) reason longdescs has, up to this point, been hard to
edit is because the Bugzilla team has often taken the attitude that history is
history, and you shouldn't alter it.

If adding a PK to longdescs allows people to edit it, is that something we
actually want to encourage?

Gerv

Comment 21

[Bradley Baetz (:bbaetz)]

jouni: Thast hwy we need this. If I email in a response,and its delayed by 2-3
days for some reason, we don't want to renumber the comments.

gerv: Hidden comments is one reason - we are able to change those.

Christopher: "Based on running bugzilla on MySQL 3.23 and 4.0 it seems that
records are added in order to the end" You mean as in a select without an ORDER
BY? don't do that - the database system can do whatever it wants in that case,
including a different order each time you run the query.

I still think that (bug_id, comment_id) is enough for uniqueness. The only issue
would be if we start suporting more complex hidden sementics for bugs, and put
the restriction into a different table - we'd need a two column FK in that case.

Comment 22

[Christopher Hicks]

I am utterly discouraged by this process.

What I'm trying to do here is little different than the way attachments are
currently handled.  Using a single field simple unique identifier for that is
fine and people see the numbers going up into infinity without anybody being
disturbed.

Whether people can accept that there are valid reasons why comments might arrive
"late", there are good reasons.  We use bugzilla for tracking all of our time. 
Putting the time on the day the work was performed instead of the data entry day
makes people much, much happier and saner.  Forcing a comment number into the
database is going to make that harder with little gain.

Bradley has desires for this that go far beyond what I was ever trying to solve.
 Considering that what I've done solves the current problem in a nice clean way
and lays the necessary basis for these other things it really torques me that
this had gotten bogged down.  I strongly feel Bradley is wrong technically and
managerially.  Pushing all of this onto somebody trying to get their first
simple patch through seems a very effective way to keep the database structure
dorked.  checksetup.pl will continue to grow and grow without check.  Simple
database fixes will just die because it's not worth the battle.  It's sad.

Comment 23

[Jouni Heikniemi]

I'm sorry to hear your disappointment. It's a sad day when the issues with this
sort of development process steal from the desire to contribute code. Everyone
of us encounters the bug they want to solve simply, but everybody else wants a
perfect solution. It's not the most motivating of all things to happen on your
first bug.


I think we all agree that comment numbers have to be per bug in the UI. The
question is, whether or not the uniqueness per bug should be implemented in the
database or not. And while I understand your frustration, I wouldn't blame
Bradley for neither db-wise or managerial bad stance here. Let's face it:
Bug-wise comment ids are significantly harder to implement than what you're
suggesting. That does not indicate your solution is necessarily right. If the
needs call for a complex solution, then a simple one won't usually do. From a
code management perspective, it would be bad to check in code that fails to
achieve what we want out of this.

Now we just have to find out whether or not there will be a significant need to
handle comment references on the db level. I know a couple of situations where
gathering comment content by its relative number is necessary; one of these is
automatically pulling data from references like "bug 225221 comment 4". Another
important one is bug 113096.

Joel, Bbaetz and everybody else: Do you have further ideas where we might need
this comment number in the db?

Comment 24

[Gervase Markham [:gerv]]

What I don't get is:

- Comment numbers need to be per-bug
- Comment numbers need to not change

How can you possibly allow the insertion of out-of-order comments without
violating these two constraints? Number the comments in increments of ten so you
can insert others later?

Gerv

Comment 25

[Christian Reis]

Comment on attachment 135731 [details] [diff] [review]
simple fix with cleanups

From bbaetz' comment 14.

Comment 26

[Christopher Hicks]

Alright, I'm going to give this going a whirl again.  Here's my fresh start.

(A) I believe all tables in bugzilla should have a primary key.  This is a
necessity for being able to uniquely refer to rows.

(B) I believe a single column key is easier to work with.  We use a single
column key in attachments which is exposed to users and I don't think any harm
has been done, so using a single column key that's not exposed to the user in
any way should cause even less harm.

(C) I'm not promising that fixing this problem is going to fix any other
problem.  This is particularly relevant for bug 176979 which I regret promising
to fix.  If we can agree on how to fix this bug (which I feel is primarily a
debate between multi-column PK's and single column PK's) then I'll be happy to
move onto fixing bug 176979 by working on a patch to make the single-column PK
be in order.  I don't appreciate folks who have expanded the scope of this bug
to include fixing these other bugs (which I haven't been assigned).

Comment 27

[Christopher Hicks]

(In reply to comment #24)
> What I don't get is:
> 
> - Comment numbers need to be per-bug
> - Comment numbers need to not change
> 
> How can you possibly allow the insertion of out-of-order comments without
> violating these two constraints? Number the comments in increments of ten so
> you can insert others later?

Comment numbers can go up only incremented by 1, but the real choice is whether
to show the comments in comment# order or date order.

Comment 28

[Gervase Markham [:gerv]]

> Comment numbers can go up only incremented by 1, but the real choice is whether
> to show the comments in comment# order or date order.

The thing is, having the two orders be not the same isn't good. It would be
really confusing to have:

Additional Comment #1 From Jouni Heikniemi    2003-11-10 07:27 PDT  [reply]
Additional Comment #17 From Gervase Markham   2003-11-11 14:33 PDT  [reply]
Additional Comment #9 From Christopher Hicks  2003-11-12 19:25 PDT  [reply]
Additional Comment #2 From Dave Miller        2003-11-14 09:21 PDT  [reply]

but it would be even more confusing to have:

Additional Comment #1 From Jouni Heikniemi    2003-11-10 07:27 PDT  [reply]
Additional Comment #2 From Dave Miller        2003-11-14 09:21 PDT  [reply]
Additional Comment #9 From Christopher Hicks  2003-11-12 19:25 PDT  [reply]
Additional Comment #17 From Gervase Markham   2003-11-11 14:33 PDT  [reply]

Gerv

Comment 29

[Gervase Markham [:gerv]]

BTW, I should say that I happily agree with your A), B) and C), and I'm not
trying to frustrate you here. I don't have a particular objection to a primary
key for longdescs if we can show that one is vaguely useful (and you seem to
strongly think it is.)

But I'm very concerned about allowing the insertion of out-of-order comments. I
can't see any way to permit this without ending up with something that sucks
from a usability perspective. So if that's the only reason for a longdescs
primary key, then I don't see the need. If there are other reasons, cool, let's
enumerate them and do it, and have the out-of-order discussion in another bug.

Gerv

Comment 30

[Christopher Hicks]

(In reply to comment #28)
> The thing is, having the two orders be not the same isn't good.

Comment 18 explains the need, what else would you propose to do to meet those needs?

Comment 31

[Christopher Hicks]

(In reply to comment #29)
> BTW, I should say that I happily agree with your A), B) and C), and I'm not
> trying to frustrate you here.

That's good.

> I don't have a particular objection to a primary key for longdescs if we can
> show that one is vaguely useful (and you seem to strongly think it is.)

Its truly difficult for me to believe that noone with any database design
knowledge hasn't pointed this out long, long ago.  Read any of CJ Dates books
and every other chapter will mention the fact in passing.  Its some of the most
basic common database design knowledge out there!

http://www.standardreporting.net/survival/view.aspx?_@id=53484

> But I'm very concerned about allowing the insertion of out-of-order comments.

Reality is that email and PDAs are asynchronous devices.

> I can't see any way to permit this without ending up with something that sucks
> from a usability perspective.

We're doing this now and it sucks much less than the alternative.  The
alternative was to have everything asynchronous say in the note "originated on X
date".  That was ugly, confusing, and required repeated explanations to clients.

> So if that's the only reason for a longdescs
> primary key, then I don't see the need.

Have you ever read anything on database design?

> If there are other reasons, cool, let's
> enumerate them and do it, and have the out-of-order discussion in another bug.

All tables need primary keys.  Its necessary so that you can uniquely refer to a
given row.  This is a basic precept of relational database design.  Every table
needs a primary key.  How bugzilla got this far without PK's is a really
fascinating question though.

Comment 32

[Christian Reis]

(In reply to comment #31)
> All tables need primary keys.  Its necessary so that you can uniquely refer to a
> given row.  This is a basic precept of relational database design.  Every table
> needs a primary key.  How bugzilla got this far without PK's is a really
> fascinating question though.

I agree with Chris here, for the record. The specifics here may make that
difficult, but I do think it's certainly a goal to have a PK applied to longdescs.
I recently added variable comment tags locally, and replicating the data over
into related tables was horrible. I wish this bug was solved back then.

Comment 33

[Gervase Markham [:gerv]]

OK, then. Let's do the PK thing, and if Chris wants to try and get something
that does out-of-order comments into Bugzilla in another bug, we can have a
fight about it there :-)

bbaetz is our database design guru, and we normally get him to sign off on
schema changes. If you can convince him of what you want to do, it's cool with
me. Comment 21 was his last thought...

Gerv

Comment 34

[Christopher Hicks]

(In reply to comment #33)
> OK, then. Let's do the PK thing, and if Chris wants to try and get something
> that does out-of-order comments into Bugzilla in another bug, we can have a
> fight about it there :-)

Bug 264057 it is.  Feel free to assign it to me.

> bbaetz is our database design guru, and we normally get him to sign off on
> schema changes. If you can convince him of what you want to do, it's cool with
> me. Comment 21 was his last thought...

So I'm back to being nowhere on this.  Lovely.

Comment 35

[Christian Reis]

I'd work with comment 21, just ignoring the last paragraph, since we've agreed
that a primary key on longdescs is indeed convenient (I'm joining on it to other
tables in some custom patches I have flying around).

Comment 36

[Christopher Hicks]

(In reply to comment #35)
> I'd work with comment 21, just ignoring the last paragraph, since we've agreed
> that a primary key on longdescs is indeed convenient

How can I ignore it?  Gerv says bbaetz is the database guru and he's not
weighing in any further.  I disagree with his last paragraph and without some
agreement on how to fix this I'm dead in the water.  Is someone official willing
to say "a single value PK is fine" so I can proceed?

> (I'm joining on it to other
> tables in some custom patches I have flying around).

I do an outer join on longdescs to a "billed" table to determine what's not
billed.  I can do that with a one or two part key, but a one part key still
seems much cleaner for all involved.

Comment 37

[Christian Reis]

Both Gerv and I have said it. I'm second-place in reviews so I hold some
authority at least I hope :-)

Comment 38

[Christopher Hicks]

(In reply to comment #37)
> Both Gerv and I have said it. I'm second-place in reviews so I hold some
> authority at least I hope :-)

Since Gerv seemed to be deferring to bbaetz I wasn't clear.  I'll produce a new
patch for this against a current version.

Comment 39

[Dave Miller [:justdave]]

(In reply to comment #33)
> bbaetz is our database design guru, and we normally get him to sign off on
> schema changes. If you can convince him of what you want to do, it's cool with
> me. Comment 21 was his last thought...

Based on the rest of the comments I've seen here, I'll sign off on it. :)

Do we have an updated patch yet?

I won't block release for this, but I'll certainly take it if it gets done
before then.

Comment 40

[Stephen Lee]

Bug 281354, which I recently filed proposes an alternative mechanism by which a 
single-column UNIQUE key would be generated for the longdescs table (which I 
think MySql treats as a PK in absence of a "real" PK).

Comments on here about the desirability of properly increasing ID values for 
historical data would also apply to that. A simple comment_id as proposed here 
would do no harm, but would become redundant if both were implemented.

Comment 41

[Shane H. W. Travis]

Just ran across a comment in 
https://bugzilla.mozilla.org/show_bug.cgi?id=52557#c187  saying:

"This patch also includes a patch for bug 225221. I was too lazy to remove that
patch from this one."

Haven't looked at his solution there.

Comment 42

[Stephen Lee]

(In reply to comment #41)
> https://bugzilla.mozilla.org/show_bug.cgi?id=52557#c187
> Haven't looked at his solution there.

It's not a different solution - he had applied attached to this bug.

Comment 43

[Max Kanat-Alexander]

I would agree that a single-column PK is more flexible, even if a dual-column PK
seems to make more sense based on the architecture that we have now.

I think the reason that bbaetz was thinking in terms of dual-column PK is all of
the "bug X comment Y" links that exist in the world. That is, Bugzilla already
has a sort of history where comments aren't alterable.

However, having a single-column PK works for Attachments, you're right. And it
would probably make threaded comments easier, and also would probably make
solving bug 540 easier.

Of course, we'd still have to always display the correct bug-relative comment
number to the end-user (and those numbers can't change -- think of all the #c34
or something like that links in the world). So why not make a single-column PK
for this bug, and then on another bug we can add comment_id?

I'll do the review for the single-column PK, if you will write it.

Comment 44

[Stephen Lee]

The r- for the current patch was based on comment 14, which only applies if we 
need to be able to do an "ORDER BY comment_id" instead of "ORDER BY bug_when".

This seems short-sighted, since the logical solution to bug 540 would involve a 
new row to longdescs that replaces the existing row. This would (by definition) 
have a higher comment_id than any comments submitted in-between, but should 
still appear in the original comment's position.

Bug 281354 would provide an update_id that was linked to a particular bug 
change, and this could be used (amongst other things) for ordering comments. 
This could also allow extensions that would make a comment_id completely 
meaningless except for referencing a comment's text (bug 281354 comment 9).


IMHO, this bug should merely provide a PK to the longdescs table, AND NOTHING 
ELSE. i.e. the existing patch would be all that is required for the version to 
which it applies.

I expect that being over a year old it is rather bitrotten though.

Comment 45

[Christopher Hicks]

Sorry for being out of touch and not cleaning this up for submission yet.  I got
Max's note and decided to punt until 2.20.

Comment 46

[Frédéric Buclin]

The lack of a primary key for this table is really irritating me. Could you
please submit a patch asap? We should unfreeze in the next few days and I would
be really happy to see thix bug being fixed. Adding bug 152899 in the blocking
list, in order to know when to hack AppendComment to return the comment's ID.

Comment 47

[Max Kanat-Alexander]

This is now blocking enough bugs and causing enough trouble that I think it's become a major issue.

Comment 48

[Albert Ting]

I would vote to have comment_id represent the comment # for that specific bug.  Main reason is that one can nicely record changes to "private" comments in the Bug Activity page.  Moreover, the Bug.pm and template routines could then use the sql tables to get the comment #, rather than auto-calculate this on the fly.

Comment 49

[Albert Ting]

Attachment: cvs tip patch

(In reply to comment #44)
> IMHO, this bug should merely provide a PK to the longdescs table, AND NOTHING 
> ELSE. i.e. the existing patch would be all that is required for the version to 
> which it applies.

I thought about it some more and agree with comment #44.  A simple PK will do for now and can use this patch for some enhancements I'm working on.

Comment 50

[Albert Ting]

Attachment: cvs tip patch v2

My bad, patch was created incorrectly.

Comment 51

[Max Kanat-Alexander]

Comment on attachment 205349 [details] [diff] [review]
cvs tip patch v2

Sure, I agree.

We can add a comment_num field later, if we want a per-bug comment_id.

Comment 52

[Max Kanat-Alexander]

Comment on attachment 205349 [details] [diff] [review]
cvs tip patch v2

>+# 2005-12-07 -- Bug 225221
>+$dbh->bz_add_column('longdescs', 'comment_id',
>+                    {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>+

  Oh, except, wait -- you put this in the wrong place. The new Dev Guide explains where this is *supposed* to go. Look for the last occurence of "--TABLE--".

Comment 53

[Albert Ting]

Attachment: cvs tip patch v3

Ok, moved the checksetup code in the right place.  Thanks.

Comment 54

[Max Kanat-Alexander]

Comment on attachment 205401 [details] [diff] [review]
cvs tip patch v3

Yes, good. :-) I'm tired of this bug being open. :-)

Comment 55

[Frédéric Buclin]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.471; previous revision: 1.470
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.47; previous revision: 1.46
done

Comment 56

[Vlad Dascalu]

This might have caused bug 328063 regression.

Comment 57

[Kristis Makris]

(In reply to comment #51)
> (From update of attachment 205349 [details] [diff] [review] [edit])
> Sure, I agree.
> 
> We can add a comment_num field later, if we want a per-bug comment_id.

I need a per-bug comment_id to produce a Version Description Document in Scmbug by querying the Bugzilla database. I would need to list for a specific bug the comments that were added between a date range. So I would need to report something like:

Bug 1, comment 65:

Bug 1, comment 68:

etc.

Without per-bug comment ids, this will be very difficult to do.

Comment 58

[Ravinder]

test

Comment 59

[Ravinder]

test

Comment 60

[Max Kanat-Alexander]

Okay, this doesn't *actually* need a relnote, now that I think of it. I was just so excited that it was fixed that I added the relnote keyword.