Last Comment Bug 98304 - (bz-postgres) Make Bugzilla's SQL generally compatible with PostgreSQL (PgSQL)
(bz-postgres)
: Make Bugzilla's SQL generally compatible with PostgreSQL (PgSQL)
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Database (show other bugs)
: 2.15
: All All
: P1 enhancement with 51 votes (vote)
: Bugzilla 2.20
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
http://sql-info.de/postgresql/postgre...
: 1104 (view as bug list)
Depends on: 43600 114696 bz-dbschema 156834 174295 182136 190432 228917 bz-dbcompat 248001 bz-dbinstall 250547 250591 250832 251960 253357 253360 280493 281550 284125 284244 285397 285401 285403 285407 285411 285532 285534 285552 285555 285678 285692 285695 285705 285740 285748 286235 286360 286392 286490 286686 286695 287138 289012 289042 289043 291803 292119 292715 292718 292768 293015 296039 296054 296075
Blocks: bz-oracle 274266
  Show dependency treegraph
 
Reported: 2001-09-04 19:47 PDT by Dave Miller [:justdave] (justdave@bugzilla.org)
Modified: 2016-08-24 16:59 PDT (History)
57 users (show)
justdave: blocking2.18-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
PostgreSQL first pass patch to current Mozilla CVS (v0.1) (114.69 KB, patch)
2001-10-25 11:25 PDT, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
PostgreSQL second pass patch to current Mozilla CVS (v0.2) (114.76 KB, patch)
2001-10-28 23:30 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Setup utility similar to checksetup.pl except for PostgreSQL (85.97 KB, text/plain)
2001-10-29 00:21 PST, David Lawrence [:dkl]
no flags Details
PostgreSQL configuration utility similar to checksetup.pl v(1) (122.61 KB, text/plain)
2002-09-23 15:10 PDT, David Lawrence [:dkl]
no flags Details
Port CVS Head to PostgreSQL (v1) (72.60 KB, patch)
2002-09-23 15:14 PDT, David Lawrence [:dkl]
bbaetz: review-
Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v2) (56.56 KB, patch)
2002-09-25 17:14 PDT, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v3) (69.44 KB, patch)
2002-10-09 15:08 PDT, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v4) (67.96 KB, patch)
2002-11-07 20:38 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Splinter Review
105532: Port CVS Head to PostgreSQL (v5) (86.53 KB, patch)
2002-11-22 17:49 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v5) (86.53 KB, patch)
2002-11-22 17:49 PST, David Lawrence [:dkl]
bbaetz: review-
Details | Diff | Splinter Review
Schema for Bugzilla (16.05 KB, text/plain)
2003-08-28 14:45 PDT, Jeroen Ruigrok van der Werven
no flags Details
Compatibility layer for PostgreSQL (6.18 KB, text/plain)
2003-09-01 15:02 PDT, Jeroen Ruigrok van der Werven
no flags Details
Updated and partially rewritten pgsetup.pl file (82.51 KB, text/plain)
2003-09-04 13:50 PDT, Jeroen Ruigrok van der Werven
no flags Details
PostgreSQL compatibility layer, revision 1 (9.14 KB, text/plain)
2003-09-05 15:22 PDT, Jeroen Ruigrok van der Werven
no flags Details
Proof of Concept code for DB independant schema generation (22.78 KB, text/plain)
2003-11-13 12:09 PST, Andrew Dunstan
no flags Details
Port CVS Head to PostgreSQL (v6) (132.80 KB, patch)
2004-02-16 15:56 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v7) (99.50 KB, patch)
2004-02-18 15:19 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Changes to implement Table Locking in PostgreSQL using DBcompat.pm (23.79 KB, patch)
2004-02-20 14:29 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
Port CVS Head to PostgreSQL (v8) (127.68 KB, patch)
2004-02-27 15:16 PST, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review

Description Dave Miller [:justdave] (justdave@bugzilla.org) 2001-09-04 19:47:29 PDT
David Lawrence is already in the process of converting bugzilla so that it will
work with either MySQL or PgSQL.  I couldn't find a bug to go with it, so I'm
creating one that the necessary patches can be posted to once they've gotten to
a point of being usable.
Comment 1 David Lawrence [:dkl] 2001-10-25 11:25:17 PDT
Created attachment 55101 [details] [diff] [review]
PostgreSQL first pass patch to current Mozilla CVS (v0.1)
Comment 2 Matthew Tuck [:CodeMachine] 2001-10-25 22:06:44 PDT
A few quick comments from a quick look:

I think the driver statements should check the driver is PostgreSQL too, and
give fail appropriately if it doesn't recognise the driver.  When we add
drivers, I think it's better to give explicit error messages.

I think that the REGEXP/~ stuff, INSTR/STRPOS stuff, etc should be put in subs,
eg RegExpSQL ($$).  This will make it easier to add new drivers to the code base
as well as making the code more readable because XD support isn't in the way.

Why is there still groupbitset stuff in here?  I thought we were removing that
stuff?
Comment 3 David Lawrence [:dkl] 2001-10-25 23:03:43 PDT
I agree that smaller utility functions would make the code cleaner and require
less changes throughout all the files such as things like SqlDate and the
regular expression stuff. I will make the necessary changes in the next day or
so. I left in the groupbit stuff to basically make the PostgreSQL port
functionally equivalent to the current Mozilla code base. The group changes are
a separate patch that will be merged into separately.
Comment 4 Matthew Tuck [:CodeMachine] 2001-10-26 01:43:05 PDT
*** Bug 1104 has been marked as a duplicate of this bug. ***
Comment 5 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-10-27 04:15:27 PDT
This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.
Comment 6 David Lawrence [:dkl] 2001-10-28 23:30:05 PST
Created attachment 55518 [details] [diff] [review]
PostgreSQL second pass patch to current Mozilla CVS (v0.2)
Comment 7 David Lawrence [:dkl] 2001-10-28 23:38:13 PST
Just attached second pass patch for PostgreSQL. Main differences are the
addition of two SQL utility functions SqlRegEx and SqlStrSearch. These replace
the REGEXP and INSTR function used by MySQL with functions that return the
proper syntax depending on which database you are currently using. Also I have
changed syntax from 

if ($::driver eq 'mysql) {

} else {

}

to

if ($::driver eq 'mysql') {

} elsif ($::driver eq 'Pg') {

}

buglist.cgi is currently broken due to a LEFT JOIN added in by the new
SelectVisible() function. It generates the following error:

SELECT bugs.bug_id, bugs.groupset, substring(bugs.bug_severity, 1, 3),
substring(bugs.priority, 1, 3), substring(bugs.rep_platform, 1, 3),
map_assigned_to.login_name, substring(bugs.bug_status,1,4),
substring(bugs.resolution,1,4), substring(bugs.short_desc, 1, 60) FROM bugs,
profiles map_assigned_to, profiles map_reporter LEFT JOIN cc selectVisible_cc ON
bugs.bug_id = selectVisible_cc.bug_id AND selectVisible_cc.who = 1 WHERE
((bugs.groupset & int8(9223372036854775807)) = bugs.groupset OR
(bugs.reporter_accessible = 1 AND bugs.reporter = 1) OR
(bugs.assignee_accessible = 1 AND bugs.assigned_to = 1) OR
(bugs.qacontact_accessible = 1 AND bugs.qa_contact = 1) OR
(bugs.cclist_accessible = 1 AND selectVisible_cc.who = 1 AND
selectVisible_cc.who IS NOT NULL)) AND bugs.assigned_to = map_assigned_to.userid
AND bugs.reporter = map_reporter.userid AND (bugs.bug_status = 'NEW' OR
bugs.bug_status = 'ASSIGNED' OR bugs.bug_status = 'REOPENED') GROUP BY
bugs.bug_id ORDER BY bugs.bug_status, bugs.resolution, bugs.bug_status,
map_assigned_to.login_name, bugs.bug_status, priority, bugs.bug_id: ERROR:
JOIN/ON clause refers to "bugs", which is not part of JOIN at globals.pl line 235.

If someone has an idea on how to fix the error let me know.
I also need to attach the pgsetup.pl script for creation of PostgreSQL tables as
soon as I sync it up with the current checksetup.pl.
Comment 8 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-10-29 00:00:36 PST
V0.2 has been checked in on branch Bugzilla_PgSQL_branch

you can get this with:

cvs checkout -rBugzilla_PgSQL_branch Bugzilla

or

cvs update -rBugzilla_PgSQL_branch

If you don't know how to merge with the trunk (which we'll need to do somewhat
frequently until this is ready for prime time) I can show you how to do that.

I also set up a tinderbox testing this, and it fails (Burning).  See
http://tinderbox.mozilla.org/Bugzilla/ and click the "L" to look at the error
logs.  If it's the test's fault because of something PgSQL does that we don't
handle correctly, let us know so we can fix the test. :)
Comment 9 Matthew Tuck [:CodeMachine] 2001-10-29 00:04:02 PST
Try moving the bugs table in the FROM part to just left of the LEFT JOIN?
Comment 10 David Lawrence [:dkl] 2001-10-29 00:21:08 PST
Created attachment 55523 [details]
Setup utility similar to checksetup.pl except for PostgreSQL
Comment 11 Bradley Baetz (:bbaetz) 2001-10-29 10:10:13 PST
Yeah, you need to have bugs LEFT JOIN cc, I think.

If you can't rework the regexp to do this (and doing so may break other
queries), then you'll need to do:

bugs AS selectVisible_bugs LEFT JOIN cc AS selectVisible_cc ON ...

and then add selectVisible_bugs.bug_id = bugs.bug_id to the WHERE part, unless
you can think of something better. I'd think that that would be slower, though.
Comment 12 Matthew Tuck [:CodeMachine] 2001-11-17 22:41:53 PST
We might do some of this for 2.16 but not all, so moving off to 2.18.
Comment 13 David Lawrence [:dkl] 2001-12-10 14:05:44 PST
I have made a change to CanSeeBug to alleviate some of the joining problems.
Please see my comment made on bug 68022 which is relevant to this report also.
Comment 14 Bradley Baetz (:bbaetz) 2002-01-20 03:59:46 PST
From IRC:

<bbaetz> I think we should do this in stages
<bbaetz> 1. group stuff
<bbaetz> 2. " -> '
<bbaetz>  /other sql syntax changes which don't affect mysql
<bbaetz> 3. checksetup/conversion/etc
<bbaetz> 4. transations
<bbaetz> 5. constraints
<bbaetz> oh, and 5 may include ON DELETE CASCADE type stuff

4 and 5 may be swapped arround/combined/etc
Comment 15 Bradley Baetz (:bbaetz) 2002-01-20 04:39:12 PST
Oh, and somewhere in there we need to handle locking (course grained locking
before transactions are supported are ok).

The pgsql branch currently just wraps all calls to LOCK/UNLOCK in an if
($::driver eq 'mysql') block - thats not a good idea...
Comment 16 David Lawrence [:dkl] 2002-01-20 10:58:46 PST
The reason I conditioned out the LOCK stuff to be only MySQL is because PgSQL
handles locking pretty well out of the box. It does also support explicit
locking using LOCK and UNLOCK but the syntax is a little different then MySQL so
I chose the other route.
Comment 17 Bradley Baetz (:bbaetz) 2002-01-20 17:14:32 PST
I must be missing something, then. Whats to stop two simultanious updates to a
bug, if we don't do locking? If I add an attachment (which for some reason
doesn't  use an auto_increment field), whats to stop one getting one id, and the
other getting the same id (and them failing due to the uniqueness test)?
Comment 18 David Lawrence [:dkl] 2002-01-20 18:21:49 PST
PostgreSQL has a special locking mechanism called MVCC (Multi Version
Concurrency Control). For more information on this check out:

http://www.postgresql.org/idocs/index.php?mvcc.html

Basically when a access the databasem he/she gets a current snapshot of the
database to themselves. If they need a sequence number it grabs the next id, if
they do not use it and their session ends it does not go back in the pool so it
is lost. I dont really see this happening alot but if it does it is only a
single number gap worst case.

PostgreSQL does support SELECT FOR UPDATE so if we move in that direction for
the future, it shouldnt be a problem to get rid of the conditionals at that time.
Comment 19 Bradley Baetz (:bbaetz) 2002-01-20 18:47:16 PST
OK, then I think I do misunderstand ;)

That only protects against other _uncommited_ transactions, AIUI.

Consider the following, which is how editattachstatuses.cgi adds new attachments.

table test2 has two columns, id, and test_id, with a unique index on id.

user1                                user2
---------------------------------------------------------
bbaetz=> begin;                      bbaetz=> begin;
BEGIN                                BEGIN
bbaetz=> select max(id) from test2;
 max 
-----
   4
(1 row)
                                     bbaetz=> select max(id) from test2;
                                      max 
                                     -----
                                        4
                                    (1 row)
bbaetz=>insert into test2 values(5,5);
INSERT 18776 1
                                     bbaetz=> insert into test2 values(5,7);
                                     (blocks....)
bbaetz=>commit;
COMMIT
                                     ERROR: Cannot insert a duplicate key into
                                     unique index test2_id_index

Oops. So the index is still unique, but the second attachmentstatus addition
failed. Now our code has to know how to rollback, and start again. Yuck.

What the code really needs to do is to lock the table in SHARE ROW EXCLUSIVE
mode first. The current code does LOCK TABLES attachstatusdefs WRITE, before this.

Mysql only has read and write locks, which translate directly to SHARE and
ACCESS EXCLUSIVE modes, I think. ACCESS EXCLUSIVE is very heavy weight - it even
blocks other SELECTS. Here we can get away with something simpler, since we
don't mind other people trying to select on the table for display. (of course,
if they want a read lock, they'd have to block) Locks only matter within
transactions, too, and postgresql doesn't have an UNLOCK command, so we'd have
to have psuedo support for those, at least.

What am I missing? (If you want to discuss this on IRC< that would probably be
easier...)
Comment 20 David Lawrence [:dkl] 2002-01-20 19:00:00 PST
That is one reason why I have found ever doing SELECT MAX(id)+1 for getting a
next primary key is not really a good idea. That is where sequences are helpful.
This is a paragraph from the PgSQL docs that explains alittle.

To avoid blocking of concurrent transactions that obtain numbers from the same
sequence, a nextval operation is never rolled back; that is, once a value has
been fetched it is considered used, even if the transaction that did the nextval
later aborts. This means that aborted transactions may leave unused "holes" in
the sequence of assigned values. setval operations are never rolled back, either.

So if you were using sequences which I am, you would also need to LOCK the
sequence table itself to make sure that noone can get a number until you are
done if the above method wasnt implemented.
Comment 21 Bradley Baetz (:bbaetz) 2002-01-20 19:39:18 PST
Sure. But:

a) The code does this already, so we have to support it (and the frequency of
doing an INSERT for a new bug/etc is reasonably low that locking the sequence
table/using a trigger for MAX(id)+1/etc wouldn't introduce noticable contention).

A sequence won't work for tokens, once we move to making them non-unique.

b) That was the simple case, but consider adding a vote to a bug. You haven't
change doeditvotes.cgi, but it needs to be locked. Currently it deletes all
votes, then adds the existing ones, counting + updating the cache on the bugs
record. You don't want that happening concurrently... Even if it was using
RelationSet, you'd still have problems if the same user modified their votes
simultaniously.

(OTOH, looking through a code I see a lack of locking in places - eg I think its
possible for the <title> of a bug to be different to its summary in the
textfield, since the SELECT is called once from show_bug, and again from
bug_form, while process_bug can happen in the middle. Using pgsql doesn't change
that, but if it was fixed, SHARE locking would be needed on the bugs table.

I agree that pgsql can get rid of a large number of the locks, but not all of
them...
Comment 22 Matthew Tuck [:CodeMachine] 2002-01-20 23:41:14 PST
I think PostgreSQL will fail transactions that clash in that way, so they need
to be reapplied.
Comment 23 Bradley Baetz (:bbaetz) 2002-01-20 23:50:20 PST
> I think PostgreSQL will fail transactions that clash in that way, so they need
> to be reapplied.

Unless you take care in locking, which is what we'd want to do, rather than
using serilized transactions and handling retrying ourselves.
Comment 24 Matthew Tuck [:CodeMachine] 2002-01-21 01:48:42 PST
Suggest moving transaction discussion to newly filed bug #121069 - transactions
are not necessary to support PostgreSQL AFAIK.
Comment 25 Matthew Tuck [:CodeMachine] 2002-01-29 23:47:25 PST
I suggest the first version of the PgSQL code should use locking in the same way
as MySQL does.

This would simplify the initial landing to what is necessary to get it working.

We can definitely use MVCC later but MVCC won't work unless you're using
transactions.
Comment 26 David Lawrence [:dkl] 2002-01-30 09:14:47 PST
From the PostgreSQL documentation, table locking is only effective when 
utilizing transactions which we do not do yet. Also most of the necessary row locking
is done automatically for you by PostgreSQL for most common operations such
as SELECT, UPDATE, INSERT, and DELETE. Please refer to the following page
and correct me if I am off base.

http://www3.us.postgresql.org/users-lounge/docs/7.1/reference/sql-lock.html
Comment 27 Matthew Tuck [:CodeMachine] 2002-01-30 20:19:03 PST
bbaetz mentioned this on IRC and mooted the idea that for a first pass we could
replace locks and unlocks with equivalent subs that did an implicit transaction
for PgSQL.  This would be reasonably easy.

Maybe we will introduce proper transactions for 2.18 but it would be nice if it
was a separate patch.

The full transactionising task is probably a lot harder, we need to audit for
places where there is insufficient locking currently.  We know some of these
places exist.
Comment 28 Bradley Baetz (:bbaetz) 2002-01-30 22:46:24 PST
And, again, I want to try it out, _then_ work on an API. When I get time, I'll
sit down with some paper, a pen, and the bugzilla source, and work out what
locks we need (as opposed to what we currently have)
Comment 29 Joel Peshkin 2002-08-27 09:14:17 PDT
Note to pgSQL implementers...
When running a query that relies on the ordering of NULL values in SELECTS
versus non-NULL values, MySQL and PgSQL follow opposite conventions. 
MySQL 3.23 puts NULL first if ascending, last if descending
MySQL 4.0 documentation claims NULL is first regardless
pgSQL documentation claims NULL is last if ascending and first if descending.

Since doing JOINS with ISNULL is vital to get decent performance out of all of
the maps Bugzilla is getting, it is a good idea to make a provision for
portability on this.  ORDER BY statements that are relying on this behavior
should be able to do somthing like "ORDER BY fieldname $::NullFirst" where
NullFirst would be defined according to the database in use.

Comment 30 David Lawrence [:dkl] 2002-09-23 15:10:07 PDT
Created attachment 100290 [details]
PostgreSQL configuration utility similar to checksetup.pl v(1)
Comment 31 David Lawrence [:dkl] 2002-09-23 15:14:46 PDT
Created attachment 100291 [details] [diff] [review]
Port CVS Head to PostgreSQL (v1)

This is initial first pass at making the new cvs head with integrated group
schema changed to work with PostgreSQL. Still needs work:

1. regular expression searches in Search.pm
2. buglist.cgi has permission checking turned off to allow it to work. Probable

cause of error is different JOIN syntax with Pg.
3. Attachments are large text fields for now. Need to be converted to support
Pg's large binary object interface.
4. Some substring searches are probably borken too in Search.pm but have not
tried it yet.

Most of this stuff just needs for me to bring over my changed from the previous
PostgreSQL port which had most of these working.
Comment 32 Bradley Baetz (:bbaetz) 2002-09-23 16:00:52 PDT
Comment on attachment 100291 [details] [diff] [review]
Port CVS Head to PostgreSQL (v1)

This diff has a whole lot of other changes - I suspect it wasn't diffed against
the correct base tag.

A lot of the conditionaly stuff shouldn't be there, since its valid for mysql
too (eg the 'delete from logincookes' stuff)

What mode does |LOCK TABLE| w/o an expicit mode use?

Can't you handle the LOCK stuff in SendSQL, like you do UNLOCK? Also, unless
you lock the tables in the same order (eg alphabetical), Pg will end up in a
deadlock state eventually.
Comment 33 David Lawrence [:dkl] 2002-09-25 17:14:52 PDT
Created attachment 100656 [details] [diff] [review]
Port CVS Head to PostgreSQL (v2)

Second attempt. Made changes based on bbaetz suggestions. 

Transactions seem to work properly now the AutoCommit set to off.
No if/else for LOCK TABLES. Handled now in SendSQL.
Added support functions for SqlRegEx() and CurrId().
Changed occurences of INSTR() to POSITION() which both Mysql and PgSQL seem to
like.
Comment 34 Bradley Baetz (:bbaetz) 2002-09-25 17:40:05 PDT
Your REPLACE changes are subject to races - for pg, you need to LOCK the table
in share row exclusive (I think) mode, then SELECT FOR UPDATE the row.

You need the lock in case the entry doesn't exist, and the FOR UPDATE in case it
does.

HandleError shoud really be a die, but I've handled that separately. Implicty
rolling back won't help, because then you'd have subseqeunt statments eecuting.

Please do not use an END sub - thast just going to stuff mod_perl up. What you
should do instead is to have the LOCK commands change autocommit, then call
->begin, and have unlock do ->commit. YEs, this isn't safe, but its the best
you're going to get for the moment.
Comment 35 Bradley Baetz (:bbaetz) 2002-09-25 18:31:47 PDT
Actually, after a bit of google seraching, the best replacment for REPLACE in
this case is:

BEGIN
LOCK TABLE foo IN SHARE ROW EXCLUSIVE MODE
DELETE FROM foo where (unique_key=val);
INSERT INTO foo (unique_key,a,b) values (unique_key,new_a_val,new_b_val);
COMMIT

This allows concurrent processes to access the old value until this is committed
(inlcuding other select for updates on other rows), but blocks any updates (to
any row). Given the shortness/non-perf-ciritcal nature of this, I don't think
its a problem. (It also means that we don't need to SELECT first, and then test
the result)

For mysql, just use a write lock on the table.

I do note that query.cgi's use of replace looks racy, because there aren't any
locks on the namedquery table...
Comment 36 Myk Melez [:myk] [@mykmelez] 2002-10-07 14:16:37 PDT
Correcting summary to make bug easier to find.
Comment 37 Bill McGonigle (not currently reading bugmail; please contact directly) 2002-10-07 14:58:11 PDT
PostgresSQL -> PostgreSQL

If you're going to be pendantic... :) 

'postgres' should still hit as a case-insensitive substring.
Comment 38 David Lawrence [:dkl] 2002-10-09 15:08:01 PDT
Created attachment 102377 [details] [diff] [review]
Port CVS Head to PostgreSQL (v3)

Various bug fixes. 
Better locking/transaction support for PostgreSQL.
Much testing needs to be done.
Comment 39 Bradley Baetz (:bbaetz) 2002-10-09 17:03:39 PDT
I really really really think that you're going to want to use checksetup. With
other db support coming along, having to update the schema in 5 different places
is just going to be a nightmare.

Also, you probably want to create tables WITHOUT OIDS, to save space.
Comment 40 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-11-07 20:38:46 PST
Created attachment 105532 [details] [diff] [review]
Port CVS Head to PostgreSQL (v4)

I felt the need to sync this patch because I'm basing the Sybase conversion on
it.

globals.pl had a conflict with the E|A|R patch that I had a hard time figuring
out (postgres removed a column from a query and added another one, and E|A|R
added the same new column, but didn't remove the other one)

I wiped out reports.cgi from this patch.  It got *completely* rewritten
recently, so we're better off starting over with that than trying to resolve
the 450-line conflict :-)

pgsetup.pl is groussly out of date.  We've had a lot of schema changes in the
last month.

I *will* be doing some more work on this in the next few days, since I have a
deadline to meet with the Sybase conversion.

I have postgresql 7.2.2 installed on Landfill, and an installation running this
patch there also.  I'll publish the URL as soon as I get pgsetup.pl up-to-date
and run it. :-)
Comment 41 David Lawrence [:dkl] 2002-11-22 17:49:31 PST
Created attachment 107199 [details] [diff] [review]
105532: Port CVS Head to PostgreSQL (v5)

Updated patch with PostgreSQL changes ported to current CVS. WIll post new
patch soon with merged checksetup.pl that I worked on while on my road tour ;)
Comment 42 David Lawrence [:dkl] 2002-11-22 17:49:56 PST
Created attachment 107200 [details] [diff] [review]
Port CVS Head to PostgreSQL (v5)

Updated patch with PostgreSQL changes ported to current CVS. WIll post new
patch soon with merged checksetup.pl that I worked on while on my road tour ;)
Comment 43 Bradley Baetz (:bbaetz) 2003-01-24 04:11:44 PST
Comment on attachment 107200 [details] [diff] [review]
Port CVS Head to PostgreSQL (v5)

This has bitrotted. A few quick comments:

Some of the mysql vs pg differences don't need to be there. Date arithmatic can
be mostly done wthe same way with mysql 3.23, I think. (ie foo + interval '1
day'). However, pg requires quotes arround the '1 day', and mysql doesn't allow
it. Sigh. Anyway, we can abstract that, which is probably better/easier/etc.

The CurrId stuff should take two arguments, rather than a : separate string
which is split.

Would it be worth installing sql functions for the mysql IF function?
Similarlly for IFNULL.

For attachment.cgi, use a recent DBD::Pg version, and bind the var as a blob.
See dbi-dev/pg-interfaces disucussions froma couple of months back for details.

Avoid extra indentation - it just makes stuff hard to follow.

Since you have presumably found all of the places delta_ts needs to be updates
(because pg doesn't have an auto-updating timestamp type), its probably worht
splitting that out, making it a 'normal' time val on the mysql side, and then
you can drop the date format stuff.

Can the security stuff be rewtitten to use EXISTS for pg, and thgus be faster?
Comment 44 Damien Miller 2003-05-16 20:23:48 PDT
I'd like to try out and help debug PgSQL support. Given that the most recent
patch has bitrot, where can I obtain the latest code? Is there a branch in CVS?
Comment 45 Antonis Christofides 2003-06-09 10:44:26 PDT
The attachment has code like this in several places:

  if ($::db_driver eq 'mysql') {
    do it the mysql way
  } elsif ($::db_driver eq 'Pg') {
    do it the pg way
  }

And, of course, my imagination extends it like this:

  if ($::db_driver eq 'mysql') {
    do it the mysql way
  } elsif ($::db_driver eq 'Pg') {
    do it the pg way
  } elsif ($::db_driver eq 'oracle') {
    do it the oracle way
  } elsif ($::db_driver eq 'informix') {
    do it the informix way
  } elsif ($::db_driver eq 'sybase') {
    do it the sybase way
  } elsif ($::db_driver eq 'ehm... msaccess' ) {
    whatever
  }

All this in numerous places around numerous files. Do we really want it like
this? How about creating an abstract database interface? (Don't hit me!)
Comment 46 Joel Peshkin 2003-06-09 10:59:00 PDT
WRT comment 45,

  The lowest common denominator between all those DBs is really low.  Hopefully,
*MOST* places where DB operations are done can be common to all the DBs.

Comment 47 Taral 2003-06-09 12:16:22 PDT
I think comment 45 was asking if we could consolidate all of the
database-specific stuff into one file for better tracking...
Comment 48 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-06-09 12:28:06 PDT
Yeah, we did a Bugzilla/DBCompat.pm for the Sybase port that contains stuff like:

sub SQLNow {
    if ($::db_driver eq 'mysql') {
        return 'NOW()';
    }
    elsif ($::db_driver eq 'Sybase') {
        return 'GETDATE()';
    }
}

So then when we need to get the current time, we can do this:

SendSQL("SELECT @{[SQLNow()]}");

(just a simple example, there's worse ones, like date formatting and date math
and so forth).

It's on my long list of things to get contributed one of these days.
Comment 49 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-06-12 17:26:50 PDT
Just FYI, I just posted the DBCompat.pm to bug 173130
Comment 50 Antonis Christofides 2003-07-05 03:33:45 PDT
With comment 45 I was actually suggesting what I just now filed as bug 211769,
but there are also less extreme solutions, such as bug 131136 or Dave's
DBCompat.pm file.
Comment 51 Jeroen Ruigrok van der Werven 2003-08-28 14:45:10 PDT
Created attachment 130562 [details]
Schema for Bugzilla

This is a schema based on CVS Bugzilla's database creation as per
checksetup.pl.	I think the way to go would be to rip all database specifics
from the checksetup.pl and make files like: schema.pgsql, schema.mysql,
schema.oracle, etc.  I have some work on that which I hope to post soon.
Comment 52 Jeroen Ruigrok van der Werven 2003-09-01 15:02:28 PDT
Created attachment 130751 [details]
Compatibility layer for PostgreSQL

Based on Dave Miller's DBCompat.pm for Sybase/MySQL I added PostgreSQL code to
the file.
Comment 53 Jeroen Ruigrok van der Werven 2003-09-04 13:50:04 PDT
Created attachment 130925 [details]
Updated and partially rewritten pgsetup.pl file

This is an updated pgsetup.pl file.  It creates the database, sets things up. 
All without problems and using a schema which benefits pgsql.
Comment 54 Jeroen Ruigrok van der Werven 2003-09-05 15:22:14 PDT
Created attachment 130979 [details]
PostgreSQL compatibility layer, revision 1

This new compatibility layer replaces the previous one I submitted.  It has
stubs for Oracle, introduces a DBConnect function/method (which needs to be
fixed wrt db_user and db_pass, I can get them from Bugzilla::Config, but it is
not picking them up, help appreciated), and some more fleshed out skeleton
documentation.
Comment 55 Bradley Baetz (:bbaetz) 2003-09-05 18:01:10 PDT
Comment on attachment 130979 [details]
PostgreSQL compatibility layer, revision 1

People should use the Bugzilla::DB connect method instead.
Comment 56 Ralf Hauser 2003-09-26 00:29:37 PDT
How does this effort relate to the
http://bugzilla.redhat.com/download/rh-bugzilla-pg-LATEST.tar.gz that can be
found on https://bugzilla.redhat.com/bugzilla/index.cgi?
Comment 57 Jeroen Ruigrok van der Werven 2003-09-28 02:47:50 PDT
The Red Hat guys did a quick 'n dirty port.  It works, but doesn't quite make 
use of the best of PostgreSQL.  Also, their tarball is out of date with the 
current schema used by Bugzilla.
Comment 58 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-09 05:38:00 PST
dkl:  in two weeks it will have been one year since the last time you touched
this bug.  Is there any chance at all of you being able to work on this again to
get it landed, or should we allow someone else to take it over and drive it in?
Comment 59 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-09 05:53:25 PST
Going with bbaetz's comment 39:

One thing that really needs doing is a unified way of doing checksetup.pl so
that it works with all the databases.  having a separate schema file for each
database is going to get out of sync fast, and people who add a feature that
requires schema changes are going to forget to update the schema files other
than the one they're using.  It would be best if we had some generic way to
describe the schema that all of them used, but could easily be adjusted to suit
the individial databases.

A Perl hash which describes the schema would probably work well for this.  The
database could be a single object which contains an array of table objects,
which contains an array of column objects, index objects, references to other
tables, etc.  We should have our own definitions for table types that are a
superset of everything available on the different databases, and let the actual
setup code pick the best type for that database based on that.  Using specific
hash keys also allows database-specific keys to be added where needed without
interfering with things the setup scripts for other databases expect to find,
but keeping it all in the same place.
Comment 60 Andrew Dunstan 2003-11-10 13:01:24 PST
I've start something along these lines today, which I think could work well,
except for the enumerated fields. It is quite possible to constrain these values
within a text field, but this will lead to some nasty bloatage. If I were doing
this in PostgreSQL (or almost anything but MySQL) from scratch I would put these
values in a table (or one per enumeration) with a code for each value and then
use a foreign key constraint on the code in the referencing table, possibly
adding functions to encode/decode values. But you can only do FKs with MySQL
3.23.44 and up and then only using InnoDB, and you can only define functions in
C, as I understand it. 

I'm sure there are many other wrinkles (e.g. fulltext) but that's what I came
across first up.

So although a super-schema sounds nice in theory, ISTM in practice that it might
not work so well, and a better way would be to have a schema per RDBMS
supported, and functions to go along with it that hide the actual implementation
from most of the code.
Comment 61 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-10 13:34:13 PST
The problem with multiple schema documents is who is going to maintain them? 
Anyone maintaining a database schema will have to monitor all changes to any of
the other schemas and make changes to match.  With the generic scheme it's
likely someone with knowledge of only one database could add to that schema (if
we have limitations such as "no enums" spelled out) and have a reasonable
certainty that it would still work on the other databases.
Comment 62 Antonis Christofides 2003-11-10 13:54:40 PST
I mostly agree with comment 60, and I've made some additional comments
supporting this in my disillusioned bug 211769 comment 1. A lowest common
denominator schema may be possible, however, although it might be better to use
lowest common denominator CREATE TABLE statements instead of Perl data structures.
Comment 63 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-10 14:09:11 PST
There's no such thing as a lowest-common-denominator TABLE CREATE statement,
especially because of data types.  Although a lot of databases claim to be
"ANSI-compatible" a lot of them (like Sybase) suck at it in practice.  Sure, the
table create works, but you wind up with crappy performance.  It would be better
for performance reasons to declare our own datatypes for each column which
explicitly defines the type of use we have for that column, and let each
database driver translate that to the most effective datatype for that database.

There may be cases where 3 different columns might have the same datatype in the
MySQL schema, but the most effective schema in some other database might use
different types for each of the three.

For example, there are a number of tables that have what are essentially boolean
columns in them (attachments.ispatch, groups.isbuggroup, bugs.cc_accessible,
etc).  MySQL doesn't have booleans, so they're TINYINT in our schema.  Postgres
and Sybase both have boolean types, and in most cases it would make better sense
to actually use them.

Other differences (like how many digits versus how many bytes for integers, and
how many characters vs how many bytes in unicode fields, etc) could come into
play, too.
Comment 64 Andrew Dunstan 2003-11-10 14:27:56 PST
I expect to have something along the lines of Dave's suggestion in a day or two.

If we are fine saying "no enums" etc. then we might get somewhere. Can we also
say MySQL >= 3.23.44 and InnoDb?

In any case it will mean major surgery, but it it the Right Thing (tm).
Comment 65 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-10 14:37:27 PST
Databases like the older versions of MySQL could even ignore the foreign key
references, just like we do now.  And if the schema is in a perl module file
which can be loaded by other scripts than just checksetup.pl, then sanitycheck
can use those reference definitions to test for it on the databases that don't
enforce it themselves.
Comment 66 David D. Kilzer (ddk) 2003-11-10 21:03:02 PST
Before anyone goes off to write their own generic database schema module,
please take a look at DBIx::DBSchema.  It currently supports MySQL,
PostgreSQL and (partially) Sybase.  It's being actively updated, and I'm
sure the author would welcome patches.

http://search.cpan.org/~ivan/DBIx-DBSchema-0.22/

As far as making database connections more transparent, the DBIx::AnyDBD
module may help with that.

http://search.cpan.org/~msergeant/DBIx-AnyDBD-2.01/
Comment 67 Jeroen Ruigrok van der Werven 2003-11-10 23:02:02 PST
Having spent a few weeks hacking on the PostgreSQL support before I got pulled 
into migrating the office to a new building over the course of a month I can 
tell you that not only the schema is important to fix.  The entire code as it is 
is good for MySQL, but pretty braindead if you want to use a real RDBMS.  The 
way MySQL locks tables is unacceptable for (most) other databases.  So real 
transactions need to be introduced in order to solve these things.  Doing that 
also requires a lot of rewrites on the source level.  I am not sure this is 2.18 
material.  Also, a lot of the backwards compatibility cruft isn't helping 
rewriting the code.  Yes, I understand the importance, but man, it is a LOT of 
additional stuff you have to worry about.
Comment 68 Bill McGonigle (not currently reading bugmail; please contact directly) 2003-11-11 14:57:07 PST
Usually when you have to rip everything out and rewrite it, the project just
never gets done.  That might be where this bug is.  "baby-steps" is my motto.

Would it be possible to divide this project up in to smaller bits and work away
at it over a few releases?  For instance, get DBIx::DBSchema and
DBIx-AnyDBD-2.01 supported in spirit for optional use.  Then when somebody
touched a bit of code they could opt to use DBIx::DBSchema instead of writing
mysql-compatible code directly.  Helpers could port small bits as they have
time.  For a while, you'd have the overhead of the abstraction to still only
support one databse, but eventually enough would get done that one final slog
could pick up the leftover bits and complete the transition.

Even though the bug is for pgsql support, database independence is the nicer
goal.  Comments show some conflicts though: independence (mysql,pgsql,sybase),
ease of coding, and speed (write 1 SQL statement vs n optimized SQL statements).
 I think it's a 'pick any two' situation.  It would be helpful to pick the two
before embarking on the solution.

If all the SQL were sql-standard, that would be a good start.  There are
probably several queries that are not taking advantage of any mysql tricks that
could be brought up to standard which would help get the ball rolling.  There's
a web validator here:  http://developer.mimer.com/validator/  Maybe helpers
could pick off some of those queries to chip away at the problem?

Of course, to even do that, we'd have to pick the standard first (92?), which is
a balancing of trade-offs.  [aside: The only reason vendors get away with shoddy
standards support is because the customers let them...]   Mozilla.org stuff
_tends_ towards supporting standards on principle.  I'd be happy to file some
breakout bugs if any of this sounds reasonable.
Comment 69 Andrew Dunstan 2003-11-11 16:32:14 PST
Some things can be done in baby steps, some can't - they just require big jumps.

I tried getting DBIx::DBSchema working, but it wanted a version of DBD::Pg that
isn't even on CPAN, so I gave up. 

I now have a sort of abstract database structure and some database dependent
stuff that will now generate DDL for both MySQL and PostgreSQL and is easily
extensible to other Dbs. It has the whole Bugzilla schema down. It even has
rules for emulating MySQL's hokey auto-update-timestamp thingy in PostgreSQL.
The one MySQL thing that is gone is the enums in the bugs table - replaced by FK
references to tables holding the allowed values. It still has some testing to go
but appears to work. The aim is to have something that doesn't bitrot when the
schema changes.

However - and this is really where the non-baby steps come in - what I think is
really needed is to provide a layer between the cgi code and the database, so
that the cgi code did no SQL construction and the database layer does no
presentation. It just becomes procedural - make a call, get back a hash. I've
done quite a few Perl webapps (in the commercial world) using this architecture,
and ported them with minimal fuss between MySQL, PostgreSQL and Oracle. *BUT* it
is a big job, and before I can get started on it I need to know the code far
more intimately.

I would like very much to get this done for 2.18, but I have no idea of the
timeframe involved. If anyone can enlighten me a bit on that it would help me to
prioritize (WAGs OK). 
Comment 70 Michael Evans 2003-11-11 21:47:03 PST
Excuse an amateur, but could the "layer between the cgi code and the database" 
referred to in #69 be ODBC?

* Almost all databases have ODBC support
* there is a DBI interface
    http://search.cpan.org/~jurl/DBD-ODBC-1.06/ODBC.pm
* there is plenty of experience in using ODBC on th eWin platform
* there are free ODBC drivers for non-Windows platforms, for example unixODBC

/Mike (don't reinvent the wheel)
Comment 71 Joel Peshkin 2003-11-11 22:19:47 PST
The key issue is not how one communicates the SQL itself to the server or even
the exact syntax of the table creation commnads.

The key issue is that the various databases have fundamental differrences in
their approach.  MySQL (MyISAM) require table locking.  PgSQL and Oracle support
transactions (as does MySQL InnoDB).  Some databases support boolean operations
while others do not.  The behavior of NULL values varies by database. There are
differrences in the degree of support for foreign keys and enumerated datatypes.
 Some DBs handle LEFT JOINs very efficiently while others demand that they be
avoided.  Some DBs have POSIX regular expression support while others lack it.

All of those issues currently have driven design decisions that are captured in
the very way that Bugzilla is structured.  Moving to a DB-agnostic structure
will require some rather dramatic changes.  

My suggestion is we start by evaluating....
a) Moving recommended MySQL version to 4.something
b) Migrating from MyISAM to InnoDB table types
c) Replacement of MySQL-specific SQL with more generic SQL
d) Freeze of checksetup.pl (upgrade to final MySQL-only version)
e) Creation of new checksetup.pl replacement that abstracts multiple databases.
(No need to port old conversion code.... existing installations can be presumed
to have MySQL, so they can be upgraded to final state of checksetup.pl.  Only
the new checksetup.pl replacement needs to be maintained beyond that point.)
Comment 72 Andrew Dunstan 2003-11-13 12:09:36 PST
Created attachment 135422 [details]
Proof of Concept code for DB independant schema generation

This is some proof of concept code that aims at the 'single schema - multiple
database engines' idea that Dave proposed.

I haven't worked on integrating it yet with the bugzilla code, and
unfortunately a large project has just landed on my plate, so it might be a
while before I can get back to it.
Comment 73 Max Kanat-Alexander 2003-12-18 18:46:39 PST
That proof-of-concept code looks nice. However, arrays might not be the way to go.

I'm almost sure that the way to go is something like DBCompat.pm, overriding it.
I'd like to see Bugzilla::DB::Pg, Bugzilla::DB::MySQL, Bugzilla::DB::Sybase,
etc. (In fact, I thought this _was_ the way we were going -- I thought I talked
about this with somebody already, who expressed this.)

Impractical Theoretical Solution:
In an ideal world, I'd have every bug as an object, and have every object load
itself into memory with all of its fields initialized by reading the DB, and
have it write itself back out to the database when needed. I'd abstract out
every DB read/write to getters and setters. :-) Basically, under most
circumstances, I'd have to have an object in memory for every bug in a database.
(The database just becomes a persistence layer.) However, I've implemented
solutions like this before, and the memory usage is larger than most Bugzilla
administrators would be likely to accept.

[Note: There are other ways to implement this that _would_ be practical in
Bugzilla, using memcached and lazy reads.]

Though that's impractical, there is something to be gained from it. It
demonstrates that the root of the problem lies in abstraction of database reads
and writes. (I think this is what justdave was getting at.)

So, the general, ideal, practical solution is just more abstraction, one way or
another.

I think a good first step would be to have checksetup call functions in
DBCompat.pm, just like the rest of Bugzilla should.

Possibly, it would mean a LOT of functions in DBCompat.pm. I think that's okay.

DB-independent schema are are really cool idea, and they're a lot of fun to
think about.

By the way, another posibility would be to take that proof-of-concept code and
make it more modular, using objects.

I'm very willing to work on this.

-M
Comment 74 Andrew Dunstan 2003-12-19 11:24:54 PST
Regarding use of arrays: it was done deliberately where order could be important
- namely tables (for FK dependencies) and fields (to preserve field order).

Regardless of how OO it is made, ITSM it will only be maintainable if a Db
independent schema is kept in *one* place. Otherwise it will be far to easy to
forget to update some Db's schema and break things. That's what I tried to do.
Wrapping some OO-ness around that would be hard.
Comment 75 Bradley Baetz (:bbaetz) 2003-12-19 17:02:58 PST
We need to name all of our indexes,so that we can drop them in the schema change
code.

How will teh schema change stuff happen independantly?
Comment 76 David D. Kilzer (ddk) 2003-12-19 19:45:45 PST
>How will the schema change stuff happen independently?

I think DBIx::AnyDB could handle that.  You'd have to write methods like
addColumn("table", "colname", "coltype"), deleteColumn("table", "colname"),
etc. for each supported database, then checksetup.pl (or its replacment)
could then call the "generic" method, which would then call the appropriate
database-specific method depending on the current Bugzilla configuration
(which would determine which database is being used).

See also Comment #66.
Comment 77 Max Kanat-Alexander 2003-12-29 14:58:30 PST
By the way, I'm not sure that DBIx::AnyDBD is really what we're interested in,
anyhow. That's what DBI is for.

I think that DBIx::DBSchema and DBIx::Abstract are what's interesting.

-M
Comment 78 Andrew Dunstan 2003-12-30 06:51:33 PST
Be careful about loading up more module dependencies - I had troubler getting
DBIx::DBSchema installed. Your (Max K-A's) earlier suggestion of taking my POC
code and OOiffying it might be a better way to go.
Comment 79 David D. Kilzer (ddk) 2004-01-26 11:38:07 PST
Regarding Comment #58:

I noticed that Red Hat has a new download on their Bugzilla index.cgi
page (dated 2003-11-20) for a version of Bugzilla that uses PostgreSQL.
I have no idea if this is DKL's work, or someone else's.

  https://bugzilla.redhat.com/bugzilla/index.cgi

Here's the direct download link:

  http://bugzilla.redhat.com/download/bugzilla-redhat-20031120.tar.gz
Comment 80 josh 2004-01-26 14:10:43 PST
According to Dave Lawrence at RedHat, the version that you see on their website
is based on version 2.17.1:

It is based on 2.17.1 which is available at http://www.bugzilla.org.

On Mon, 2004-01-26 at 15:34 -0500, Joshua Kramer wrote:

>> Hello,
>> 
>> I see you have a PostgreSQL version of Bugzilla at
>> https://bugzilla.redhat.com/bugzilla/index.cgi.  What version of Bugzilla
>> is this based on - does it have all of the features and fixes of 2.16?
>> 
>> Thanks,
>> --Josh
>> 
Comment 81 Max Kanat-Alexander 2004-01-27 21:48:24 PST
Okay, so there are two actual bugs here -- one for schema, and one for the SQL
that Bugzilla uses. Since all the docs for PG integration point to this bug,
let's make this the SQL bug. I'll file another bug for DB-independent schema,
and move the important attachments and CC's there. When it's all made, I'll post
the bug number here.

Also, dkl hasn't worked on this in a while, I'll assign it to nobody for now.

-M
Comment 82 Max Kanat-Alexander 2004-01-27 22:19:25 PST
All right, it turns out that bug 146679 was close enough to what we were talking
about that our ideas could be merged over there. DB Schema conversation goes
there. For this bug, let's focus on the actual SQL that Bugzilla sends, outside
of checksetup.pl.

What I suggest is to use this as a tracking bug. Find little points within
Bugzilla where you could make the SQL DB-independent, and then file a bug
against that part, blocking this bug.

Once that is complete, we can file another bug for "use DBCompat.pm", which I
think is what we should do for the differences between databases that can't be
resolved.

This means that certain bugs actually now block the new bug, not this one, so
those blockers will be moved there, to bug 146679, and that bug will be marked
as blocking this one.

-Max
Comment 83 David Lawrence [:dkl] 2004-02-16 15:56:46 PST
Created attachment 141569 [details] [diff] [review]
Port CVS Head to PostgreSQL (v6)

Another stab at creating a better patch against CVS head. I just noticed the
compat layer created earlier by another person. I have also create a
DBcompat.pm module which I will look at merging the better of the two into one
and upload a new patch soon. Couple of things to note, FlagType matches are
commented out in Bug.pm until I figure out what is wrong. Also I have included
a pgsetup.sql script for database creation until I or someone else can create a
unified checksetup.pl in order to get rid of pgsetup.pl. Please let me know
what you all think about the latest patch.
Comment 84 Max Kanat-Alexander 2004-02-16 19:02:09 PST
Comment on attachment 141569 [details] [diff] [review]
Port CVS Head to PostgreSQL (v6)

Hrm, this is a big hulking patch. I'll look over DBCompat first.

My first note is that I'm *really* happy about the subroutine docs. :-)

>+# Contributor(s): Terry Weissman <terry@mozilla.org>

   Hrm... aren't *you* actually the contributor for this one? :-)

>+sub GetLastInsertID {
>+    my ($table, $column) = @_;
>+    if ($db_driver eq 'mysql') {
>+        return "SELECT LAST_INSERT_ID()";

     Is this actually guaranteed to do what the documentation above says? If I
insert into table A, and then insert into table B, and I call this on table A,
I get...?

     You might also want to note in the subroutine doc that this should always
be called inside of a transaction, so there aren't threading issues. (Thread1:
INSERT Thread2: INSERT Thread1: GetLastInsertID())

>+# subroutine:  RegExp
>+# description: Outputs SQL syntax for doing regular expressions searches in format
>+#              suitable for a given database.
>[snip]

    An example of how this works would be useful to me, and probably to others.
I don't fully understand how that function is used, looking at the code of it.
(Is it actually complete?)

>+sub DateFormat {
>[snip]

   I like this because it allows us to keep the MySQL syntax we have there
already.

>+sub ToDays {
>+    my ($column) = @_;
>+    if ($db_driver eq 'mysql') {
>+        return " TO_DAYS($column) ";
>+    } elsif ($db_driver eq 'Pg') {
>+        return " $column ";
>+    }
>+}

     For Pg, I think that should actually be to_char($column, 'J')::int, based
on some code I've fixed in our local Pg Bugzilla. That returns the
date/timestamp as # of Julian days, which can be added/subtracted from other
numbers of the same type.

>+sub Rand {
>+    return " RAND() " if $db_driver eq 'mysql';
>+    return " RANDOM() " if $db_driver eq 'Pg';

Nit: The Pg idiom seems to be lowercase function names. (At least in the docs.)

-M
Comment 85 Max Kanat-Alexander 2004-02-16 19:25:02 PST
I see that have some dummy code in DB.pm, too, for handling LOCK TABLES and
UNLOCK TABLES. Is there a reason that you added the "return;"?

I think a better solution than this would be to add a Lock($table, $row) and
Unlock($table, $row) function to DBCompat.pm, or even to DB.pm. Then, change all
the LOCK TABLES and UNLOCK TABLES statements to that (which would be a simple
grep-then-edit). That way, for MySQL you could ignore the $table and $row, and
for PgSQL you could get more granular locks.
Comment 86 Max Kanat-Alexander 2004-02-16 20:21:03 PST
I hope you can forgive the spam. There's just no way to digest that whole patch
at once. :-)

Further comments:

+ You have a few if(db_driver) statements that do two whole chunks of SQL for
only a difference between (e.g.) "INTERVAL 3 DAY" and "INTERVAL '3 days'". (See
userprefs.cgi for an example.) 
     I think that a TimeInterval($num, $type) function in DBCompat would do the
trick. You could do a switch statement on $type, which would be either "D" or
"days" or something like that, depending on your (dkl) personal preference.

+ There's a lot of DateFormat that goes on in the patch. I would think that we
should do the DateFormat on the presentation side, and let the databases handle
their own date types internally. I think it makes it simpler overall. The
double-comment bug over on the RH BZ
<https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=114353> is example of what
can go wrong.

+ In query.cgi, you changed a REPLACE into an INSERT. If there's a PRIMARY KEY
on that table (I didn't check), then those two won't actually be the same.

+ In process_bug.cgi:

>                detaint_natural($::FORM{"$field"});
>                SendSQL("UPDATE longdescs SET isprivate = $private
> 	
>                        WHERE bug_id = $::FORM{'id'} AND " .
>
> Bugzilla::DB::DateFormat('bug_when','%Y%m%d%H%i%s') . 
>                        " = " . SqlQuote($::FORM{"$field"}));

  You don't need to SqlQuote that field, since detaint_natural assures that it's
an integer. (However, now that I'm thinking about it, why in the world is
bug_when being compared to an integer??)

+ In long_list.cgi, you changed a "IFNULL(bugs.alias, '')" to just "bugs.alias"
-- are you certain that that is safe?

+ In buglist.cgi
   * You manually limited the buglist to 2000 results. Is that a Red
Hat-specific thing, or a PgSQL limitation?
   * Another change of REPLACE to INSERT for namedqueries.
   * I don't understand why "SELECT DISTINCT  groups.id, name, description,
isactive" was changed to "SELECT DISTINCT  groups.id, name, description, isactive"

+ For attachment.cgi, A brief explanation of why attachments need to be coded
into Base64 for PostgreSQL might be useful for those who aren't initiates. (In
fact, I'm not sure, myself.)
Comment 87 Max Kanat-Alexander 2004-02-16 20:27:30 PST
(In reply to comment #86)
>    * You manually limited the buglist to 2000 results. Is that a Red
> Hat-specific thing, or a PgSQL limitation?

  Oh, slight correction. You limited *something* having to do with bug group
privacy to 2000 bug ids.
Comment 88 Jeroen Ruigrok van der Werven 2004-02-16 22:00:11 PST
Hi David,

yeah the code resembles mine so it seems we're at least on the same track.

However, I see you kept the MySQL locking stuff in there.  I am still not 
convinced this is the way to go since it effectively limits the throughput of 
Bugzilla on a PostgreSQL or other transactions-able database.

I'll do a compare on it all and see what we do and do not agree about and then 
we'll hash it out Tekken-style.  (Nah, just kidding. ;) We'll do some discussion 
back and forth.)
Comment 89 David Lawrence [:dkl] 2004-02-18 15:19:57 PST
Created attachment 141703 [details] [diff] [review]
Port CVS Head to PostgreSQL (v7)

Many changes. Will comment further in replies to bug comments.
Comment 90 David Lawrence [:dkl] 2004-02-18 15:25:31 PST

(In reply to comment #88)
> Hi David,
> 
> yeah the code resembles mine so it seems we're at least on the same track.
> 
> However, I see you kept the MySQL locking stuff in there.  I am still not 
> convinced this is the way to go since it effectively limits the throughput of 
> Bugzilla on a PostgreSQL or other transactions-able database.
> 
> I'll do a compare on it all and see what we do and do not agree about and then 
> we'll hash it out Tekken-style.  (Nah, just kidding. ;) We'll do some discussion 
> back and forth.)


In the v7 patch, I merged alot of your stuff in with that I had. I removed the
SQL from the function names since I thought it was redundant and without made
the names shorter and easier to type ;) I can add them back if everyone thinks
we need that level of description. Let me know what you think. As for the
locking, I agree that could remove them, at least PostgreSQL does not need them
per say since it does it's own level of locking. I am not familiar with what is
required with the new MySQL that has transaction support. It may be the same
situation there.
Comment 91 David Lawrence [:dkl] 2004-02-18 15:44:49 PST
(In reply to comment #84)
> (From update of attachment 141569 [details] [diff] [review])
> Hrm, this is a big hulking patch. I'll look over DBCompat first.
> 
> My first note is that I'm *really* happy about the subroutine docs. :-)
> 
> >+# Contributor(s): Terry Weissman <terry@mozilla.org>
> 
>    Hrm... aren't *you* actually the contributor for this one? :-)
> 

Heh, fixed that.

> >+sub GetLastInsertID {
> >+    my ($table, $column) = @_;
> >+    if ($db_driver eq 'mysql') {
> >+        return "SELECT LAST_INSERT_ID()";
> 
>      Is this actually guaranteed to do what the documentation above says? If I
> insert into table A, and then insert into table B, and I call this on table A,
> I get...?
> 
>      You might also want to note in the subroutine doc that this should always
> be called inside of a transaction, so there aren't threading issues. (Thread1:
> INSERT Thread2: INSERT Thread1: GetLastInsertID())
>

It does make the assumption that you are running it after the insert that you
meant to retrieve the current value from. I admit that it is not the most ideal
solution but it is not really that much different than the way it happens now.
 
> >+# subroutine:  RegExp
> >+# description: Outputs SQL syntax for doing regular expressions searches in
format
> >+#              suitable for a given database.
> >[snip]
> 
>     An example of how this works would be useful to me, and probably to others.
> I don't fully understand how that function is used, looking at the code of it.
> (Is it actually complete?)
>

It is complete and I added some more description to the code in comments.
 
> >+sub DateFormat {
> >[snip]
> 
>    I like this because it allows us to keep the MySQL syntax we have there
> already.
> 
> >+sub ToDays {
> >+    my ($column) = @_;
> >+    if ($db_driver eq 'mysql') {
> >+        return " TO_DAYS($column) ";
> >+    } elsif ($db_driver eq 'Pg') {
> >+        return " $column ";
> >+    }
> >+}
> 
>      For Pg, I think that should actually be to_char($column, 'J')::int, based
> on some code I've fixed in our local Pg Bugzilla. That returns the
> date/timestamp as # of Julian days, which can be added/subtracted from other
> numbers of the same type.
>

Fixed
 
> >+sub Rand {
> >+    return " RAND() " if $db_driver eq 'mysql';
> >+    return " RANDOM() " if $db_driver eq 'Pg';
> 
> Nit: The Pg idiom seems to be lowercase function names. (At least in the docs.)
> 

I like to keep column names and values in lower case and SQL keywords in
UPPERCASE. Helps me see things better.

>+ You have a few if(db_driver) statements that do two whole chunks of SQL for
>only a difference between (e.g.) "INTERVAL 3 DAY" and "INTERVAL '3 days'". (See
>userprefs.cgi for an example.) 
>     I think that a TimeInterval($num, $type) function in DBCompat would do the
>trick. You could do a switch statement on $type, which would be either "D" or
>"days" or something like that, depending on your (dkl) personal preference.

I added a function called Interval to DBcompat.pm that does that now.

>+ There's a lot of DateFormat that goes on in the patch. I would think that we
>should do the DateFormat on the presentation side, and let the databases handle
>their own date types internally. I think it makes it simpler overall. The
>double-comment bug over on the RH BZ
><https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=114353> is example of >what
>can go wrong.

I think I see what you are saying. Can you provide some code examples to further
explain this?

>+ In query.cgi, you changed a REPLACE into an INSERT. If there's a PRIMARY KEY
>on that table (I didn't check), then those two won't actually be the same.

Well there isnt a primary key per say in namedqueries but there is a unique
constraint on (userid, name) which may cause a problem possibly. I did this
originally since PostgreSQL did not have a REPLACE statement.

>+ In process_bug.cgi:
>
>                detaint_natural($::FORM{"$field"});
>                SendSQL("UPDATE longdescs SET isprivate = $private
> 	
>                        WHERE bug_id = $::FORM{'id'} AND " .
>
> Bugzilla::DB::DateFormat('bug_when','%Y%m%d%H%i%s') . 
>                        " = " . SqlQuote($::FORM{"$field"}));
>
>  You don't need to SqlQuote that field, since detaint_natural assures that >it's
>an integer. (However, now that I'm thinking about it, why in the world is
>bug_when being compared to an integer??)

Took the SqlQuote off.

>+ In long_list.cgi, you changed a "IFNULL(bugs.alias, '')" to just "bugs.alias"
>-- are you certain that that is safe?

Wrapped it with Bugzilla::DB::IfNull()

>+ In buglist.cgi
>   * You manually limited the buglist to 2000 results. Is that a Red
>Hat-specific thing, or a PgSQL limitation?

At the time I was having problems with putting more that 2000 integers inside of
the IN() clause. It was causing PostgreSQL to generate an error. It may have
something do with a statement length limitation or something else. If the number
of bug_ids was more than 2000 it just didnt do the check and there wasn't any
color highlighting showing private bugs. That may have been an older problem
with PostgreSQL and might be fixed now. I will try it out sometime.

>+ For attachment.cgi, A brief explanation of why attachments need to be coded
>into Base64 for PostgreSQL might be useful for those who aren't initiates. (In
>fact, I'm not sure, myself.)

Was a quick fix at the time to allow binary attachments with little code change
to the main bugzilla code. What needs to be done eventually is to utilize the
built in binary object data types that PostgreSQL uses. I am going to work on
something like that soon.

P.S. Changes to this bug report sure take a long time with the number of Cc
addresses attach to this ;)
Comment 92 David Lawrence [:dkl] 2004-02-18 15:50:04 PST
One additional comment. I added two new functions to DBcompat.pm called
LockTable and UnlockTable. Please comment on those as well. I have only added
the functions into votes.cgi so far so people comment before changing every LOCK
TABLES to the new format and wasting time. You will notice that I send dummy
args in the if condition first before actually sending the real LOCK TABLES
since on certain databases that don't need it, LockTable() will just return an
empty value and the statement will be skipped instead of SendSQL sending
something invalid.
Comment 93 Max Kanat-Alexander 2004-02-18 17:33:02 PST
(In reply to comment #91)
>>+ There's a lot of DateFormat that goes on in the patch. [snip]
> 
> I think I see what you are saying. Can you provide some code examples to further
> explain this?

  Okay, I suppose the basic idea that I'm proposing is that at some point,
DATE_FORMAT() shouldn't happen at all in SQL, and instead should happen in perl. :-)

  There's an example in this patch, though, that I can give, from process_bug.cgi:

-                SendSQL("UPDATE longdescs SET isprivate = $private 
-                    WHERE bug_id = $::FORM{'id'} AND bug_when = " .
$::FORM{"$field"});
+                SendSQL("UPDATE longdescs SET isprivate = $private
+                        WHERE bug_id = $::FORM{'id'} AND " .
+                        Bugzilla::DB::DateFormat('bug_when','%Y%m%d%H%i%s') . 
+                        " = $::FORM{$field}");

  As you can see here, every time we touch bug_when, we have to remember to
DateFormat the value we insert, and DateFormat it when we take it out again,
before the comparison. This is a sort of "hidden requirement" that programmers
have to remember without any way to enforce the requirement in code (that I know
of). 

  Otherwise, we end up with various hidden inconsistencies as a result of the
fact that PostgreSQL is actually tracking microseconds in every date/time type.
If we hide the microseconds, we have two choices: we can round off the
microseconds, or we can truncate them. Either way, this means that 10:10:10 and
10:10:10 aren't guaranteed to have *actually* been simultaneous, and 10:10:10
and 10:10:11 aren't guaranteed to have *actually* been one second apart.

  This is the worst when you're dealing with delta_ts and lastdiffed, which are
almost always a half-second apart. One way or another, if we don't keep
microseconds in them, we'll end up in trouble. When I was working on the patch
for the double-comment bug in the Red Hat Bugzilla, I tried a *lot* of different
ways to work around this. The only viable long-term solution was to keep the
microseconds.

  But, if you keep microseconds on one field, you have to keep microseconds on
all of them, since you *might* at some point compare a date-with-microseconds to
a date-without-microseconds and get an invalid answer. (Even if Bugzilla doesn't
compare two specific date fields now, it may in the future.)

  So, all storing and comparison of dates in Bugzilla has to be done in the
native format of the database being used. Otherwise, it's a headache and hidden
bugs waiting to happen. It's okay to SELECT them with DateFormat, but *only* if
you're not going to use that as a comparison argument.

  What about moving bugs from MySQL to PgSQL? Well, that's not a problem, since
the MySQL dates will all be correct relevant to each other, and the new PgSQL
dates will all *become* correct relevant to each other on the first update. I'm
not sure that moving between databases is something that we even want to think
about right now, though. :-)

-M
Comment 94 Max Kanat-Alexander 2004-02-18 18:27:14 PST
Comment on attachment 141703 [details] [diff] [review]
Port CVS Head to PostgreSQL (v7)

  Okay, going over DBCompat.pm first, again.

  You might want to put a comment at the top of the file about the fact that
not all of these functions are currently used in every SendSQL, but should be
if they can be. Also, that Oracle support is nonexistent, currently, but
perhaps give them a bug number to contribute to.

>+# Sybase:   Dave Miller <justdave@netscape.com>
>+#       Gayathri Swaminath <gayathrik00@netscape.com>

  I think that justdave is at bugzilla.org, now.

>+sub LastKey {
>+    my ($table, $column) = @_;
>+    if ($db_driver eq 'mysql') {
>+        return "SELECT LAST_INSERT_ID()";
>+    } elsif ($db_driver eq 'Pg') {
>+        my $seq = $table . "_" . $column . "_seq";
>+        return "SELECT currval('$seq')";

  I think that this shouldn't include the SELECT, so that people can use the
function as part of a larger SendSQL statement, if they want.

>+sub RegExp {

   My only concern with this function is that someday we might run into a
database that doesn't do RegExps the same way as other DBs do, and we have to
actually mangle the pattern itself. Honestly, I can't say if this will ever
happen, since I don't know how databases do RegExps besides PgSQL and MySQL.

>+# subroutine:   ToDays
> [snip]
>+# returns:      formatted SQL for TO_DAYS function. TO_DAYS($column)
> for Mysql and just $column for Postgresql. (scalar)

  What it returns changed for PgSQL. :-)

>+sub If {
> [snip]
>+    elsif ($db_driver eq "Oracle") {
>+        # Stub
>+    }

  If you're going to do the Stub for this one, you probably ought to do it for
the rest of them, too. :-) In fact, instead of doing a Stub here, you might
want to see if there's a ThrowCodeError for "not_implemented" or something.

>+# description:  Returns list of values from the enum data in Mysql 
>                 or a particular table in PostgreSQL and others.
>+#               Assumes that enums don't ever contain an apostrophe!

  Is that a safe assumption? (Did we actually make it before and it just wasn't
documented?)

>+# subroutine:   Interval
> [snip]

  This looks great.

>+sub DBConnect {

  Hrm, is this supposed to be called by ConnectToDatabase? (That is, what's it
for?)


>+sub DaysDiff {
>+    my ($lowdate, $highdate) = @_;
>+    if ($db_driver eq "mysql") {

  For the MySQL and PgSQL versions, it might make more sense to implement this
using the ToDays function that you wrote above instead of manually using the
TO_DAYS SQL. (It's too bad that we have to have this function at all -- I see
why, for Sybase, though.)

>+    elsif ($db_driver eq "Pg") {
>+        return "COALESCE($one,'$two')";

  Why does this have a space before the $two?

>+# subroutine:   IfNullString
>+# description:  Outputs SQL syntax for the Mysql IFNULL function based on database type

   You might want to describe when this should be used instead of good ol'
IfNull.

>+sub IsNullString {

  Also, this sub is called "IsNullString" but the docs are for "IfNullString"

>+sub Limit {
> [snip]
>+    elsif ($db_driver eq "Sybase") {
>+        # Sybase doesn't support LIMIT, but we have a hack in SendSQL
>+        # to remove it from the SQL string and emulate it from there
>+        # using $::db->{rowcount}
>+        return "LIMIT $limit";

  Hrm, I think that hack is only in justdave's version. I think that Sybase
support in general requires a *lot* of hacks, and that justdave was of the
opinion that upstream Bugzilla will never need to support it. If you removed
all of the if...sybase, I don't think anybody would complain.

>+sub DateFormat {
>+    my ($data) = @_;
>+    # For compatibility reasons, we force the format to
>+    my $format = "%Y.%m.%d %H:%i:%s";
>+    # for everyone.  Use Date::Parse and friends after retrieving it if you
>+    # need to convert it to another format.

  Eek. I can see why you do this, but I think that it is maybe not such a good
idea (see my previous comment for my reasoning). If Sybase support is the
reason, then I think we should scrap Sybase support.

>+# subroutine:   LockTable
> [snip]
>+# params:       $tables = hash structure containing table names and type of lock needed
>+#               { 'foo' => 'WRITE', 'bar' => 'READ', 'foo2 AS bar2' => 'READ' } 

  Is this a necessary syntax? Is it possible to just call LockTable() multiple
times in a row, or does that Not Work?

>+    if ($db_driver eq 'mysql') {
>+        $str = "LOCK TABLES ";

  If it's possible, I'd tend to think that this function should just *do* the
table locking, the SendSQL and everything. Is there a situation where LOCK
TABLE needs to be part of a larger statement? Other than that, I think it's a
really good idea and it looks. :-)

>+    } elsif ($db_driver eq 'Pg') {
>+       $str = "LOCK TABLE ";
>+        foreach my $key (keys %{$tablesref}) {
>+            push (@tables, "$key");
>+        }
>+        $str .= join(', ', @tables);
>+        return $str; 

  This LOCK mode will default to ACCESS EXCLUSIVE for PgSQL, which is a global
lock on that table. It means that nobody else can even *access* that table
while the lock is being held. I think that MySQL WRITE locks are close to PgSQL
EXCLUSIVE MODE and READ locks are close to PgSQL ACCESS SHARE MODE. I don't
know a lot about MySQL's locking. For PgSQL, I'd like to see people just use
PgSQL's inherent locking, except for when they need a guarantee that LastKey
won't change during their write. (In which case I think they should be using
transactions instead of locking, anyway.) However, for now you might want to
use the MODE statements.

  Also, does Bugzilla ever try to do a LOCK TABLES outside of a transaction? If
it does, that won't work for Pg.

  Man, that ol' DBCompat is getting pretty big. :-) It sure is impressive,
though.
Comment 95 Andrew Dunstan 2004-02-19 06:34:29 PST
(In reply to comment #94)
> For PgSQL, I'd like to see people just use
> PgSQL's inherent locking, except for when they need a guarantee that LastKey
> won't change during their write. (In which case I think they should be using
> transactions instead of locking, anyway.) However, for now you might want to
> use the MODE statements.
> 

LastKey appears to be calling currval() for Pg. This should be safe without any
special locking - see the Pg docs for currval(), which state:

"Return the value most recently obtained by nextval for this sequence in the
current session. (An error is reported if nextval has never been called for this
sequence in this session.) Notice that because this is returning a session-local
value, it gives a predictable answer even if other sessions are executing
nextval meanwhile."

and nextval() states:

"Advance the sequence object to its next value and return that value. This is
done atomically: even if multiple sessions execute nextval concurrently, each
will safely receive a distinct sequence value."


(I hope I haven't misunderstood what you said).
Comment 96 David Lawrence [:dkl] 2004-02-19 11:18:23 PST
(In reply to comment #94)
> (From update of attachment 141703 [details] [diff] [review])
>   Okay, going over DBCompat.pm first, again.
> 
>   You might want to put a comment at the top of the file about the fact that
> not all of these functions are currently used in every SendSQL, but should be
> if they can be. Also, that Oracle support is nonexistent, currently, but
> perhaps give them a bug number to contribute to.
> 
> >+# Sybase:   Dave Miller <justdave@netscape.com>
> >+#       Gayathri Swaminath <gayathrik00@netscape.com>
> 
>   I think that justdave is at bugzilla.org, now.
> 

Will fix.

> >+sub LastKey {
> >+    my ($table, $column) = @_;
> >+    if ($db_driver eq 'mysql') {
> >+        return "SELECT LAST_INSERT_ID()";
> >+    } elsif ($db_driver eq 'Pg') {
> >+        my $seq = $table . "_" . $column . "_seq";
> >+        return "SELECT currval('$seq')";
> 
>   I think that this shouldn't include the SELECT, so that people can use the
> function as part of a larger SendSQL statement, if they want.
> 

Good point. Easy change.

> >+sub RegExp {
> 
>    My only concern with this function is that someday we might run into a
> database that doesn't do RegExps the same way as other DBs do, and we have to
> actually mangle the pattern itself. Honestly, I can't say if this will ever
> happen, since I don't know how databases do RegExps besides PgSQL and MySQL.
> 

Well in the past I have not been able to find a way to do it with Oracle without
 using sort of external procedures. And then the syntax would be something like
Regex(column, pattern) which would then again be incompatible. So not that I
think of it having just the operator broken out would break if someone was using
Oracle if it returned and empty string. 

> >+# subroutine:   ToDays
> > [snip]
> >+# returns:      formatted SQL for TO_DAYS function. TO_DAYS($column)
> > for Mysql and just $column for Postgresql. (scalar)
> 
>   What it returns changed for PgSQL. :-)
> 

Will fix.

> >+sub If {
> > [snip]
> >+    elsif ($db_driver eq "Oracle") {
> >+        # Stub
> >+    }
> 
>   If you're going to do the Stub for this one, you probably ought to do it for
> the rest of them, too. :-) In fact, instead of doing a Stub here, you might
> want to see if there's a ThrowCodeError for "not_implemented" or something.
> 

Will change.

> >+# description:  Returns list of values from the enum data in Mysql 
> >                 or a particular table in PostgreSQL and others.
> >+#               Assumes that enums don't ever contain an apostrophe!
> 
>   Is that a safe assumption? (Did we actually make it before and it just wasn't
> documented?)
> 

I pulled the SplitEnumType straight out of globals.pl so I used the same
comment that was already there.

> >+# subroutine:   Interval
> > [snip]
> 
>   This looks great.
> 

Cool

> >+sub DBConnect {
> 
>   Hrm, is this supposed to be called by ConnectToDatabase? (That is, what's it
> for?)
> 

This was part of the merge of Jeroen's DBCompat.pm code so eventually it can be
called from within DB.pm to form the dsn line correctly. I hadn't implimented
that far yet.

> >+sub DaysDiff {
> >+    my ($lowdate, $highdate) = @_;
> >+    if ($db_driver eq "mysql") {
> 
>   For the MySQL and PgSQL versions, it might make more sense to implement this
> using the ToDays function that you wrote above instead of manually using the
> TO_DAYS SQL. (It's too bad that we have to have this function at all -- I see
> why, for Sybase, though.)

Ok, will change. 

> >+    elsif ($db_driver eq "Pg") {
> >+        return "COALESCE($one,'$two')";
> 
>   Why does this have a space before the $two?
> 
> >+# subroutine:   IfNullString
> >+# description:  Outputs SQL syntax for the Mysql IFNULL function based on
database type
> 
>    You might want to describe when this should be used instead of good ol'
> IfNull.
> 

Yeah, documentation incomplete. Will fix.

> >+sub IsNullString {
> 
>   Also, this sub is called "IsNullString" but the docs are for "IfNullString"
>

ditto
 
> >+sub Limit {
> > [snip]
> >+    elsif ($db_driver eq "Sybase") {
> >+        # Sybase doesn't support LIMIT, but we have a hack in SendSQL
> >+        # to remove it from the SQL string and emulate it from there
> >+        # using $::db->{rowcount}
> >+        return "LIMIT $limit";
> 
>   Hrm, I think that hack is only in justdave's version. I think that Sybase
> support in general requires a *lot* of hacks, and that justdave was of the
> opinion that upstream Bugzilla will never need to support it. If you removed
> all of the if...sybase, I don't think anybody would complain.
> 

I can remove the Sybase conditions if Dave doesn't think it will ever be merged
in. Dave?

> >+sub DateFormat {
> >+    my ($data) = @_;
> >+    # For compatibility reasons, we force the format to
> >+    my $format = "%Y.%m.%d %H:%i:%s";
> >+    # for everyone.  Use Date::Parse and friends after retrieving it if you
> >+    # need to convert it to another format.
> 
>   Eek. I can see why you do this, but I think that it is maybe not such a good
> idea (see my previous comment for my reasoning). If Sybase support is the
> reason, then I think we should scrap Sybase support.

If we pull out DateFormat altogether then this wont be a problem anymore.

> >+# subroutine:   LockTable
> > [snip]
> >+# params:       $tables = hash structure containing table names and type of
lock needed
> >+#               { 'foo' => 'WRITE', 'bar' => 'READ', 'foo2 AS bar2' => 'READ' } 
> 
>   Is this a necessary syntax? Is it possible to just call LockTable() multiple
> times in a row, or does that Not Work?

First stab. I think it is more efficient on the database side to just call it
altogether in one statement but I have not looked into whether it could be
separated out. Anyone tried this before?
 
> >+    if ($db_driver eq 'mysql') {
> >+        $str = "LOCK TABLES ";
> 
>   If it's possible, I'd tend to think that this function should just *do* the
> table locking, the SendSQL and everything. Is there a situation where LOCK
> TABLE needs to be part of a larger statement? Other than that, I think it's a
> really good idea and it looks. :-)
> 

It could, but I was trying to refrain from actually doing any real SQL calls
within DBcompat.pm and have it purely only return SQL strings. 

> >+    } elsif ($db_driver eq 'Pg') {
> >+       $str = "LOCK TABLE ";
> >+        foreach my $key (keys %{$tablesref}) {
> >+            push (@tables, "$key");
> >+        }
> >+        $str .= join(', ', @tables);
> >+        return $str; 
> 
>   This LOCK mode will default to ACCESS EXCLUSIVE for PgSQL, which is a global
> lock on that table. It means that nobody else can even *access* that table
> while the lock is being held. I think that MySQL WRITE locks are close to PgSQL
> EXCLUSIVE MODE and READ locks are close to PgSQL ACCESS SHARE MODE. I don't
> know a lot about MySQL's locking. For PgSQL, I'd like to see people just use
> PgSQL's inherent locking, except for when they need a guarantee that LastKey
> won't change during their write. (In which case I think they should be using
> transactions instead of locking, anyway.) However, for now you might want to
> use the MODE statements.
> 

Will look into that.

>   Also, does Bugzilla ever try to do a LOCK TABLES outside of a transaction? If
> it does, that won't work for Pg.
> 

Right, you need to be in a transaction to do table locks in Pg. I should
implement some sort of BeginWork(), Commit(), Rollback() functions also in
DBcompat.pm. These would need to be added in almost every CGI that does DB access.
 
Comment 97 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-02-19 13:30:33 PST
(In reply to comment #96)
> > >+# Sybase:   Dave Miller <justdave@netscape.com>
> > >+#       Gayathri Swaminath <gayathrik00@netscape.com>
> > 
> >   I think that justdave is at bugzilla.org, now.
> 
> Will fix.

Actually, make those <davem00@aol.com> and <gayathrik00@aol.com>.  Neither
address will work after next week (Gayathri's has been gone for a year already),
but AOL owns the copyright, so any relicensing issues would need to go through
them unless I can get them to reassign copyright on it to MF like they did with
the Mozilla stuff (I'll work on that).

> > >+# description:  Returns list of values from the enum data in Mysql 
> > >                 or a particular table in PostgreSQL and others.
> > >+#               Assumes that enums don't ever contain an apostrophe!
> > 
> >   Is that a safe assumption? (Did we actually make it before and it just wasn't
> > documented?)
> > 
> 
> I pulled the SplitEnumType straight out of globals.pl so I used the same
> comment that was already there.

I would venture to say that we're intending for this to just go away in the
MySQL code as well.  However, if you've already got the compatibility hack for
it, there's no reason not to use this until then.

> > >+sub Limit {
> > > [snip]
> > >+    elsif ($db_driver eq "Sybase") {
> > >+        # Sybase doesn't support LIMIT, but we have a hack in SendSQL
> > >+        # to remove it from the SQL string and emulate it from there
> > >+        # using $::db->{rowcount}
> > >+        return "LIMIT $limit";
> > 
> >   Hrm, I think that hack is only in justdave's version. I think that Sybase
> > support in general requires a *lot* of hacks, and that justdave was of the
> > opinion that upstream Bugzilla will never need to support it. If you removed
> > all of the if...sybase, I don't think anybody would complain.
> > 
> 
> I can remove the Sybase conditions if Dave doesn't think it will ever be
> merged in. Dave?

I don't think we'll ever have Sybase support out of the box.  However, I have no
objection to including code that will make the hacking easier to do.

> > >+sub DateFormat {
> > >+    # For compatibility reasons, we force the format to
> > >+    my $format = "%Y.%m.%d %H:%i:%s";
> > >+    # for everyone.  Use Date::Parse and friends after retrieving it if you
> > >+    # need to convert it to another format.
> > 
> >   Eek. I can see why you do this, but I think that it is maybe not such a
> > good idea (see my previous comment for my reasoning). If Sybase support is
> > the reason, then I think we should scrap Sybase support.
> 
> If we pull out DateFormat altogether then this wont be a problem anymore.

You have to date format on Sybase in order to get accurate resolution, because
it'll format it for you if you don't ask it to, and the default format doesn't
even include seconds.  But yes, if Sybase is the only reason we need it, skip
it.  I still think there's a bit to be gained by using a common format to
retrieve the timestamps and letting the display code deal with formatting it. 
Date formatting tends to be someone people like to localize anyway.

> >   Is this a necessary syntax? Is it possible to just call LockTable() multiple
> > times in a row, or does that Not Work?
> 
> First stab. I think it is more efficient on the database side to just call it
> altogether in one statement but I have not looked into whether it could be
> separated out. Anyone tried this before?

You can't in MySQL.  Any time you call LOCK TABLES in MySQL it unlocks anything
you already had locked previously that's not mentioned in the current lock
statement.

> >   If it's possible, I'd tend to think that this function should just *do* the
> > table locking, the SendSQL and everything. Is there a situation where LOCK
> > TABLE needs to be part of a larger statement? Other than that, I think it's a
> > really good idea and it looks. :-)
> > 
> 
> It could, but I was trying to refrain from actually doing any real SQL calls
> within DBcompat.pm and have it purely only return SQL strings. 

Maybe LockTables should be a function in Bugzilla::DB.pm.  It should in turn
retrieve the SQL from DBCompat. :)  (just throwing ideas out, don't feel like
you have to follow it)

> >   Also, does Bugzilla ever try to do a LOCK TABLES outside of a transaction?
> > If it does, that won't work for Pg.
> 
> Right, you need to be in a transaction to do table locks in Pg. I should
> implement some sort of BeginWork(), Commit(), Rollback() functions also in
> DBcompat.pm. These would need to be added in almost every CGI that does DB
> access.  

Yes, MySQL (at the time all of Bugzilla was originally written) didn't have
transactions.  Using locks is how you create a transaction type environment in
MySQL (except that you don't get rollback).  We have autocommit turned on in the
DBI handle.
Comment 98 Ed Sabol 2004-02-19 14:49:05 PST
Frankly, I don't like what I'm reading in the recent comments here. Please don't
throw out any of the existing Sybase stuff in DBCompat.pm or limit your solution
in such a way that it would make adding Sybase support more difficult. The
appropriate solution to this bug should make Bugzilla work with virtually any
RDBMS, not just PostgreSQL and MySQL. Don't throw the baby out with the bath
water just because all you are personally concerned about is PostgreSQL.
Comment 99 Max Kanat-Alexander 2004-02-19 17:49:51 PST
(In reply to comment #98)
> Frankly, I don't like what I'm reading in the recent comments here. Please don't
> throw out any of the existing Sybase stuff in DBCompat.pm or limit your solution
> in such a way that it would make adding Sybase support more difficult.

  Ed --

  It's good to know that there are users interested in Sybase support. For right
now, I think our focus is to push out a version that supports *any* other
database, *at all*. :-) To that effect, I'd like to make the port to that
platform (Postgres) as easy as possible, and possibly grease other ports along
the way. 

Sybase implemented its database in a way that made sense at the time, but
doesn't make as much sense in the modern world. That doesn't mean that I think
that we should make life hard for people who want to port Bugzilla to Sybase --
quite the contrary. :-) I'd love to see Bugzilla work on every database in
existence. I'd also like to see a Postgres version get out the door, a version
with as few potential bugs as possible, that doesn't break the MySQL version. 

We're been waiting for PostgreSQL support for over two years, now. At this
point, if Sybase support will delay PostgreSQL support, then I think that we
should skip it. However, I don't see anything wrong with Sybase support in 2.20
or 2.22.

-M
Comment 100 Max Kanat-Alexander 2004-02-19 18:03:06 PST
dkl -- In regard to REPLACE, I just noticed comment 35 and bug 190432 -- I think
one of those has the fix.
Comment 101 David Lawrence [:dkl] 2004-02-19 18:56:47 PST
(In reply to comment #100)
> dkl -- In regard to REPLACE, I just noticed comment 35 and bug 190432 -- I think
> one of those has the fix.

When transactions are in place for both PostgreSQL and MySQL with proper table
locking then this should no longer be a problem with race conditions right?
Probably will defer worrying about this one til the bigger picture is solved.
Comment 102 Jeroen Ruigrok van der Werven 2004-02-19 22:14:20 PST
For those we are interested in MySQL <> PostgreSQL SQL differences, please see:

http://www.in-nomine.org/~asmodai/mysql-to-pgsql.txt

I need to update it again and rework it though.
Comment 103 David Lawrence [:dkl] 2004-02-20 11:27:00 PST
(In reply to comment #99)
> (In reply to comment #98)
> > Frankly, I don't like what I'm reading in the recent comments here. Please don't
> > throw out any of the existing Sybase stuff in DBCompat.pm or limit your solution
> > in such a way that it would make adding Sybase support more difficult.
> 
>   Ed --
> 
>   It's good to know that there are users interested in Sybase support. For right
> now, I think our focus is to push out a version that supports *any* other
> database, *at all*. :-) To that effect, I'd like to make the port to that
> platform (Postgres) as easy as possible, and possibly grease other ports along
> the way. 
> 
> Sybase implemented its database in a way that made sense at the time, but
> doesn't make as much sense in the modern world. That doesn't mean that I think
> that we should make life hard for people who want to port Bugzilla to Sybase --
> quite the contrary. :-) I'd love to see Bugzilla work on every database in
> existence. I'd also like to see a Postgres version get out the door, a version
> with as few potential bugs as possible, that doesn't break the MySQL version. 
> 
> We're been waiting for PostgreSQL support for over two years, now. At this
> point, if Sybase support will delay PostgreSQL support, then I think that we
> should skip it. However, I don't see anything wrong with Sybase support in 2.20
> or 2.22.
> 
> -M


No worries, ed. I am not going to remove any previous Sybase work that is
currently there. But it has been a long time goal of mine to get Pg support into
the main code base so I think we should focus on Mysql and Pg for the time being
and worry about other support in future releases. If anyone is actively using
Sybase for Bugzilla (Dave?) and would be interested in testing then that would
be great. Otherwise I don't have access to a server myself.

-d
Comment 104 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-02-20 12:26:46 PST
(In reply to comment #103)
> If anyone is actively using
> Sybase for Bugzilla (Dave?) and would be interested in testing then that would
> be great. Otherwise I don't have access to a server myself.

I lose access to mine in a week.

I do have the "developers edition" of Sybase ASE on my laptop, but I don't have
enough hard drive space to actually run it :)  So I could probably test again at
some point in the future when I come up with a bigger hard drive.
Comment 105 Ed Sabol 2004-02-20 14:00:17 PST
(In reply to comment #103)
> No worries, ed. I am not going to remove any previous Sybase work that is
> currently there. But it has been a long time goal of mine to get Pg support into
> the main code base so I think we should focus on Mysql and Pg for the time being
> and worry about other support in future releases. If anyone is actively using
> Sybase for Bugzilla (Dave?) and would be interested in testing then that would
> be great. Otherwise I don't have access to a server myself.

I've got no problems with focusing on PostgreSQL first, and I applaud the
efforts so far. My main concerns were about a couple of comments that "Bugzilla
would never support Sybase" (or words to that effect) followed by suggestions to
rip out the Sybase support that was already present. I'm glad to hear that that
will not be happening, Dave. Although I do have a particular interest in using
Bugzilla on Sybase, I would hope that I'm not speaking only for the prospective
Sybase Bugzilla users, but also for users of Oracle and other popular DBMSes.
The goal should be make Bugzilla as DBMS-agnostic as possible.

As far as date/time fields go, I strongly feel that you should consider NOT
using DBMS-specific date/time field types at all. I would strongly advocate
using char fields or float fields. I've been writing DBMS-agnostic database
applications for years, specifically Sybase, Oracle, INGRES, and MySQL, and, if
you steer clear of the things that the various DBMS implementations disagree
upon from the beginning, then you save yourself a lot of work. A char field
containing a date/time in ISO 8601 format (YYYY-MM-DD hh:mm:ss.ssss) is just as
effective (alphabetical sorting results in chronological sorting) and it will
work with every DBMS out there guaranteed. For some applications, to save on
storage space, I convert date/times to MJDs (Modified Julian Dates) and store
them as floats. Anyway, those are a couple of options that I feel should be
considered.
Comment 106 David Lawrence [:dkl] 2004-02-20 14:29:16 PST
Created attachment 141854 [details] [diff] [review]
Changes to implement Table Locking in PostgreSQL using DBcompat.pm

Here is my latest changes to DBcompat.pm so that people could chew on it over
the weekend. I have implemented LockTable and UnlockTable differently in that I
am performing the actual locks inside instead of simply returning the SQL
string.
This allows me to format it specifically for the database or return nothing if
we don't need it. I have only converted votes.cgi so far and it seems to work.
You can look at that file to see how the function is called.

Also cleaned up some things, added some more docs, throws code error for
functions not yet supported for a particular database.

More to come!
Comment 107 Bradley Baetz (:bbaetz) 2004-02-20 18:30:33 PST
I will look at this, at some point. One comment is that it may be better off
going to transactions for everything, since we're going to have to audit all the
LOCK TABLE commands anyway to do this.

That requires innodb for mysql, though, and that is very, very, very, very slow.
Maybe they've improved it recently - anyone know?

Re Sybase, there was a whole lot of uglyness required, starting at the fact that
the DBD driver doesn't let you have two open statements at once. That one is a
killer. MySQL does support that (although by default it reads all the data into
RAM at once) Postgres < 7.4 doesn't support it at all, but DBD hides it by
storing all the data internally as a buffer. Or you could use cursors, but thats
an invasive change. I don't know if DBD for postgres >= 7.4 uses the max row
count option to fetch bits of a portal, rather than the entire thing at once.
Starting up a seccond DBI connection for Sybase doesn't work becaus you then
don't have LOCKs/transactions from the first.
Comment 108 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-02-20 18:38:11 PST
(In reply to comment #107)
> Re Sybase, there was a whole lot of uglyness required, starting at the fact
> that the DBD driver doesn't let you have two open statements at once. That
> one is a killer.

Actually, the DBD does let you have multiple open statements.  But the core
C-level driver for Sybase doesn't.  The DBD will emulate it by opening
additional connections behind the scenes and not telling you (which of course
kills all hopes of maintaining locks and transactions).  You can, of course,
disable that behavior, and force it to only have one open connection, but then
you have all kinds of problems when you're dealing with looping over data in
hopes of doing something with it, unless you can afford to load all of said data
into RAM first.
Comment 109 Bradley Baetz (:bbaetz) 2004-02-20 18:45:20 PST
I didn't realise that DBD did the multiple conenctions thing for you. Its still
silly and broken, though. Like I said, other DBs have DBD pretend to support this...
Comment 110 David Lawrence [:dkl] 2004-02-27 15:16:55 PST
Created attachment 142476 [details] [diff] [review]
Port CVS Head to PostgreSQL (v8)

Change all table locking to use new LockTables() and UnlockTables in
DBcompat.pm. Added new subroutines StartTransaction() and EndTransaction() as a
first step towards transaction support. Seems to work correctly since DBI will
automatically switch over to AutoCommit=0 when a begin_work is called and back
to AutoCommit=1 when commited. Also changed LIMIT to support Mysql and
Postgresql style OFFSET.
Comment 111 Christian Reis 2004-02-27 15:41:41 PST
If we want to move on with this, I'd suggest us making a landing plan, which
involves perhaps breaking up the patch into smaller bits and tying them into
separate bugs for review. If the patches are small enough, I volunteer to review
them.
Comment 112 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-02-27 17:00:43 PST
Comment on attachment 142476 [details] [diff] [review]
Port CVS Head to PostgreSQL (v8)

>Index: Attachment.pm
>@@ -76,9 +76,9 @@
>-              SELECT attach_id, DATE_FORMAT(creation_ts, '%Y.%m.%d %H:%i'),
>+              SELECT attach_id, " . Bugzilla::DB::DateFormat('creation_ts', '%Y.%m.%d %H:%i') . ", 

This was this way already, so we probably should fix it in another bug, but
shouldn't we be retrieving seconds on all our time retrievals?

>Index: Bug.pm
>@@ -148,7 +148,14 @@
>     where bugs.bug_id = $bug_id
>       AND products.id = bugs.product_id
>       AND components.id = bugs.component_id
>-    group by bugs.bug_id";
>+    GROUP BY 
>+      bugs.bug_id, alias, bugs.product_id, products.name, version, 
>+      rep_platform, op_sys, bug_status, resolution, priority, 
>+      bug_severity, bugs.component_id, components.name, assigned_to, 
>+      reporter, bug_file_loc, short_desc, target_milestone, 
>+      qa_contact, status_whiteboard, creation_ts, delta_ts,
>+      reporter_accessible, cclist_accessible,
>+      estimated_time, remaining_time";
> 
>   &::SendSQL($query);
>   my @row = ();

This completely toasted us on Sybase...  what does DISTINCT bug_id do if you
use that at the beginning of the query in place of the GROUP BY?  That's what
we ended up having to do for Sybase.

>@@ -211,44 +218,43 @@
>       $self->{'keywords'} = join(', ', @list);
>     }
>   }
>-
>   $self->{'attachments'} = Attachment::query($self->{bug_id});
> 
>   # The types of flags that can be set on this bug.
>   # If none, no UI for setting flags will be displayed.
>-  my $flag_types = 
>-    Bugzilla::FlagType::match({ 'target_type'  => 'bug', 
>-                                'product_id'   => $self->{'product_id'}, 
>-                                'component_id' => $self->{'component_id'} });
>-  foreach my $flag_type (@$flag_types) {
>-      $flag_type->{'flags'} = 
>-        Bugzilla::Flag::match({ 'bug_id'      => $self->{bug_id},
>-                                'type_id'     => $flag_type->{'id'},
>-                                'target_type' => 'bug' });
>-  }
>-  $self->{'flag_types'} = $flag_types;
>-  $self->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
>+  #my $flag_types = 
>+  #  Bugzilla::FlagType::match({ 'target_type'  => 'bug', 
>+  #                              'product_id'   => $self->{'product_id'}, 
>+  #                              'component_id' => $self->{'component_id'} });
>+  #foreach my $flag_type (@$flag_types) {
>+  #    $flag_type->{'flags'} = 
>+  #      Bugzilla::Flag::match({ 'bug_id'      => $self->{bug_id},
>+  #                              'type_id'     => $flag_type->{'id'},
>+  #                              'target_type' => 'bug' });
>+  #}
>+  #$self->{'flag_types'} = $flag_types;
>+  #$self->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
> 
>   # The number of types of flags that can be set on attachments to this bug
>   # and the number of flags on those attachments.  One of these counts must be
>   # greater than zero in order for the "flags" column to appear in the table
>   # of attachments.
>-  my $num_attachment_flag_types =
>-    Bugzilla::FlagType::count({ 'target_type'  => 'attachment',
>-                                'product_id'   => $self->{'product_id'},
>-                                'component_id' => $self->{'component_id'},
>-                                'is_active'    => 1 });
>-  my $num_attachment_flags =
>-    Bugzilla::Flag::count({ 'target_type'  => 'attachment',
>-                            'bug_id'       => $self->{bug_id} });
>-
>-  $self->{'show_attachment_flags'}
>-    = $num_attachment_flag_types || $num_attachment_flags;
>+  #my $num_attachment_flag_types =
>+  #  Bugzilla::FlagType::count({ 'target_type'  => 'attachment',
>+  #                              'product_id'   => $self->{'product_id'},
>+  #                              'component_id' => $self->{'component_id'},
>+  #                              'is_active'    => 1 });
>+  #my $num_attachment_flags =
>+  #  Bugzilla::Flag::count({ 'target_type'  => 'attachment',
>+  #                          'bug_id'       => $self->{bug_id} });
>+  #
>+  #$self->{'show_attachment_flags'}
>+  #  = $num_attachment_flag_types || $num_attachment_flags;
> 
>-  $self->{'milestoneurl'} = $::milestoneurl{$self->{product}};
>+  #$self->{'milestoneurl'} = $::milestoneurl{$self->{product}};
> 
>-  $self->{'isunconfirmed'} = ($self->{bug_status} eq $::unconfirmedstate);
>-  $self->{'isopened'} = &::IsOpenedState($self->{bug_status});
>+  #$self->{'isunconfirmed'} = ($self->{bug_status} eq $::unconfirmedstate);
>+  #$self->{'isopened'} = &::IsOpenedState($self->{bug_status});
>   
>   my @depends = EmitDependList("blocked", "dependson", $bug_id);
>   if (@depends) {

Why is the flag stuff all getting commented out?

>@@ -340,11 +346,11 @@
>              " LEFT JOIN user_group_map" .
>              " ON user_group_map.group_id = groups.id" .
>              " AND user_id = $::userid" .
>-             " AND NOT isbless" .
>+             " AND isbless = 0" .
>              " LEFT JOIN group_control_map" .
>              " ON group_control_map.group_id = groups.id" .
>              " AND group_control_map.product_id = " . $self->{'product_id'} .
>-             " WHERE isbuggroup");
>+             " WHERE isbuggroup = 1");

Heh, I see Postgres has the same problem with Booleans that Sybase does. :)

>Index: Bugzilla.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v
>retrieving revision 1.9
>diff -u -r1.9 Bugzilla.pm
>--- Bugzilla.pm	27 Nov 2003 01:00:59 -0000	1.9
>+++ Bugzilla.pm	27 Feb 2004 23:08:45 -0000
>@@ -29,6 +29,7 @@
> use Bugzilla::Config;
> use Bugzilla::Constants;
> use Bugzilla::DB;
>+use Bugzilla::DBcompat;
> use Bugzilla::Template;
> use Bugzilla::User;

Why are we adding Bugzilla::DBcompat here if we're not adding any references to
it?

>Index: CGI.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v
>retrieving revision 1.209
>diff -u -r1.209 CGI.pl
>--- CGI.pl	30 Oct 2003 01:31:57 -0000	1.209
>+++ CGI.pl	27 Feb 2004 23:08:45 -0000
>@@ -313,21 +313,22 @@
>     die "Invalid id: $id" unless $id=~/^\s*\d+\s*$/;
> 
>     if (defined $starttime) {
>-        $datepart = "and bugs_activity.bug_when > " . SqlQuote($starttime);
>+        $datepart = "and bugs_activity.bug_when > TO_TIMESTAMP(" . SqlQuote($starttime) . ", 'YYYYMMDDHH24MISS') ";
>     }

Won't this break MySQL?  This should be a call into one of the compatibility
functions.  In fact, the entire date comparison should probably be a
compatibility function, since you have to do DATEDIFF() and check if the result
is positive or not on Sybase.

>@@ -157,10 +157,10 @@
> sub CleanTokenTable {
>-    &::SendSQL("LOCK TABLES tokens WRITE");
>-    &::SendSQL("DELETE FROM tokens 
>-                WHERE TO_DAYS(NOW()) - TO_DAYS(issuedate) >= " . $maxtokenage);
>-    &::SendSQL("UNLOCK TABLES");
>+    Bugzilla::DB::LockTables("LOCK TABLES tokens WRITE");
>+    &::SendSQL("DELETE FROM tokens WHERE " . Bugzilla::DB::ToDays('NOW()') . " - " . 
>+                Bugzilla::DB::ToDays('issuedate') . " >= " . $maxtokenage);
>+    Bugzilla::DB::UnlockTables();

OK, I realize this was already here, but what's the point of a lock here? 
Isn't a single DELETE statement atomic on its own?

>@@ -233,9 +233,9 @@
>     close SENDMAIL;
> 
>     # Delete the token from the database.
>-    &::SendSQL("LOCK TABLES tokens WRITE");
>+    Bugzilla::DB::LockTables("LOCK TABLES tokens WRITE");
>     &::SendSQL("DELETE FROM tokens WHERE token = $quotedtoken");
>-    &::SendSQL("UNLOCK TABLES");
>+    Bugzilla::DB::UnlockTables();
> }

ditto here.

>Index: attachment.cgi
>@@ -476,6 +477,11 @@
>+    if ($db_driver eq 'Pg') {
>+        use MIME::Base64;
>+        $thedata = decode_base64($thedata);
>+    }

Heh, this brings back memories.  We have to encode it as hexadecimal digits for
Sybase.

>@@ -837,15 +843,23 @@
>   $filename = SqlQuote($filename);
>   my $description = SqlQuote($::FORM{'description'});
>   my $contenttype = SqlQuote($::FORM{'contenttype'});
>-  my $thedata = SqlQuote($data);
>+  my $thedata = $::FORM{'data'};

Where did we lose $data from that we've going back to the FORM to get it?  If
we need to do this, use $cgi->param('data').  $::FORM is going away, if it
hasn't already, and you'll have less cvs conflicts :)

>@@ -961,15 +975,16 @@
>   # Get a list of flag types that can be set for this attachment.
>   SendSQL("SELECT product_id, component_id FROM bugs WHERE bug_id = $bugid");
>   my ($product_id, $component_id) = FetchSQLData();
>-  my $flag_types = Bugzilla::FlagType::match({ 'target_type'  => 'attachment' , 
>-                                               'product_id'   => $product_id , 
>-                                               'component_id' => $component_id });
>-  foreach my $flag_type (@$flag_types) {
>-    $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->{'id'}, 
>-                                                    'attach_id' => $::FORM{'id'} });
>-  }
>-  $vars->{'flag_types'} = $flag_types;
>-  $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
>+  # PGSQL
>+#  my $flag_types = Bugzilla::FlagType::match({ 'target_type'  => 'attachment' , 
>+#                                               'product_id'   => $product_id , 
>+#                                               'component_id' => $component_id });
>+#  foreach my $flag_type (@$flag_types) {
>+#    $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->{'id'}, 
>+#                                                    'attach_id' => $::FORM{'id'} });
>+#  }
>+#  $vars->{'flag_types'} = $flag_types;
>+#  $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);

Again we have flagtype stuff commented out.

>Index: buglist.cgi
>@@ -220,12 +220,12 @@
>     return if !$userid;
> 
>     SendSQL("
>-        SELECT DISTINCT  groups.id, name, description, isactive
>-                   FROM  groups, user_group_map
>-                  WHERE  user_id = $userid AND NOT isbless
>-                    AND  user_group_map.group_id = groups.id
>-                    AND  isbuggroup
>-               ORDER BY  description ");
>+        SELECT  groups.id, name, description, isactive
>+          FROM  groups, user_group_map
>+         WHERE  user_id = $userid AND isbless = 0
>+           AND  user_group_map.group_id = groups.id
>+           AND  isbuggroup = 1
>+      ORDER BY  description ");

Why did we lose the DISTINCT here?  Was it unnecessary to begin with?

>Index: collectstats.pl

reminder for myself that I left off here. :)

More to come...
Comment 113 Andrew Dunstan 2004-02-27 17:33:21 PST
(In reply to comment #110)

Surely the correct way to do attachments in Postgres is to use a field of type
BYTEA, which does binary data quite happily. Alternatively, Postgres has its own
lightning fast base64 encode and decode routines. Using MIME::Base64 routines
seems ... odd. Let the database do the work for you.
Comment 114 David Lawrence [:dkl] 2004-03-02 14:44:04 PST
(In reply to comment #112)
> (From update of attachment 142476 [details] [diff] [review])
> >Index: Attachment.pm
> >@@ -76,9 +76,9 @@
> >-              SELECT attach_id, DATE_FORMAT(creation_ts, '%Y.%m.%d %H:%i'),
> >+              SELECT attach_id, " . Bugzilla::DB::DateFormat('creation_ts',
'%Y.%m.%d %H:%i') . ", 
> 
> This was this way already, so we probably should fix it in another bug, but
> shouldn't we be retrieving seconds on all our time retrievals?
> 

Added %s to the format string. 

> >Index: Bug.pm
> >@@ -148,7 +148,14 @@
> >     where bugs.bug_id = $bug_id
> >       AND products.id = bugs.product_id
> >       AND components.id = bugs.component_id
> >-    group by bugs.bug_id";
> >+    GROUP BY 
> >+      bugs.bug_id, alias, bugs.product_id, products.name, version, 
> >+      rep_platform, op_sys, bug_status, resolution, priority, 
> >+      bug_severity, bugs.component_id, components.name, assigned_to, 
> >+      reporter, bug_file_loc, short_desc, target_milestone, 
> >+      qa_contact, status_whiteboard, creation_ts, delta_ts,
> >+      reporter_accessible, cclist_accessible,
> >+      estimated_time, remaining_time";
> > 
> >   &::SendSQL($query);
> >   my @row = ();
> 
> This completely toasted us on Sybase...  what does DISTINCT bug_id do if you
> use that at the beginning of the query in place of the GROUP BY?  That's what
> we ended up having to do for Sybase.
> 

This still doesn't work with PostgreSQL. Adding DISTINCT and removing the GROUP
BY causes SQL error because of the sum() function around votes.count.


> >+  #my $flag_types = 
> >+  #  Bugzilla::FlagType::match({ 'target_type'  => 'bug', 
> >+  #                              'product_id'   => $self->{'product_id'}, 
> >+  #                              'component_id' => $self->{'component_id'} });
> >+  #foreach my $flag_type (@$flag_types) {
> >+  #    $flag_type->{'flags'} = 
> >+  #      Bugzilla::Flag::match({ 'bug_id'      => $self->{bug_id},
> >+  #                              'type_id'     => $flag_type->{'id'},
> >+  #                              'target_type' => 'bug' });
> >+  #}
> >+  #$self->{'flag_types'} = $flag_types;
> >+  #$self->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'},
@$flag_types);
>
> Why is the flag stuff all getting commented out?
>

Fixed in my latest code. PostgreSQL was failing complaining of missing a JOINed
table in one of the LEFT JOIN clauses. Apparently MySQL was including it
automatically and Pg couldn't figure it out. I had the code commented out so
that I would come back and visit the problem later.
 
> >Index: Bugzilla.pm
> >===================================================================
> >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v
> >retrieving revision 1.9
> >diff -u -r1.9 Bugzilla.pm
> >--- Bugzilla.pm	27 Nov 2003 01:00:59 -0000	1.9
> >+++ Bugzilla.pm	27 Feb 2004 23:08:45 -0000
> >@@ -29,6 +29,7 @@
> > use Bugzilla::Config;
> > use Bugzilla::Constants;
> > use Bugzilla::DB;
> >+use Bugzilla::DBcompat;
> > use Bugzilla::Template;
> > use Bugzilla::User;
> 
> Why are we adding Bugzilla::DBcompat here if we're not adding any references to
> it?

Removed. Not necessary.  

> >Index: CGI.pl
> >===================================================================
> >RCS file: /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v
> >retrieving revision 1.209
> >diff -u -r1.209 CGI.pl
> >--- CGI.pl	30 Oct 2003 01:31:57 -0000	1.209
> >+++ CGI.pl	27 Feb 2004 23:08:45 -0000
> >@@ -313,21 +313,22 @@
> >     die "Invalid id: $id" unless $id=~/^\s*\d+\s*$/;
> > 
> >     if (defined $starttime) {
> >-        $datepart = "and bugs_activity.bug_when > " . SqlQuote($starttime);
> >+        $datepart = "and bugs_activity.bug_when > TO_TIMESTAMP(" .
SqlQuote($starttime) . ", 'YYYYMMDDHH24MISS') ";
> >     }
> 
> Won't this break MySQL?  This should be a call into one of the compatibility
> functions.  In fact, the entire date comparison should probably be a
> compatibility function, since you have to do DATEDIFF() and check if the result
> is positive or not on Sybase.
> 

Removed the TO_TIMESTAMP. Pg seems to work happily without it.

> >@@ -157,10 +157,10 @@
> > sub CleanTokenTable {
> >-    &::SendSQL("LOCK TABLES tokens WRITE");
> >-    &::SendSQL("DELETE FROM tokens 
> >-                WHERE TO_DAYS(NOW()) - TO_DAYS(issuedate) >= " . $maxtokenage);
> >-    &::SendSQL("UNLOCK TABLES");
> >+    Bugzilla::DB::LockTables("LOCK TABLES tokens WRITE");
> >+    &::SendSQL("DELETE FROM tokens WHERE " . Bugzilla::DB::ToDays('NOW()') .
" - " . 
> >+                Bugzilla::DB::ToDays('issuedate') . " >= " . $maxtokenage);
> >+    Bugzilla::DB::UnlockTables();
> 
> OK, I realize this was already here, but what's the point of a lock here? 
> Isn't a single DELETE statement atomic on its own?
> 
> >@@ -233,9 +233,9 @@
> >     close SENDMAIL;
> > 
> >     # Delete the token from the database.
> >-    &::SendSQL("LOCK TABLES tokens WRITE");
> >+    Bugzilla::DB::LockTables("LOCK TABLES tokens WRITE");
> >     &::SendSQL("DELETE FROM tokens WHERE token = $quotedtoken");
> >-    &::SendSQL("UNLOCK TABLES");
> >+    Bugzilla::DB::UnlockTables();
> > }
> 
> ditto here.
> 

Not sure about this one. Tried to find some real world cases supporting whether
it is necessary or not. I feel it may be necessary when dealing with tables that
have a primary key as one of the columns.

> >@@ -837,15 +843,23 @@
> >   $filename = SqlQuote($filename);
> >   my $description = SqlQuote($::FORM{'description'});
> >   my $contenttype = SqlQuote($::FORM{'contenttype'});
> >-  my $thedata = SqlQuote($data);
> >+  my $thedata = $::FORM{'data'};
> 
> Where did we lose $data from that we've going back to the FORM to get it?  If
> we need to do this, use $cgi->param('data').  $::FORM is going away, if it
> hasn't already, and you'll have less cvs conflicts :)
>

Fixed.
 
> >Index: buglist.cgi
> >@@ -220,12 +220,12 @@
> >     return if !$userid;
> > 
> >     SendSQL("
> >-        SELECT DISTINCT  groups.id, name, description, isactive
> >-                   FROM  groups, user_group_map
> >-                  WHERE  user_id = $userid AND NOT isbless
> >-                    AND  user_group_map.group_id = groups.id
> >-                    AND  isbuggroup
> >-               ORDER BY  description ");
> >+        SELECT  groups.id, name, description, isactive
> >+          FROM  groups, user_group_map
> >+         WHERE  user_id = $userid AND isbless = 0
> >+           AND  user_group_map.group_id = groups.id
> >+           AND  isbuggroup = 1
> >+      ORDER BY  description ");
> 
> Why did we lose the DISTINCT here?  Was it unnecessary to begin with?
>

Fixed. 


Comment 115 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-02 15:02:02 PST
(In reply to comment #114)
> (In reply to comment #112)
> > This completely toasted us on Sybase...  what does DISTINCT bug_id do if you
> > use that at the beginning of the query in place of the GROUP BY?  That's what
> > we ended up having to do for Sybase.
> 
> This still doesn't work with PostgreSQL. Adding DISTINCT and removing the GROUP
> BY causes SQL error because of the sum() function around votes.count.

OK, I'd rather have MySQL and Postgres both working than Sybase. :)  Like I've
said before, Sybase is just too different - it'll never work out of the box.  We
can get it as close as we can though.
I'd be willing to bet anyone stuck using Sybase will have people on site capable
of completing the conversion. :)
Comment 116 Max Kanat-Alexander 2004-03-09 17:15:44 PST
dkl --

Do you think that you could possibly split DBCompat.pm and the standard MySQL
changes out into two other patches? I think it'd be way easier to push this in
as three patches (1st DBCompat.pm, 2nd MySQL changes, 3rd PostgreSQL changes).

Of course, that doesn't mean that patch 2 can't modify patch 1, etc., but it'd
just be nice to be able to push *something* in. :-)

If we could get DBCompat.pm in 2.18, that'd be pretty nice. (I can always dream...)
Comment 117 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-17 00:09:33 PST
This is close enough that it might make it, and I'd love to see it get in. 
Putting in the blocking queue for a later decision after seeing how far we get. :)
Comment 118 David Lawrence [:dkl] 2004-03-17 14:55:21 PST
(In reply to comment #116)
> dkl --
> 
> Do you think that you could possibly split DBCompat.pm and the standard MySQL
> changes out into two other patches? I think it'd be way easier to push this in
> as three patches (1st DBCompat.pm, 2nd MySQL changes, 3rd PostgreSQL changes).
> 
> Of course, that doesn't mean that patch 2 can't modify patch 1, etc., but it'd
> just be nice to be able to push *something* in. :-)
> 
> If we could get DBCompat.pm in 2.18, that'd be pretty nice. (I can always
dream...)


Not sure how each this would be since most of the PostgreSQL changes require the
DBcompat.pm module to be there. But we could just include the DBcompat.pm module
with 2.18 once it is stabilized and then that way people can use it for new
enhancements as needed and then I can get the other changes in post 2.18. I plan
on putting the new PG changes into production here at RH once 2.18 is out so it
should get some good testing past that point. Also if we get the DBcompat.pm in
there before, it shouldnt cause any problems, would just be an extra module
laying around. Should I file a separate bug and attach DBcompat.pm to it or
something else similar already out there?
Comment 119 Max Kanat-Alexander 2004-03-17 18:15:29 PST
I think that bug 211769 is almost what we're looking for there, but I think that
DBCompat.pm should probably just get its own bug. File it, and ask for review &
approval, and we'll see if we can get it in for 2.18. I think that just the
existence of the file in Bugzilla should be easy to get approval for.
Comment 120 David Lawrence [:dkl] 2004-03-17 22:46:26 PST
(In reply to comment #119)
> I think that bug 211769 is almost what we're looking for there, but I think that
> DBCompat.pm should probably just get its own bug. File it, and ask for review &
> approval, and we'll see if we can get it in for 2.18. I think that just the
> existence of the file in Bugzilla should be easy to get approval for.

Created bug 237862 specifically for the DBcompat.pm module. Please put further
comments regarding that code in that report. Hopefully we can at least get
review going on that one for possible inclusion into next release.
Comment 121 David Lawrence [:dkl] 2004-03-19 09:31:42 PST
Adding bug 237862 to blocker list. I will submit a new patch to this bug that
has DBcompat.pm removed since it will be reviewed in the separate bug.
Comment 122 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-19 22:07:51 PST
:: sigh ::  :)
Comment 123 Max Kanat-Alexander 2004-03-26 17:48:30 PST
An interesting thing I just discovered on my PgSQL Bugzilla installation --

You must CREATE DATABASE bugs WITH ENCODING = 'SQL_ASCII' (which is the default
if you just type CREATE DATABASE bugs;). If you use 'UNICODE', Bugzilla will die
when you try to insert Unicode characters higher than 0x10000.

I thought I should note this here.
Comment 124 Bradley Baetz (:bbaetz) 2004-03-26 17:59:54 PST
Surely it only dies on invalid unicode bits? you'd want UTF-8 if we were going
to do this.

I have this feeling that postgre disables certain optimisations for non-ascii
dbs, though. I know if does if you have a non-default sort order.
Comment 125 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-26 18:08:18 PST
According to http://www.postgresql.com/docs/7.4/static/multibyte.html, UNICODE
for the charset given to Postgres means utf-8.
Comment 126 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-31 17:59:10 PST
If a miracle happens I'll be happy to change my mind, but I'm not gonna hold
2.18 for this ;)
Comment 127 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-09-18 18:36:48 PDT
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Comment 128 Jeroen Ruigrok van der Werven 2005-01-05 13:20:24 PST
So people.

Where are we standing with this? : )
Comment 129 Tomas Kopal 2005-01-10 15:22:17 PST
We(In reply to comment #128)
> So people.
> 
> Where are we standing with this? : )

Well... I have Bugzilla running on Postgres for a while now in production
environment, with just minor problems I'm just working on to resolve (e-mails
are case sensitive, full text search is broken etc.). For most of the other
changes, the patches are submitted, waiting for review and bit-rotting... Have
some time for reviews? ;-)
Comment 130 tmh 2005-02-13 04:35:31 PST
> Well... I have Bugzilla running on Postgres for a while now in production
> environment, with just minor problems I'm just working on to resolve (e-mails
> are case sensitive, full text search is broken etc.). For most of the other
> changes, the patches are submitted, waiting for review and bit-rotting... Have
> some time for reviews? ;-)

The patches seem to have bitrotted - does anyone have any thay apply against 2.18?
Comment 131 Tomas Kopal 2005-02-13 04:56:58 PST
(In reply to comment #130)
> 
> The patches seem to have bitrotted - does anyone have any thay apply against 2.18?
> 

If you mean patches at this bug, yes, they are very bitrotted, and also
obsolete. Look at the blockers of this bug, you will find newer. But do not
expect them to apply against 2.18, they are against the cvs tip. And they are
still not complete, if you think about geting bz runing on postgres...
Comment 132 Max Kanat-Alexander 2005-04-22 18:27:54 PDT
OK, the focus of this bug will now be to have *basic* PostgreSQL compatibility
for 2.20. That is, to the best of our ability as the Bugzilla developer and
testing community, we should have a version of Bugzilla that works on PostgreSQL.

Once bug 286695 is fixed, PostgreSQL support should be ready for general testing.
Comment 133 Max Kanat-Alexander 2005-04-27 04:44:40 PDT
[Also posted to the mozilla-webtools newsgroup and the developers list.]

        As of today's CVS version, Bugzilla's PostgreSQL support is
ready for beta testing. There are still some known bugs -- see the
blockers on this for details.

        If you encounter any bugs while using Bugzilla on PostgreSQL, please
report them to bugzilla.mozilla.org and make them block this bug. Also
CC me on them. 

        *Don't* add comments to this bug about problems you encounter. File a
new bug instead, per the above instructions.

        The steps for setting up a Bugzilla installation on PostgreSQL are very
simple:

        (0) Connect to the template1 database in PostgreSQL as a PostgreSQL
superuser. (That's usually the "postgres" user.)

        (1) Create a 'bugs' user:

        CREATE USER bugs PASSWORD 'password' CREATEDB;

        (2) Make sure that your pg_hba.conf allows md5 login from the local
machine, from the 'bugs' user:

        host all bugs 127.0.0.1 255.255.255.255 md5

        (3) Make sure that your postgresql.conf allows TCP/IP connections from
the local machine:

        tcpip_socket = true

        (4) Stop PostgreSQL, and then start it again.

        (5) Set up Bugzilla normally, editing localconfig and so forth per the
instructions in the Bugzilla Guide.

        Have fun!
Comment 134 Andrew Dunstan 2005-04-27 08:53:56 PDT
(In reply to comment #133)
> [Also posted to the mozilla-webtools newsgroup and the developers list.]
> 
>         As of today's CVS version, Bugzilla's PostgreSQL support is
> ready for beta testing. 

woohoo!!!


> 
>         (2) Make sure that your pg_hba.conf allows md5 login from the local
> machine, from the 'bugs' user:
> 
>         host all bugs 127.0.0.1 255.255.255.255 md5

Or from postgres 7.4 on:

          host all bugs 127.0.0.1/32 md5

> 
>         (3) Make sure that your postgresql.conf allows TCP/IP connections from
> the local machine:
> 
>         tcpip_socket = true

This setting disappeared in postgres version 8.0, which in any case listens to
localhost by default, unlike previous versions.

Comment 135 Max Kanat-Alexander 2005-08-31 01:10:01 PDT
As far as I'm concerned, this is actually done. There are a few features that
don't yet work on PostgreSQL (new charts and whining), but the major effort to
port Bugzilla to PostgreSQL is complete. All of the most-important features
(creating bugs, searching for bugs, and editing bugs) work very well, or at
least well-enough to use in a production environment. PostgreSQL support has
started to be considered a regular part of Bugzilla development, and within a
few months should be completely up to par with MySQL support.

Future bugs filed against PostgreSQL, instead of blocking this bug, should be
filed in the Database component and should have their summary start with
"[PostgreSQL]".

I'd like to thank everybody who ever helped work on this; this was one of the
most significant projects in Bugzilla's history, and now that it's in place,
future cross-DB work will be so much easier. :-)

Well done, everybody. :-)

Note You need to log in before you can comment on or make changes to this bug.