Another email, focusing on the tables/ directory.
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote:
> - Table files (rhnsat/tables/*)
> - <table>.sql files:
> - Most moved to common/tables/
> - Trigger DDL split out and moved into existing or new
> oracle/triggers/<table>.sql
> - *_triggers.sql files:
> - (git) moved to oracle/triggers/
> - _triggers suffix removed because redundant w/ directory scoping.
> - *_data.sql files:
> - Most (git) moved to common/data/
> - Few files forked and (git) moved to oracle/data. Forked files copied
> to
> postgres/data and ported to PG.
> - _data suffix removed because redundant w/ directory scoping.
> - *_index.sql files consolidated into file w/ table DDL. Most of the
> indexes
> were already there anyway.
This all sound like a sane to do for just Oracle anyway. Polishing the
schema sources is always good thing, so I propose doing the changes
in master where we can, one issue per commit, without waiting for
PostgreSQL-specific parts to be ready.
I see the following problems:
The rhn_ksscript_mod_trig trigger is still defined on wrong table, see
https://www.redhat.com/archives/spacewalk-devel/2008-September/msg00136.html
That would signal that no validation of the semantics of the code
was done. I'm very affraid that by doing large shuffles without having
some more formal validation in place, we will lose information and
potentially introduce many bugs. I also wonder if this would be a good
time to generate the
for each row
begin
:new.modified := sysdate;
end;
and other common types of triggers automatically.
There seem to be not only moves of table DDL tables but also changes
in the content. Things like
-create table
-web_user_site_type
+
+CREATE TABLE web_user_site_type
(
- type char(1)
- constraint wust_type_nn not null
- constraint wust_type_pk primary key,
- description varchar2(64)
- constraint wust_desc_nn not null
+ type CHAR(1) NOT NULL
+ CONSTRAINT wust_type_pk PRIMARY KEY,
+ description VARCHAR2(64) NOT NULL
)
- enable row movement
- ;
+ENABLE ROW MOVEMENT
+;
Why were these changes done? They do not seem to be necessary, it's
hard to see what is changing, and again, we potentially introduce
bugs.
The new code seems to have trailing spaces. Is that some kind of
format=flowed thing or is this a bug which should be addressed?
This particular patch also shows another issue -- the wust_type_nn
constraint is dropped and pure NOT NULL clause is used. Which means
that the cosntraint will have generated (SYS_*) name, which in turn
will cause freshly installed and upgraded schemas to differ, making
the validation hard(er) and more error-prone. Please do not remove
those names. Isn't chameleon's optimizer supposed to do just that in
build time?
I can see that you've renamed tables/Makefile.deps to
tables/tables.deps while also changing the indentation and introducing
some extra trailing spaces. Again, it makes it very hard to see what
has happened. I propose reverting the rename and/or doing the rename
in master in one commit, and then the indentation in another commit,
so that diff among the branches is possible.
Shouldn't we be able to generate the dependency tree of tables
automatically and render the tables/tables.deps file obsolete? With
triggers removed from tables' DDL files, parsing the foreign key
definitions shouldn't be that hard.
Why is content file rhnArchTypeActions.sql duplicated both in
oracle/data and in postgres/data when the file itself is the same? Why
is it not in common/data? The file rhnBlacklistObsoletes.sql has the
same problem. The rest of the files in oracle/data seems to only
differ from the postgres content in the sequence's nextval syntax.
Yet, many files in common/data have the Oracle syntax just fine, and
looking at chameleon's code, this should again be a build time issue.
I assume you want to review the database-specific data directories and
put most of the cotnent back to common/data.
--
Jan Pazdziora
Senior Software Engineer, Satellite Engineering, Red Hat
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
