[Spacewalk-devel] pgsql review
All, I'd like to start getting some eyes on the pgsql branch in preparation of a merge to master. So far, the changes on this branch have been focused on porting the schema and updating the application infrastructure to work with both oracle and postgres databases. None of the queries have been ported/forked yet. I'm not implying that the branch is ready to merge to master as is. Rather, that it is ready for review and I'd like everyone's help to get it to where we can merge it. Here is a summary of the changes to look for (as best I can): SCHEMA DIRECTORY The goal here was to push as much of the schema into common as practical and make the schema code tree as developer friendly as possible. - The schema directory has been refactored replacing rhnsat/ with: oracle/ postgres/. - Forked (cannot be common) items: rhnsat/class/ rhnsat/types/ rhnsat/procs/ rhnsat/packages/ (git) moved to oracle/ and copied to postgres/ and ported to PG. - Table files (rhnsat/tables/*) - .sql files: - Most moved to common/tables/ - Trigger DDL split out and moved into existing or new oracle/triggers/.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. - View files: - Most view (.sql) files were (git) moved to common/views. This involved some standardizing of the view SQL but most were unchanged. - Few view (.sql) files were forked: - (git) moved to oracle/views/ - Copied to postgres/views and ported to PG. - Upgrades (upgrade/* directories & files): - Most files (git) moved to common/upgrade/ - Few files forked: (git) moved to oracle/upgrade/. - Few files forked: copied to postgres/upgrade/ and ported to PG. - Upgrade scripts containing updated things created as CREATE OR REPLACE like views procs and packages updated replacing cut-n-paste content that is redundant with install files with an include to the install (.sql) file. See examples in git tree. - So far, git only has 0.4 -> 0.5 (example) upgrade scripts, rest to follow. COMMON --> DB SPECIFIC At build, common (common/) files are transformed by a build tool named "chameleon" into DB specific files. For example: # # cd oracle # chameleon -s oracle -o tables/common/x.sql ../common/tables/x.sql # Long term, chameleon will be a Fedora project and included in Fedora distribution. For now, it is available on the wiki: https://fedorahosted.org/spacewalk/wiki/PostgreSqlProject#Attachments. STYLE You may notice that the common/tables/*.sql were formatted using chameleon. Consistent with most DDL/SQL style guides, chameleon renders keywords in caps and uses spaces instead of tabs. If you don't like this style, don't freak out. We take a vote and I can easily re-run the files through chameleon and reformat to any style we like :-) MAKEFILES The Makefile.schema refactored into a /regular/ makefile. Dependency sorting and .sql file aggregation split out into a build tool named "blend". Although, make does do dependency sorting with the .deps files, the Makefile.schema (although, a clever application of the technology) seems to exceed make's intended use and was difficult to maintain and debug. Blend also provides analysis of .deps files and reports unused rules, unfound references, duplicate files and circular dependencies. Anyway, if someone really believes we should go back to using make for this, I suppose we can. PACKAGING The spacewalk-schema.spec was updated to package and install install / upgrade scripts for both oracle and postgres. The files are installed in: /etc/sysconif/rhn/oracle/ main.sql spacewalk_0.4-spacewalk_0.5.sql ... /etc/sysconif/rhn/postgres/ main.sql spacewalk_0.4-spacewalk_0.5.sql ... As you can see, the upgrade scripts are aggregated using blend just like the install scripts. Spacewalk::Setup and the spacewalk-schema-upgrade scripts need to be updated to support the new directory structure and files names. This work has been started but still needs more work. ** Milan, can you help with changes to spacewalk-schema-upgrade? Also, anyone know what the links are for in the upgrade directories? Do we need them? Can they be created during RPM install? INSTALL / SETUP - Installer - Isolated more Oracle specific parts of spacewalk-setup. - Added concept of specifying a database "backend" to use. - Changes to support both Oracle/PostgreSQL db connection strings. - Changes to support schema installation for both db's from new schema location. - rhn.conf
[Spacewalk-devel] pgsql review
All, The outcome of the meeting yesterday regarding the pgsql branch review is as follows: After lengthy discussion it was decided (almost unanimously by attendance) that the work on the pgsql branch would be merged to master without refactoring the commits. There was general agreement that the concerns and objections raised about doing this had a lot of merit. However, they did not justify the effort required to re-implement work that has already been completed. Pending resolution of the following content related comments and assuming no new issues are raised, the branch will be merged to master. Merge vs cherry pick, not sure which is best. Thoughts anyone? Content related concerns to be addressed before merging pgsql to master: * Trailing white space needs to be removed. I will trim the trailing white spaces. * Packaging of upgrade scripts needs to be resolved. I committed a change to pgsql branch that will *undo* the proposed changes to the packaging and installation of the upgrade scripts. They will be installed in same place and have the same form as before. This means that initially, upgrades will only be for oracle installs. After the dust settles on the merge to master we will revisit the upgrades. * Replacing the named NOT NULL constraints in the common tables (.sql) files with simple NOT NULL keywords. - Concern about the upgrade scripts. Using "ALTER TABLE MODIFY NULL;" instead of: "ALTER TABLE DROP CONSTRAINT ;" to make a column nullable in all future upgrade scripts will handle this. - The change causes problems in the upgrade verification scripts because the named NOT NULL constraints will be squawked as missing. Unfortunately, when looking at user_constraints, the constraint_type=R is for both CHECK constraints and NOT NULL constraints. But, the search_condition can easily be used to ignore NOT NULL constraints. Validation that the column is NOT NULL, can be done by matching the nullable column in user_tab_columns when validating a table's columns. I believe the effort to tweak the upgrade validation scripts will be much less then reworking the common tables (.sql) files to restore the named NOT NULL constraints. * Did I miss anything? Testing before merge to master: * Get a clean validation report from the upgrade validation script(s) to validate the equivalence between a DB created from master install and a DB created installed from pgsql. * Run automated testing on spacewalk installed from pgsql. Can this be done? * Other suggestions? ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Tue, 12 May 2009 12:42:32 -0400 Jeff Ortel wrote: > All, > > I'd like to start getting some eyes on the pgsql branch in > preparation of a merge to master. So far, the changes on this branch > have been focused on porting the schema and updating the application > infrastructure to work with both oracle and postgres databases. None > of the queries have been ported/forked yet. I'm not implying that > the branch is ready to merge to master as is. Rather, that it is > ready for review and I'd like everyone's help to get it to where we > can merge it. > > Here is a summary of the changes to look for (as best I can): > > SCHEMA DIRECTORY > > The goal here was to push as much of the schema into common as > practical and make the schema code tree as developer friendly as > possible. > > - The schema directory has been refactored replacing > rhnsat/ >with: > oracle/ > postgres/. > - Forked (cannot be common) items: > rhnsat/class/ > rhnsat/types/ > rhnsat/procs/ > rhnsat/packages/ >(git) moved to oracle/ and copied to postgres/ and ported to PG. > - Table files (rhnsat/tables/*) >- .sql files: > - Most moved to common/tables/ > - Trigger DDL split out and moved into existing or new > oracle/triggers/.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. > - View files: >- Most view (.sql) files were (git) moved to common/views. This > involved some standardizing of the view SQL but most were unchanged. >- Few view (.sql) files were forked: > - (git) moved to oracle/views/ > - Copied to postgres/views and ported to PG. > - Upgrades (upgrade/* directories & files): >- Most files (git) moved to common/upgrade/ >- Few files forked: (git) moved to oracle/upgrade/. >- Few files forked: copied to postgres/upgrade/ and > ported to PG. >- Upgrade scripts containing updated things created as CREATE OR > REPLACE like views procs and packages updated replacing cut-n-paste > content that is redundant with install files with an include to the > install (.sql) file. See examples in git tree. >- So far, git only has 0.4 -> 0.5 (example) upgrade scripts, rest > to follow. > > COMMON --> DB SPECIFIC > > At build, common (common/) files are transformed by a build tool > named "chameleon" into DB specific files. For example: > # > # cd oracle > # chameleon -s oracle -o tables/common/x.sql ../common/tables/x.sql > # > Long term, chameleon will be a Fedora project and included in Fedora > distribution. For now, it is available on the wiki: > https://fedorahosted.org/spacewalk/wiki/PostgreSqlProject#Attachments. > > STYLE > > You may notice that the common/tables/*.sql were formatted using > chameleon. Consistent with most DDL/SQL style guides, chameleon > renders keywords in caps and uses spaces instead of tabs. If you > don't like this style, don't freak out. We take a vote and I can > easily re-run the files through chameleon and reformat to any style > we like :-) > > MAKEFILES > > The Makefile.schema refactored into a /regular/ makefile. Dependency > sorting and .sql file aggregation split out into a build tool named > "blend". Although, make does do dependency sorting with the .deps > files, the Makefile.schema (although, a clever application of the > technology) seems to exceed make's intended use and was difficult to > maintain and debug. Blend also provides analysis of .deps files and > reports unused rules, unfound references, duplicate files and > circular dependencies. Anyway, if someone really believes we should > go back to using make for this, I suppose we can. > > PACKAGING > > The spacewalk-schema.spec was updated to package and install > install / upgrade scripts for both oracle and postgres. The files > are installed in: > > /etc/sysconif/rhn/oracle/ >main.sql >spacewalk_0.4-spacewalk_0.5.sql >... > > /etc/sysconif/rhn/postgres/ >main.sql >spacewalk_0.4-spacewalk_0.5.sql >... > > As you can see, the upgrade scripts are aggregated using blend just > like the install scripts. Spacewalk::Setup and the > spacewalk-schema-upgrade scripts need to be updated to support the > new directory structure and files names. This work has been started > but still needs more work. ** Milan, can you help with changes to > spacewalk-schema-upgrade? Also, anyone know what the links are for > in the upgrade directories? Do we need them? Can they be created > during RPM in
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > All, > > I'd like to start getting some eyes on the pgsql branch in preparation of a > merge to > master. So far, the changes on this branch have been focused on porting the > schema and > updating the application infrastructure to work with both oracle and postgres > databases. This might be a technicality but: do we want to merge or cherry pick? If we want to start with infrastructure work, I assume we will want to bring in the changes to master in small steps, addressing one issue at a time (SQL standard compliance, table / trigger DDL split, etc.). That way, the changes will have a chance to get verified and tested in Oracle environment while the diff against the pgsql branch will get smaller. I would not want to see huge merge commit landing in master. (I will probably send more responses to your post, commenting on one issue at a time.) -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > The goal here was to push as much of the schema into common as practical and > make > the schema code tree as developer friendly as possible. > > - The schema directory has been refactored replacing > rhnsat/ > with: > oracle/ > postgres/. > - Forked (cannot be common) items: > rhnsat/class/ > rhnsat/types/ > rhnsat/procs/ > rhnsat/packages/ > (git) moved to oracle/ and copied to postgres/ and ported to PG. It looks like you've renamed the rhnsat/ directory to common/. That however makes it very hard to do a diff against master to see what has changed. Do you plan to rename rhnsat/ to common/ any time soon in master (or temporarily rename it back in pgsql branch) so that diffing the changes is easier? -- 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
Re: [Spacewalk-devel] pgsql review
Another email, focusing on the tables/ directory. On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > - Table files (rhnsat/tables/*) > - .sql files: > - Most moved to common/tables/ > - Trigger DDL split out and moved into existing or new > oracle/triggers/.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 ( - typechar(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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > - View files: > - Most view (.sql) files were (git) moved to common/views. This involved > some standardizing of the view SQL but most were unchanged. Can you be more specific about the changes which were done? There are some indentation changes (which might have been caused by the changes that went to master in the meantime) which might not be needed, plus trailing spaces (for example views/rhnAvailableChannels.sql). Also, there are changes for the AS aliasing. I wonder if we could easily drop the alias from Oracle (and pgsql) in cases when the alias name is the same as column name -- for example create or replace view rhnChannelFamilyOverview as select pcf.org_id org_id, f.idid, f.name name, That way, the diff which will introduce the AS will again be smaller. We also have views that have the column names specified right after the view name: create or replace view rhnAvailableChannels ( org_id, channel_id, channel_depth, channel_name, channel_arch_id, padded_name, current_members, available_members, last_modified, channel_label, parent_or_self_label, parent_or_self_id ) as I wonder if this is the good time, regardless of the PostgreSQL port state, to either adopt this style or drop it, for consistency reasons (it should make writing validation parters for the schema easier as well). > - Few view (.sql) files were forked: > - (git) moved to oracle/views/ > - Copied to postgres/views and ported to PG. The views/rhnHistoryView.sql file seems to still contain definition of rhnHistoryView_pkglist function. Is that correct? If we are touching the file, we should really make sure anything which should not be there is moved someplace else. -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > - Upgrades (upgrade/* directories & files): > - Most files (git) moved to common/upgrade/ > - Few files forked: (git) moved to oracle/upgrade/. > - Few files forked: copied to postgres/upgrade/ and ported to PG. There does not seem to be anything changed there, apart from the Makefile. Please put the files to common/upgrade if no changes are needed / intended. > - Upgrade scripts containing updated things created as CREATE OR REPLACE > like views > procs and packages updated replacing cut-n-paste content that is > redundant with > install files with an include to the install (.sql) file. See examples > in git tree. > - So far, git only has 0.4 -> 0.5 (example) upgrade scripts, rest to follow. For the view, function and package definition, I'd very much like to see us move towards specifying file (we will hopefully get rid of the symlinks) which should be executed, rather than duplicating the content in the upgrade directories. -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > At build, common (common/) files are transformed by a build tool named > "chameleon" into DB > specific files. For example: > # > # cd oracle > # chameleon -s oracle -o tables/common/x.sql ../common/tables/x.sql > # > Long term, chameleon will be a Fedora project and included in Fedora > distribution. > For now, it is available on the wiki: > https://fedorahosted.org/spacewalk/wiki/PostgreSqlProject#Attachments. I wonder what is wrong with Makefiles and sed or some other templating / macro packages that already are in our distribution. Inventing new package (which will become a BuildRequires dependency) for such a simple task seems to be unnecessary. It also makes the learning / adoption curve more steep. Can you provide some reasoning what the target features of the tool should be and why they cannot be put together with existing tools? -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > STYLE > > You may notice that the common/tables/*.sql were formatted using chameleon. > Consistent with most DDL/SQL style guides, chameleon renders keywords in caps > and > uses spaces instead of tabs. If you don't like this style, don't freak out. > We > take a vote and I can easily re-run the files through chameleon and reformat > to > any style we like :-) I'm against any reformating which is not supported by semantic validation reasons, for reasons stated in previous posts -- it makes the diff against master bigger and hides things from the naked eye. We should not change the source code in the branch just because we can or just because we have a tool to do that. Note: I don't really care about the actual format, so I'm not voting against the format per se (apart from the trailing spaces it seems to introduce). We can put together some coding guidelines and use them for new code. -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > MAKEFILES > > The Makefile.schema refactored into a /regular/ makefile. Dependency sorting > and .sql > file aggregation split out into a build tool named "blend". Although, make > does do > dependency sorting with the .deps files, the Makefile.schema (although, a > clever > application of the technology) seems to exceed make's intended use and was > difficult to > maintain and debug. Blend also provides analysis of .deps files and reports > unused > rules, unfound references, duplicate files and circular dependencies. > Anyway, if someone > really believes we should go back to using make for this, I suppose we can. I'm confused. What does blend really do and how does it fit into the build chain? There still seem to be files like ./oracle/views/views.deps ./oracle/tables/tables.deps ./oracle/packages/packages.deps What are they for? And they generated? If they are, why do we have them in our sources in the first place? If they are not geenrated, what does blend really do what make could not? I wonder if we should (again, independently from the PostgreSQL port effort) focus on making the dependency resolution more automatic, rather than rewrite working thing with another thing. The types can be collated mostly automatically -- tables before triggers, types before tables, functions before triggers but after tables, etc. For the procedural code, we can recompile the whole schema anyway. For tables, we could generate the the graph / Makefile just by grepping the source code. For other types, I wonder if we could put the dependency information to the start of the SQL file, something like -- Copyright (c) 2008 Red Hat, Inc. -- -- This software is licensed to you under the GNU General Public License, -- [...] -- Requires: procs/lookup_cf_state create or replace function lookup_first_matching_cf ( [...] By having the dependency spelled out in the source code where the object is defined rather than in some separate long file, we would make it easier to maintain and keep up-to-date. I'd consider this a higher priority than any decision about make replacement. Maybe, when we do something about the dependencies in general, we will find out that any issues you might have hit with make are either gone or much more manageable. -- 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
Re: [Spacewalk-devel] pgsql review
On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > PACKAGING > > The spacewalk-schema.spec was updated to package and install install / > upgrade scripts for > both oracle and postgres. The files are installed in: > > /etc/sysconif/rhn/oracle/ > main.sql > spacewalk_0.4-spacewalk_0.5.sql > ... > > /etc/sysconif/rhn/postgres/ > main.sql > spacewalk_0.4-spacewalk_0.5.sql > ... > > As you can see, the upgrade scripts are aggregated using blend just like the > install > scripts. Spacewalk::Setup and the spacewalk-schema-upgrade scripts need to > be updated to > support the new directory structure and files names. This work has been > started but still > needs more work. ** Milan, can you help with changes to > spacewalk-schema-upgrade? Also, > anyone know what the links are for in the upgrade directories? Do we need > them? Can they > be created during RPM install? You cannot aggregate upgrade scripts because other products (like Satellite) might want to address individual upgrade scripts (via symlinks, or, work-in-progress, via references). There is a reason we kept the upgrade scripts separated. As a matter of fact, I think that if would be good to have the main schema definition (universe.satellite.sql, or main.sql) split into individual source files as well -- basically just pack the source files and concatenate them at install time. The reason is that it would make schema overrides by Satellite schema package easier, and upgrade scripts could just reference file name of new package or function, rather than having the source code duplicated and potentially out of sync. So here I feel the move has been in the wrong direction. -- 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
Re: [Spacewalk-devel] pgsql review
Thanks for you comments! Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: All, I'd like to start getting some eyes on the pgsql branch in preparation of a merge to master. So far, the changes on this branch have been focused on porting the schema and updating the application infrastructure to work with both oracle and postgres databases. This might be a technicality but: do we want to merge or cherry pick? If we want to start with infrastructure work, I assume we will want to bring in the changes to master in small steps, addressing one issue at a time (SQL standard compliance, table / trigger DDL split, etc.). That way, the changes will have a chance to get verified and tested in Oracle environment while the diff against the pgsql branch will get smaller. Yes, this would be nice. I tried to keep the commits well grouped, it was a massive amount of work and the commits were not necessarily grouped to be cherry picked. With that said, we can cheery pick commits pertaining to schema/ re-factoring separately from code changes. Given the scope of the changes it might be more conclusive to verify the schema changes by querying (and comparing) the oracle (DDL) tables of two side-by-side oracle installs. One installed from master and the other installed from pgsql. If they are deemed equivalent, couldn't we deem the sources to be equivalent? The alternative would be to manually re-implement most of the re-factoring work in another branch and grouped into commits that can be reviewed and cherry picked into master. I have no problem doing this but seem a waste to through away work that has already been done and management would have to sign off on the additional effort. But, if the team believes that this is the way to go, then ... I would not want to see huge merge commit landing in master. I understand your concern. (I will probably send more responses to your post, commenting on one issue at a time.) ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: The goal here was to push as much of the schema into common as practical and make the schema code tree as developer friendly as possible. - The schema directory has been refactored replacing rhnsat/ with: oracle/ postgres/. - Forked (cannot be common) items: rhnsat/class/ rhnsat/types/ rhnsat/procs/ rhnsat/packages/ (git) moved to oracle/ and copied to postgres/ and ported to PG. It looks like you've renamed the rhnsat/ directory to common/. No, common already existed. Most of the table files were (git) moved from rhnsat/tables to common/tables. Also, most of the views were (git) moved from rhnsat/views to common/views. All of the other content was (git) moved to oracle. Once the rhnsat was empty, it was removed. That however makes it very hard to do a diff against master to see what has changed. Do you plan to rename rhnsat/ to common/ any time soon in master (or temporarily rename it back in pgsql branch) so that diffing the changes is easier? I'm all about making it easier to see the changes via diff but I'm not sure how. The changes are massive enough that git is getting very confused. I'm not sure if renaming common -> rhnsat would work. The common directory already exists on master. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: Another email, focusing on the tables/ directory. On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: - Table files (rhnsat/tables/*) - .sql files: - Most moved to common/tables/ - Trigger DDL split out and moved into existing or new oracle/triggers/.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 thought so too :) 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. The focus during the refactoring was to ensure that the end schema was exactly the same as the original. This kind of proves that. 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'm planning on doing a side-by-side comparison of (2) oracle databases; one installed from master and the other from pgsql. We should be able to verify that no information was lost. 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. Might be better as a follow on. 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 ( - typechar(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. Yes, as I mentioned in the summary, the common table files were reformatted. I agree this was not necessary. But, I decided to use chameleon to help automate refactoring the files and it renders formatted output. Also, the style and formatting was a hodge-podge and kind of needed it. 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? Seems to load into oracle and postgres just fine. Can you be more specific about why this is a problem? 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? Yes, but having named NOT NULL constraints is terribly unnecessary and only clutters up the schema. As far as upgrades, I considered this. In oracle, a table created with a named not null constraint can be altered as (alter table person modify age null) to remove the not null constraint on the 'age' column. Keeping this in mind, this should not be a problem. 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. Since these files are now used by 'blend' and not Make, I renamed to avoid confusion as to what they are for. Also, the formatting in these files was terrible and after working with them for weeks, I just had to fix them. I was very careful to keep them correct and the schema loads just find. Again, it makes it very hard to see what has happened. I understand. 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 th
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: - View files: - Most view (.sql) files were (git) moved to common/views. This involved some standardizing of the view SQL but most were unchanged. Can you be more specific about the changes which were done? There are some indentation changes (which might have been caused by the changes that went to master in the meantime) which might not be needed, plus trailing spaces (for example views/rhnAvailableChannels.sql). This would be good to diff. They were only changed to the extent necessary to work for both oracle and postgres (as far as I know). Also, there are changes for the AS aliasing. I wonder if we could easily drop the alias from Oracle (and pgsql) in cases when the alias name is the same as column name -- for example create or replace view rhnChannelFamilyOverview as select pcf.org_id org_id, f.idid, f.name name, That way, the diff which will introduce the AS will again be smaller. Sure, so long as the DDL continues to work with both oracle and pg. We also have views that have the column names specified right after the view name: create or replace view rhnAvailableChannels ( org_id, channel_id, channel_depth, channel_name, channel_arch_id, padded_name, current_members, available_members, last_modified, channel_label, parent_or_self_label, parent_or_self_id ) as I wonder if this is the good time, regardless of the PostgreSQL port state, to either adopt this style or drop it, for consistency reasons (it should make writing validation parters for the schema easier as well). Sure. I'm very much in favor of standardizing. - Few view (.sql) files were forked: - (git) moved to oracle/views/ - Copied to postgres/views and ported to PG. The views/rhnHistoryView.sql file seems to still contain definition of rhnHistoryView_pkglist function. Is that correct? Hmm... didn't expect to find function definitions in a view file so I didn't look. I agree this function should be split out and the view.deps updated. If we are touching the file, we should really make sure anything which should not be there is moved someplace else. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: - Upgrades (upgrade/* directories & files): - Most files (git) moved to common/upgrade/ - Few files forked: (git) moved to oracle/upgrade/. - Few files forked: copied to postgres/upgrade/ and ported to PG. There does not seem to be anything changed there, apart from the Makefile. Please put the files to common/upgrade if no changes are needed / intended. The forked files are not changed but contain syntax that cannot be common (at this time). - Upgrade scripts containing updated things created as CREATE OR REPLACE like views procs and packages updated replacing cut-n-paste content that is redundant with install files with an include to the install (.sql) file. See examples in git tree. - So far, git only has 0.4 -> 0.5 (example) upgrade scripts, rest to follow. For the view, function and package definition, I'd very much like to see us move towards specifying file (we will hopefully get rid of the symlinks) which should be executed, rather than duplicating the content in the upgrade directories. You're agreeing with my approach, right? ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: STYLE You may notice that the common/tables/*.sql were formatted using chameleon. Consistent with most DDL/SQL style guides, chameleon renders keywords in caps and uses spaces instead of tabs. If you don't like this style, don't freak out. We take a vote and I can easily re-run the files through chameleon and reformat to any style we like :-) I'm against any reformating which is not supported by semantic validation reasons, for reasons stated in previous posts -- it makes the diff against master bigger and hides things from the naked eye. We should not change the source code in the branch just because we can or just because we have a tool to do that. Note: I don't really care about the actual format, so I'm not voting against the format per se (apart from the trailing spaces it seems to introduce). We can put together some coding guidelines and use them for new code. I'm for the reformatting (our SQL is pretty inconsistent), though I'm not sure if now is necessarily the best time. On one hand, I like the idea of doing such sweeping changes all at once; it'll lead to the codebase settling into its own sooner. On the other, since so much is going on there's a fair risk something will be broken, so having easier diffs while we shake out all the initial bugs might make life easier. If it's still an option, I think my vote is to hold off on the reformatting until after things have settled down a bit. I'm ok with eventually having a big hiccup in diffs, but I think it's going to complicate the initial pgsql bugs. -- Jason Dobies RHN Satellite / Spacewalk RHCE# 805008743336126 Freenode: jdob @ #spacewalk #spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: At build, common (common/) files are transformed by a build tool named "chameleon" into DB specific files. For example: # # cd oracle # chameleon -s oracle -o tables/common/x.sql ../common/tables/x.sql # Long term, chameleon will be a Fedora project and included in Fedora distribution. For now, it is available on the wiki: https://fedorahosted.org/spacewalk/wiki/PostgreSqlProject#Attachments. I wonder what is wrong with Makefiles and sed or some other templating / macro packages that already are in our distribution. Inventing new package (which will become a BuildRequires dependency) for such a simple task seems to be unnecessary. It also makes the learning / adoption curve more steep. Can you provide some reasoning what the target features of the tool should be and why they cannot be put together with existing tools? I knew this question would come up at least once :) And, it's a very good one. I considered these approaches (in consult with others) and in some ways they appear to be simpler. But, after considering the breadth and the complexity of the whole problem, I believe a cobbled up solution would suffer from a number of limitations and would end up being far more complex in the end. On the other hand, chameleon is already developed, straight forward and has very few limitations. Using a grammar based parser like chameleon means that the common schema can be expressed using an already well know syntax. The chameleon 'common' grammar is a superset of oracle and (some optional) postgres grammars so developers who know oracle DDL can work in the schema as they do today. It handles differences in syntax and grammar between oracle and postgres, as well as, any future databases (mysql?). The precision and extensibility is unmatched by cobbled up solutions because it parses the DDL into a data model and uses a plug-in architecture to render the DB specific DDL. A templating approach would be my 2nd choice and has grammar independence in its favor. But, it also has the following limitations: - Does not handle differences in grammar. - The common schema would have to be retrofitted with tags to replace such as something like (or whatever): $(TABLESPACE) and $(USING_INDEX_TABLESPACE) and $(NUMBER) $(SYSDATE) ... which would place a learning burden on developers. They would have to learn all the tags and when/where they must/can be used. - Some kind of tool (script) would still have to be developed/maintained. - Would require complex tags to handle more complex differences such as: sequence nextval syntax. A sed/awk like approach has the following limitations: - Does not handle differences in grammar. - Is difficult to make context sensitive. For example, something as simple as replacing the NUMBER oracle data type with NUMERIC for postgres would potentially find-and-replace in other places then in the column data type as intended. NUMBER could appear in a CHECK constraint, a column name and inside an number of strings. Because the necessary precision would be difficult (maybe impossible) to achieve, there would be a greater risk of a non-deterministic outcome. - Some kind of tool (script) would still have to be developed/maintained. There is probably a clever way to combine a number of available tools to cobble up something. But, I believe that, in the end, the result would be a very complicated and temperamental solution that would be a nightmare to maintain. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: STYLE You may notice that the common/tables/*.sql were formatted using chameleon. Consistent with most DDL/SQL style guides, chameleon renders keywords in caps and uses spaces instead of tabs. If you don't like this style, don't freak out. We take a vote and I can easily re-run the files through chameleon and reformat to any style we like :-) I'm against any reformating which is not supported by semantic validation reasons, for reasons stated in previous posts -- it makes the diff against master bigger and hides things from the naked eye. We should not change the source code in the branch just because we can or just because we have a tool to do that. Noted. Note: I don't really care about the actual format, so I'm not voting against the format per se (apart from the trailing spaces it seems to introduce). We can put together some coding guidelines and use them for new code. What's the problem with trailing spaces? ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: MAKEFILES The Makefile.schema refactored into a /regular/ makefile. Dependency sorting and .sql file aggregation split out into a build tool named "blend". Although, make does do dependency sorting with the .deps files, the Makefile.schema (although, a clever application of the technology) seems to exceed make's intended use and was difficult to maintain and debug. Blend also provides analysis of .deps files and reports unused rules, unfound references, duplicate files and circular dependencies. Anyway, if someone really believes we should go back to using make for this, I suppose we can. I'm confused. What does blend really do and how does it fit into the build chain? There still seem to be files like ./oracle/views/views.deps ./oracle/tables/tables.deps ./oracle/packages/packages.deps What are they for? To define dependencies as before. And they generated? No. They (.deps) provide dependencies between views, procs, packages, types, etc that cannot be generated. The table dependencies could be generated by chameleon using the FK constraints but that's all. The complexity of the dependencies goes far beyond the tables. If they are, why do we have them in our sources in the first place? If they are not geenrated, what does blend really do what make could not? This one is very subjective :) The only things that blend can do that make cannot (as far as I can tell): 1) Analyse and report on the quality of the .deps files. 2) Replace (@) references in the upgrade scripts with the content of the referenced file. Aside from that, it really has to do with what make *can* do and what make *should* do. After mucking with the Makefile.schema for a while, I decided to prototype an alternative and came up with blend. I wonder if we should (again, independently from the PostgreSQL port effort) focus on making the dependency resolution more automatic, rather than rewrite working thing with another thing. The types can be collated mostly automatically -- tables before triggers, types before tables, functions before triggers but after tables, etc. For the procedural code, we can recompile the whole schema anyway. It's not that simple. Some of the dependencies are more intertwined that this. For example: the EVR_T has a dependency on the rhn_rpm package. Views depend on tables and packages which depend of other tables, views and functions. And on, and on. For tables, we could generate the the graph / Makefile just by grepping the source code. For other types, I wonder if we could put the dependency information to the start of the SQL file, something like -- Copyright (c) 2008 Red Hat, Inc. -- -- This software is licensed to you under the GNU General Public License, -- [...] -- Requires: procs/lookup_cf_state create or replace function lookup_first_matching_cf ( [...] By having the dependency spelled out in the source code where the object is defined rather than in some separate long file, we would make it easier to maintain and keep up-to-date. Yes, good idea. I'd consider this a higher priority than any decision about make replacement. Maybe, when we do something about the dependencies in general, we will find out that any issues you might have hit with make are either gone or much more manageable. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: PACKAGING The spacewalk-schema.spec was updated to package and install install / upgrade scripts for both oracle and postgres. The files are installed in: /etc/sysconif/rhn/oracle/ main.sql spacewalk_0.4-spacewalk_0.5.sql ... /etc/sysconif/rhn/postgres/ main.sql spacewalk_0.4-spacewalk_0.5.sql ... As you can see, the upgrade scripts are aggregated using blend just like the install scripts. Spacewalk::Setup and the spacewalk-schema-upgrade scripts need to be updated to support the new directory structure and files names. This work has been started but still needs more work. ** Milan, can you help with changes to spacewalk-schema-upgrade? Also, anyone know what the links are for in the upgrade directories? Do we need them? Can they be created during RPM install? You cannot aggregate upgrade scripts because other products (like Satellite) might want to address individual upgrade scripts (via symlinks, or, work-in-progress, via references). There is a reason we kept the upgrade scripts separated. Can't this be handled at build time? As a matter of fact, I think that if would be good to have the main schema definition (universe.satellite.sql, or main.sql) split into individual source files as well -- basically just pack the source files and concatenate them at install time. What is known at install time that is not known at build time? The reason is that it would make schema overrides by Satellite schema package easier, and upgrade scripts could just reference file name of new package or function, rather than having the source code duplicated and potentially out of sync. So here I feel the move has been in the wrong direction. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
On Thu, May 14, 2009 at 02:33:47PM -0400, Jeff Ortel wrote: > >> Note: I don't really care about the actual format, so I'm not voting >> against the format per se (apart from the trailing spaces it seems to >> introduce). We can put together some coding guidelines and use them >> for new code. > > What's the problem with trailing spaces? $ git apply --whitespace=error /tmp/web_user_site_type.patch /tmp/web_user_site_type.patch:24: trailing whitespace. type CHAR(1) NOT NULL /tmp/web_user_site_type.patch:25: trailing whitespace. CONSTRAINT wust_type_pk PRIMARY KEY, fatal: 2 lines add whitespace errors. $ -- Jan Pazdziora | adelton at #satellite*, #brno Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
On Thu, May 14, 2009 at 03:18:25PM -0400, Jeff Ortel wrote: > >> You cannot aggregate upgrade scripts because other products (like >> Satellite) might want to address individual upgrade scripts (via >> symlinks, or, work-in-progress, via references). There is a reason we >> kept the upgrade scripts separated. > > Can't this be handled at build time? No. You don't know what content is in the latest spacewalk-schema package when you build satellite-schema package, and vice versa. >> As a matter of fact, I think that if would be good to have the main >> schema definition (universe.satellite.sql, or main.sql) split into >> individual source files as well -- basically just pack the source >> files and concatenate them at install time. > > What is known at install time that is not known at build time? Files brought in by other packages and files needed by other packages, from other projects (Satellite, namely). -- Jan Pazdziora | adelton at #satellite*, #brno Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Thu, May 14, 2009 at 03:18:25PM -0400, Jeff Ortel wrote: You cannot aggregate upgrade scripts because other products (like Satellite) might want to address individual upgrade scripts (via symlinks, or, work-in-progress, via references). There is a reason we kept the upgrade scripts separated. Can't this be handled at build time? No. You don't know what content is in the latest spacewalk-schema package when you build satellite-schema package, and vice versa. As a matter of fact, I think that if would be good to have the main schema definition (universe.satellite.sql, or main.sql) split into individual source files as well -- basically just pack the source files and concatenate them at install time. What is known at install time that is not known at build time? Files brought in by other packages and files needed by other packages, from other projects (Satellite, namely). Ah, right. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Thu, May 14, 2009 at 02:33:47PM -0400, Jeff Ortel wrote: Note: I don't really care about the actual format, so I'm not voting against the format per se (apart from the trailing spaces it seems to introduce). We can put together some coding guidelines and use them for new code. What's the problem with trailing spaces? $ git apply --whitespace=error /tmp/web_user_site_type.patch /tmp/web_user_site_type.patch:24: trailing whitespace. type CHAR(1) NOT NULL /tmp/web_user_site_type.patch:25: trailing whitespace. CONSTRAINT wust_type_pk PRIMARY KEY, fatal: 2 lines add whitespace errors. $ Really? git apply can't handle lines w/ trailing white spaces? Hm Ok, we can make sure they're removed. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jason Dobies wrote: Jan Pazdziora wrote: On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: STYLE You may notice that the common/tables/*.sql were formatted using chameleon. Consistent with most DDL/SQL style guides, chameleon renders keywords in caps and uses spaces instead of tabs. If you don't like this style, don't freak out. We take a vote and I can easily re-run the files through chameleon and reformat to any style we like :-) I'm against any reformating which is not supported by semantic validation reasons, for reasons stated in previous posts -- it makes the diff against master bigger and hides things from the naked eye. We should not change the source code in the branch just because we can or just because we have a tool to do that. Note: I don't really care about the actual format, so I'm not voting against the format per se (apart from the trailing spaces it seems to introduce). We can put together some coding guidelines and use them for new code. I'm for the reformatting (our SQL is pretty inconsistent), though I'm not sure if now is necessarily the best time. On one hand, I like the idea of doing such sweeping changes all at once; it'll lead to the codebase settling into its own sooner. On the other, since so much is going on there's a fair risk something will be broken, so having easier diffs while we shake out all the initial bugs might make life easier. Yeah, the formatting was really a byproduct of using chameleon to automate splitting out the triggers. Only the table files were reformatted and I suppose we can re-implement that change and try and preserve the "diffability" as much as possible. If it's still an option, I think my vote is to hold off on the reformatting until after things have settled down a bit. I'm ok with eventually having a big hiccup in diffs, but I think it's going to complicate the initial pgsql bugs. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
On Thu, May 14, 2009 at 11:24:33AM -0400, Jeff Ortel wrote: > >> The views/rhnHistoryView.sql file seems to still contain definition of >> rhnHistoryView_pkglist function. Is that correct? > > Hmm... didn't expect to find function definitions in a view file so I > didn't look. I agree this function should be split out and the view.deps > updated. This file was just an example. If we are touching the file in any way, even if just moving it from one directory to another, and especially with this large schema restructuralization effort, the commit of the file is basically a seal of correctness. If we did not check the files manually or with some tools, we should not be changing or moving the file. I'm much in favor of schema validation tools which will in rpm build time catch issues like this one. I'm very much against reformatting tools that just change the spacing and lowercase to uppercase, if they do not contain the overall validation parts as well. -- 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
Re: [Spacewalk-devel] pgsql review
Jan, I appreciate you comments :) Jan Pazdziora wrote: On Thu, May 14, 2009 at 11:24:33AM -0400, Jeff Ortel wrote: The views/rhnHistoryView.sql file seems to still contain definition of rhnHistoryView_pkglist function. Is that correct? Hmm... didn't expect to find function definitions in a view file so I didn't look. I agree this function should be split out and the view.deps updated. This file was just an example. If we are touching the file in any way, even if just moving it from one directory to another, and especially with this large schema restructuralization effort, the commit of the file is basically a seal of correctness. If we did not check the files manually or with some tools, we should not be changing or moving the file. This really isn't practical. This was a massive effort. We had to touch 400+ files and certifying semantic correctness on files we simply moved is asking a lot. The goal was semantic equivalences which can be checked by validating the installed schema. I'm much in favor of schema validation tools which will in rpm build time catch issues like this one. I'm very much against reformatting tools that just change the spacing and lowercase to uppercase, if they do not contain the overall validation parts as well. Chameleon is not a formatting tool and was not applied to simply change spacing and case. It was used to automate some of the refactoring process and the reformatting was a byproduct. Besides, the files examples you've sited for semantic checking were not reformatted at all. Seems like your primary disagreement is that the files were reformatted at all which makes them difficult to diff. If everyone feels that reformatting these files prevents accepting this work into master, we can re-implement this part of the refactoring. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jeff Ortel wrote: % Jan, I appreciate you comments :) % % Jan Pazdziora wrote: % >On Thu, May 14, 2009 at 11:24:33AM -0400, Jeff Ortel wrote: % >>>The views/rhnHistoryView.sql file seems to still contain definition of % >>>rhnHistoryView_pkglist function. Is that correct? % >>Hmm... didn't expect to find function definitions in a view file so I % >>didn't look. I agree this function should be split out and the view.deps % >>updated. % > % >This file was just an example. % > % >If we are touching the file in any way, even if just moving it from % >one directory to another, and especially with this large schema % >restructuralization effort, the commit of the file is basically a seal % >of correctness. If we did not check the files manually or with some % >tools, we should not be changing or moving the file. % % This really isn't practical. This was a massive effort. We had to touch % 400+ files and certifying semantic correctness on files we simply moved is % asking a lot. The goal was semantic equivalences which can be checked by % validating the installed schema. Well, but once the 'constraint wust_type_nn not null' was changed to 'NOT NULL' the schemas will not be equivalent. Contraint names will differ. Although it doesn't matter for satellite operation it's unacceptable change for the upgrades. % >I'm much in favor of schema validation tools which will in rpm % >build time catch issues like this one. % >I'm very much against % >reformatting tools that just change the spacing and lowercase to % >uppercase, if they do not contain the overall validation parts as well. % > % % Chameleon is not a formatting tool and was not applied to simply change % spacing and case. It was used to automate some of the refactoring process % and the reformatting was a byproduct. Besides, the files examples you've % sited for semantic checking were not reformatted at all. Seems like your % primary disagreement is that the files were reformatted at all which makes % them difficult to diff. If everyone feels that reformatting these files % prevents accepting this work into master, we can re-implement this part of % the refactoring. One issue per commit, please. Chemeleon did refactoring + reformatting + code optimization. How can anyone review the real changes? I've got completely lost in the diffs... :( -- Michael Mráka Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Michael Mraka wrote: Jeff Ortel wrote: % Jan, I appreciate you comments :) % % Jan Pazdziora wrote: % >On Thu, May 14, 2009 at 11:24:33AM -0400, Jeff Ortel wrote: % >>>The views/rhnHistoryView.sql file seems to still contain definition of % >>>rhnHistoryView_pkglist function. Is that correct? % >>Hmm... didn't expect to find function definitions in a view file so I % >>didn't look. I agree this function should be split out and the view.deps % >>updated. % > % >This file was just an example. % > % >If we are touching the file in any way, even if just moving it from % >one directory to another, and especially with this large schema % >restructuralization effort, the commit of the file is basically a seal % >of correctness. If we did not check the files manually or with some % >tools, we should not be changing or moving the file. % % This really isn't practical. This was a massive effort. We had to touch % 400+ files and certifying semantic correctness on files we simply moved is % asking a lot. The goal was semantic equivalences which can be checked by % validating the installed schema. Well, but once the 'constraint wust_type_nn not null' was changed to 'NOT NULL' the schemas will not be equivalent. Contraint names will differ. Although it doesn't matter for satellite operation it's unacceptable change for the upgrades. I checked this before making the change. ALTER TABLE A modify c1 null; This command will remove the NOT NULL constraint regardless of whether it was created by adding a named constraint of using the NOT NULL keywords. % >I'm much in favor of schema validation tools which will in rpm % >build time catch issues like this one. % >I'm very much against % >reformatting tools that just change the spacing and lowercase to % >uppercase, if they do not contain the overall validation parts as well. % > % % Chameleon is not a formatting tool and was not applied to simply change % spacing and case. It was used to automate some of the refactoring process % and the reformatting was a byproduct. Besides, the files examples you've % sited for semantic checking were not reformatted at all. Seems like your % primary disagreement is that the files were reformatted at all which makes % them difficult to diff. If everyone feels that reformatting these files % prevents accepting this work into master, we can re-implement this part of % the refactoring. One issue per commit, please. Chemeleon did refactoring + reformatting + code optimization. How can anyone review the real changes? I've got completely lost in the diffs... :( -- Michael Mráka Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Thu, 14 May 2009 13:11:19 +0200 Jan Pazdziora wrote: > On Tue, May 12, 2009 at 12:42:32PM -0400, Jeff Ortel wrote: > > All, > > > > I'd like to start getting some eyes on the pgsql branch in > > preparation of a merge to master. So far, the changes on this > > branch have been focused on porting the schema and updating the > > application infrastructure to work with both oracle and postgres > > databases. > > This might be a technicality but: do we want to merge or cherry pick? > If we want to start with infrastructure work, I assume we will want to > bring in the changes to master in small steps, addressing one issue at > a time (SQL standard compliance, table / trigger DDL split, etc.). > That way, the changes will have a chance to get verified and tested > in Oracle environment while the diff against the pgsql branch will > get smaller. > > I would not want to see huge merge commit landing in master. > > (I will probably send more responses to your post, commenting on > one issue at a time.) > I do not think this is a good course of action for a number of reasons. Firstly the end state of the branch is the only thing we really know to be working, everything in between could have bugs that we later discovered and fixed but there's no sane way to know what commit goes with what, and changes can depend on other changes in very different locations in the code base. Re-doing the changes with individual cherry-picks will amount to a tremendous amount of overhead as we verify and re-verify everything. Secondly all the work we did to merge in master periodically and resolve the conflicts would be thrown out, we'd get to do it all over again, only now for every patch we cherry-pick that conflicts as opposed to once and only once per merge. There will be a big merge commit (though it is not a huge diff) as the two trees rejoin, but they've been merged together periodically anyhow. This is how long spanning feature work is meant to be done with git. I don't think cherry-picking every individual commit would even really buy us anything. Sure the diff starts shrinking but for the above reasons this probably doesn't mean anything except that we likely have bugs with what we've taken so far and need to go digging to find the commits that they depend on, or to determine if it's a newly discovered issue. IMO we committed to this strategy some time ago, and merge back is our best option by far. Cheers, Devan - -- Devan Goodwin Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoUA+wACgkQAyHWaPV9my7BaQCeLb/mm14U4PTMw1bwJAWznhxK 6AEAoMfpu6JritobZ3hkgm38WJCX7hRo =q/ML -END PGP SIGNATURE- ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
On Thu, May 21, 2009 at 04:23:17PM -0400, Jeff Ortel wrote: > > Pending resolution of the following content related comments and assuming no > new issues > are raised, the branch will be merged to master. Merge vs cherry pick, not > sure which is > best. Thoughts anyone? I certainly hope you merge in this case -- at least that will bring in all the commits from the pgsql branch so it will be possible to search down the history. If you cherry-pick, it will just be one big commit without any details available whatsoever. But maybe I misunderstood what merge vs. cherry pick you have in mind. -- 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
Re: [Spacewalk-devel] pgsql review
On Thu, May 21, 2009 at 04:23:17PM -0400, Jeff Ortel wrote: > > After lengthy discussion it was decided (almost unanimously by attendance) > that the work > on the pgsql branch would be merged to master without refactoring the > commits. There Will you at least allow me to do the refactoring and to put some of the changes that you plan to bring in that merge commit to master before that merge? -- 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
Re: [Spacewalk-devel] pgsql review
On Thursday 21 May 2009 22:23:17 Jeff Ortel wrote: > All, > > The outcome of the meeting yesterday regarding the pgsql branch review is > as follows: > > After lengthy discussion it was decided (almost unanimously by attendance) > that the work on the pgsql branch would be merged to master without > refactoring the commits. There was general agreement that the concerns and > objections raised about doing this had a lot of merit. However, they did > not justify the effort required to re-implement work that has already been > completed. > > Pending resolution of the following content related comments and assuming > no new issues are raised, the branch will be merged to master. Merge vs > cherry pick, not sure which is best. Thoughts anyone? > > Content related concerns to be addressed before merging pgsql to master: > > * Trailing white space needs to be removed. > > I will trim the trailing white spaces. > > * Packaging of upgrade scripts needs to be resolved. > > I committed a change to pgsql branch that will *undo* the proposed > changes to the packaging and installation of the upgrade scripts. They > will be installed in same place and have the same form as before. This > means that initially, upgrades will only be for oracle installs. After the > dust settles on the merge to master we will revisit the upgrades. > > * Replacing the named NOT NULL constraints in the common tables (.sql) > files with simple NOT NULL keywords. I still don't understand why this thing was done for some not null constraints and not for foreign keys for example. > - Concern about the upgrade scripts. > > Using "ALTER TABLE MODIFY NULL;" > instead of: > "ALTER TABLE DROP CONSTRAINT ;" to make a > column nullable in all future upgrade scripts will handle this. > > - The change causes problems in the upgrade verification scripts > because the named NOT NULL constraints will be squawked as missing. > > Unfortunately, when looking at user_constraints, the > constraint_type=R is for both CHECK constraints and NOT NULL constraints. > But, the search_condition can easily be used to ignore NOT NULL > constraints. Validation that the column is NOT NULL, can be done by > matching the nullable column in user_tab_columns when validating a table's > columns. I believe the effort to tweak the upgrade validation scripts will > be much less then reworking the common tables (.sql) files to restore the > named NOT NULL constraints. Should I understand this so that we will relax from the requirement on schema equivalence and ignore the differences in constraint names? -MZ ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Thu, 21 May 2009 16:23:17 -0400 Jeff Ortel wrote: > All, > > The outcome of the meeting yesterday regarding the pgsql branch > review is as follows: > > After lengthy discussion it was decided (almost unanimously by > attendance) that the work on the pgsql branch would be merged to > master without refactoring the commits. There was general agreement > that the concerns and objections raised about doing this had a lot of > merit. However, they did not justify the effort required to > re-implement work that has already been completed. > > Pending resolution of the following content related comments and > assuming no new issues are raised, the branch will be merged to > master. Merge vs cherry pick, not sure which is best. Thoughts > anyone? Definitely merge. Cherry pick IMO would turn into an abysmal task with very little pay off. (i.e. the conflicts will hit for every commit we cherry-pick, rather than a merge where we resolve conflicts for the end result, not to mention that we've already dealt with most of this with the master to pgsql merges) > * Run automated testing on spacewalk installed from pgsql. Can this > be done? If hudson is accurately reporting errors our unit tests have been failing against spacewalk for about a week, we'll probably want to get these issues resolved and run them locally before we commit and push. I fear our other test suites will require us to merge back so we can actually tag packages and have a sane way to deploy the postgresql modified code. (right now we just have that scp script which isn't optimal) Cheers, Devan - -- Devan Goodwin Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoWsQ8ACgkQAyHWaPV9my4ArACfcW7ytNzOGiNdoCdSi1WBe2vB GuYAnRhPcmKyvxVyRfb1k4dWpFDLTvxT =Mnqt -END PGP SIGNATURE- ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Milan Zazrivec wrote: On Thursday 21 May 2009 22:23:17 Jeff Ortel wrote: All, The outcome of the meeting yesterday regarding the pgsql branch review is as follows: After lengthy discussion it was decided (almost unanimously by attendance) that the work on the pgsql branch would be merged to master without refactoring the commits. There was general agreement that the concerns and objections raised about doing this had a lot of merit. However, they did not justify the effort required to re-implement work that has already been completed. Pending resolution of the following content related comments and assuming no new issues are raised, the branch will be merged to master. Merge vs cherry pick, not sure which is best. Thoughts anyone? Content related concerns to be addressed before merging pgsql to master: * Trailing white space needs to be removed. I will trim the trailing white spaces. * Packaging of upgrade scripts needs to be resolved. I committed a change to pgsql branch that will *undo* the proposed changes to the packaging and installation of the upgrade scripts. They will be installed in same place and have the same form as before. This means that initially, upgrades will only be for oracle installs. After the dust settles on the merge to master we will revisit the upgrades. * Replacing the named NOT NULL constraints in the common tables (.sql) files with simple NOT NULL keywords. I still don't understand why this thing was done for some not null constraints and not for foreign keys for example. Well, unlike the other constraints such as PRIMARY KEY, FOREIGN KEY, UNIQUE, CHECK, etc ..., the NULL-ability is typically managed via keyword and is stored with the column definition itself. As such, it can (and usually is) managed via NOT NULL or NULL keyword(s) using a simple ALTER TABLE statement. Most of the other constraints can be added by keyword but unlike NOT NULL can only be removed by dropping the constraint by name. At least as far as I know. So, to support upgrade scripts, the other constraints must remain named. So, unlike the other constraints, creating the NOT NULL named constraint has no value and adds cruft to the table definition. - Concern about the upgrade scripts. Using "ALTER TABLE MODIFY NULL;" instead of: "ALTER TABLE DROP CONSTRAINT ;" to make a column nullable in all future upgrade scripts will handle this. - The change causes problems in the upgrade verification scripts because the named NOT NULL constraints will be squawked as missing. Unfortunately, when looking at user_constraints, the constraint_type=R is for both CHECK constraints and NOT NULL constraints. But, the search_condition can easily be used to ignore NOT NULL constraints. Validation that the column is NOT NULL, can be done by matching the nullable column in user_tab_columns when validating a table's columns. I believe the effort to tweak the upgrade validation scripts will be much less then reworking the common tables (.sql) files to restore the named NOT NULL constraints. Should I understand this so that we will relax from the requirement on schema equivalence and ignore the differences in constraint names? If you are familiar with the upgrade validation scripts and can make this change then I would greatly appreciate your doing so :-) -MZ ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Devan Goodwin wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Thu, 21 May 2009 16:23:17 -0400 Jeff Ortel wrote: All, The outcome of the meeting yesterday regarding the pgsql branch review is as follows: After lengthy discussion it was decided (almost unanimously by attendance) that the work on the pgsql branch would be merged to master without refactoring the commits. There was general agreement that the concerns and objections raised about doing this had a lot of merit. However, they did not justify the effort required to re-implement work that has already been completed. Pending resolution of the following content related comments and assuming no new issues are raised, the branch will be merged to master. Merge vs cherry pick, not sure which is best. Thoughts anyone? Definitely merge. Cherry pick IMO would turn into an abysmal task with very little pay off. (i.e. the conflicts will hit for every commit we cherry-pick, rather than a merge where we resolve conflicts for the end result, not to mention that we've already dealt with most of this with the master to pgsql merges) Okay, just wondering. I know there have been holy wars over merge vs cherry pick and wanted to be sure. * Run automated testing on spacewalk installed from pgsql. Can this be done? If hudson is accurately reporting errors our unit tests have been failing against spacewalk for about a week, we'll probably want to get these issues resolved and run them locally before we commit and push. Good idea. I fear our other test suites will require us to merge back so we can actually tag packages and have a sane way to deploy the postgresql modified code. (right now we just have that scp script which isn't optimal) What are you suggesting? Rather, what do you think is missing? Cheers, Devan - -- Devan Goodwin Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoWsQ8ACgkQAyHWaPV9my4ArACfcW7ytNzOGiNdoCdSi1WBe2vB GuYAnRhPcmKyvxVyRfb1k4dWpFDLTvxT =Mnqt -END PGP SIGNATURE- ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jan Pazdziora wrote: On Thu, May 21, 2009 at 04:23:17PM -0400, Jeff Ortel wrote: After lengthy discussion it was decided (almost unanimously by attendance) that the work on the pgsql branch would be merged to master without refactoring the commits. There I would appreciate anything you can do to make the merge as clean and smooth as possible. However, I'm concerned that changes (refactoring) made in master that resemble changes made in pgsql will cause a lot of merge conflicts or change the end content. We merged master -> pgsql recently and a merge to master now would have very few (if any) conflicts and a deterministic result. With that said, if what you're proposing will make the commit/diffs more usable and not introduce tons of merge conflicts or changes to the end content (not even a little) then we should try it. Maybe while I'm on PTO next week you can create a branch off master, do what you're proposing and when you're finished (and I'm back from PTO) we can do a test merge pgsql => yourbranch and see what happens. Then, compare to a test merge pgsql => master. Will you at least allow me to do the refactoring and to put some of the changes that you plan to bring in that merge commit to master before that merge? ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Fri, 22 May 2009 13:09:44 -0400 Jeff Ortel wrote: > > > > I fear our other test suites will require us to merge back so we can > > actually tag packages and have a sane way to deploy the postgresql > > modified code. (right now we just have that scp script which isn't > > optimal) > > What are you suggesting? Rather, what do you think is missing? Probably need someone more familiar with our other test suites (API and QA automation) but from what I understand they need to be run against an installed and live instance of Spacewalk. To do that we really need to merge back and push to master so we can tag packages and submit them to the devel repo, rather than the hacky scp script we've been getting by with. Nothing missing per se just a suspicion that we can't use these to test prior to merging as we would probably like too. Cheers, Devan - -- Devan Goodwin Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoW9T8ACgkQAyHWaPV9my7/ewCdFO4spTxvz+CZXgA0or9VrkGQ UzQAoJY/8dltRBOqO+NTkAn8oxCiHTT6 =kaeF -END PGP SIGNATURE- ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Devan Goodwin wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Fri, 22 May 2009 13:09:44 -0400 Jeff Ortel wrote: I fear our other test suites will require us to merge back so we can actually tag packages and have a sane way to deploy the postgresql modified code. (right now we just have that scp script which isn't optimal) What are you suggesting? Rather, what do you think is missing? Probably need someone more familiar with our other test suites (API and QA automation) but from what I understand they need to be run against an installed and live instance of Spacewalk. To do that we really need to merge back and push to master so we can tag packages and submit them to the devel repo, rather than the hacky scp script we've been getting by with. Nothing missing per se just a suspicion that we can't use these to test prior to merging as we would probably like too. For the API tests, we can run those against a dev environment that has the latest code/db. It won't have to be an officially installed environment. The tests only care about being able to connect via xmlrpc. (I have run these against my dev env) Cheers, Devan - -- Devan Goodwin Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoW9T8ACgkQAyHWaPV9my7/ewCdFO4spTxvz+CZXgA0or9VrkGQ UzQAoJY/8dltRBOqO+NTkAn8oxCiHTT6 =kaeF -END PGP SIGNATURE- ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
Jeff Ortel writes: > Milan Zazrivec wrote: >> On Thursday 21 May 2009 22:23:17 Jeff Ortel wrote: >>> * Replacing the named NOT NULL constraints in the common tables (.sql) >>> files with simple NOT NULL keywords. >> >> I still don't understand why this thing was done for some not null >> constraints and not for foreign keys for example. > Well, unlike the other constraints such as PRIMARY KEY, FOREIGN KEY, > UNIQUE, CHECK, etc ..., the NULL-ability is typically managed via > keyword and is stored with the column definition itself. As such, it > can (and usually is) managed via NOT NULL or NULL keyword(s) using a > simple ALTER TABLE statement. One point worth mentioning is that the current Postgres implementation simply does not store any constraint name attached to a NOT NULL constraint. It's not clear to me whether any part of Spacewalk depends on having such a name. If it does, then we'd have to fake it by translating simple "NOT NULL" into a "CHECK (field IS NOT NULL)" constraint, which takes more catalog space and is probably a bit slower to check too. If this is considered not necessary, then I support Jeff's approach of removing the names altogether, in hopes of making the Oracle and Postgres behaviors more alike (ie, prevent any Oracle-based developer from inserting a dependency on the name of some NOT NULL constraint...) regards, tom lane ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] pgsql review
On Sun, May 24, 2009 at 08:23:16PM -0400, Tom Lane wrote: > > It's not clear to me whether any part of Spacewalk depends on having > such a name. If it does, then we'd have to fake it by translating > simple "NOT NULL" into a "CHECK (field IS NOT NULL)" constraint, which > takes more catalog space and is probably a bit slower to check too. > > If this is considered not necessary, then I support Jeff's approach > of removing the names altogether, in hopes of making the Oracle and > Postgres behaviors more alike (ie, prevent any Oracle-based developer > from inserting a dependency on the name of some NOT NULL constraint...) It's not Spacewalk runtime functionality which depends on the names. It's schema upgrades and their validation which are done easier if you know that freshly installed schema and schema upgraded all the way up from an ancient Spacewalk/Satellite version have the same constraints, including their names. -- 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
