As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 731156 - [Oracle] Adding or removing a DB column does not handle SERIAL correctly
: [Oracle] Adding or removing a DB column does not handle SERIAL correctly
Product: Bugzilla
Classification: Server Software
Component: Database (show other bugs)
: 4.2
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: Frédéric Buclin
: default-qa
Depends on:
  Show dependency treegraph
Reported: 2012-02-28 02:26 PST by David Taylor
Modified: 2012-08-29 15:48 PDT (History)
2 users (show)
LpSolit: approval+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

patch, v1 (6.20 KB, patch)
2012-08-19 17:58 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2 (6.80 KB, patch)
2012-08-29 14:57 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review

Description User image David Taylor 2012-02-28 02:26:14 PST
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

Upgrade from Bugzilla 4.0 to 4.2

Actual results:

The new column was created without the Oracle-compatibility sequence & trigger necessary for SERIAL-type columns.

Expected results:

DB/Schema/ should have created a BUG_SEE_ALSO_ID_SEQ sequence and BUG_SEE_ALSO_ID_TR trigger to automatically populate the BUG_SEE_ALSO.ID field when inserting new records.
Comment 1 User image David Taylor 2012-02-28 02:33:02 PST
The obvious solution is to override DB/Schema/ to check if we are creating a new column with a SERIAL type, and call _get_create_seq_ddl($table, $field_name) as done elsewhere.

However, as the broken code has now been released, I'm not sure how to create a backward-compatible change to Install/ which will work for all Oracle users:

- Upgrading from 4.0 -> 4.2.1
- Upgrading from 4.0 -> 4.2.0 -> 4.2.1; or
- Installing 4.2.0 then upgrading to 4.2.1; or
- Installing 4.2.1
Comment 2 User image Frédéric Buclin 2012-08-19 11:44:39 PDT
I try to remember how to list sequences and triggers in Oracle...
Comment 3 User image Frédéric Buclin 2012-08-19 11:55:23 PDT
SELECT sequence_name FROM all_sequences WHERE sequence_owner = 'BUGS' ORDER BY sequence_name;
SELECT trigger_name  FROM all_triggers  WHERE owner          = 'BUGS' ORDER BY trigger_name;
Comment 4 User image Frédéric Buclin 2012-08-19 14:46:25 PDT
Also, deleting such a column doesn't remove its sequence and trigger.

About the column, we will have to delete and recreate the column, unless it's possible to add the sequence and trigger after the column has already been created and populated.
Comment 5 User image Frédéric Buclin 2012-08-19 17:58:59 PDT
Created attachment 653233 [details] [diff] [review]
patch, v1

This patch fixes several things:
- It adds missing sequences and triggers to existing primary keys which lack them.
- It fixes get_add_column_ddl() and get_drop_column_ddl() to correctly add/remove these sequences and triggers.
- Various cleanup, such as fixing the indentation and moving get_drop_column_ddl() at its right place.
Comment 6 User image Frédéric Buclin 2012-08-20 15:55:59 PDT
David: could you apply my patch to your 4.2.x installation and tell me if this correctly fixes the problem for you?

If you could test some of your testcases mentioned in your comment 1, this would be great! :)
Comment 7 User image David Lawrence [:dkl] 2012-08-28 20:42:17 PDT
I am still not able to get this work properly when the bug_see_also table has rows of data before upgrading from 4.0 to 4.2. I get the following error:

Adding new column 'id' to the 'bug_see_also' table...
DBD::Oracle::db do failed: ORA-01758: table must be empty to add mandatory (NOT NULL) column (DBD ERROR: error possibly near <*> indicator at char 12 in 'ALTER TABLE <*>bug_see_also ADD id integer PRIMARY KEY') [for Statement "ALTER TABLE bug_see_also ADD id integer PRIMARY KEY"] at Bugzilla/DB/ line 401
	Bugzilla::DB::Oracle::do(undef, 'ALTER TABLE bug_see_also ADD id integer PRIMARY KEY') called at Bugzilla/ line 606
	Bugzilla::DB::bz_add_column('Bugzilla::DB::Oracle=HASH(0x54f5ea0)', 'bug_see_also', 'id', 'HASH(0x7e06290)') called at Bugzilla/Install/ line 647
	Bugzilla::Install::DB::update_table_definitions('HASH(0x17098d0)') called at ./ line 199

According to the Oracle docs, it is not possible to add a NOT NULL column without setting a default value.

Or by first creating the column as NULL, updating all rows to a default value and then adding a NOT NULL constraint to the column. A three step process.

What if when adding a SERIAL type column, we add a default value of 0 automatically, then we can add the sequence and trigger after and update the column with proper incrementing values?

How has this ever worked before when upgrading table definitions with Oracle from one BZ version to another? Also this issue is unrelated to the patch under review as I get the error with or without the patch when updating the DB from 4.0 to 4.2.

Comment 8 User image Frédéric Buclin 2012-08-29 14:57:26 PDT
Created attachment 656627 [details] [diff] [review]
patch, v2

Fixes the problem dkl found while upgrading with an already populated table.
Comment 9 User image David Lawrence [:dkl] 2012-08-29 15:40:01 PDT
Comment on attachment 656627 [details] [diff] [review]
patch, v2

Review of attachment 656627 [details] [diff] [review]:

Works as expected. r=dkl
Comment 10 User image Frédéric Buclin 2012-08-29 15:48:00 PDT
Committing to: bzr+ssh://
modified Bugzilla/DB/
modified Bugzilla/DB/Schema/
Committed revision 8367.

Committing to: bzr+ssh://
modified Bugzilla/DB/
modified Bugzilla/DB/Schema/
Committed revision 8130.

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