Re: [Gnumed-devel] gmClinical.sql and general comments

[Karsten Hilbert]

On Thu, Jan 12, 2006 at 08:44:45AM +1100, Richard wrote:

> ========
> gmClinical.sql
> ========
> Apologies first if this is a bit garbled, but I've been jotting notes as I go.
No problem.

> I'm getting really stuck on this one Karsten, it seems to  me this file  is a 
> maintenance nightmare.
It is.

> It is a huge file, and covers many many areas of clinical things all stuffed 
> in together.
Yes. The reason is historical. I have been getting away from
that with the newer stuff.

> Is there any reason that it can't be split up into logical units eg 
> gmVaccinations.sql, gmAllergies.sql etc?.
No reason. However, if so do maintain the separation of
static (tables) vs dynamic (pretty much everything else)
stuff. The reason is that the dynamic stuff can be *rerun*
on an existing database without putting the actual data in
jeopardy at all.

> Some of the areas causing me problems seem to be around statements containing 
> lines like:
> 
> alter table clin.vacc_def  (from gmClinicalViews.sql) 
>       add constraint numbered_shot_xor_booster
>               check (
>                       ((is_booster is true) and (seq_no is null)) or
>                       ((is_booster is false) and (seq_no > 0))
>               );
> 
> this won't compile, stops and asks to input  a parameter
Very strange. I tend to think pgAdmin is not sophisticated
enough. The above is perfectly valid SQL and it bootstraps
just fine. You might have to contact the pgAdmin people.

> Also it  crashes and burns with syntax errors in some statements  eg:
> 
> -- crashes and burns (syntax error at char 19) --- start *****
> create or replace trigger tr_ins_booster_must_have_base_immunity
         ^^^^^^^^^^
This DOES NOT EXIST in CVS. It would indeed be invalid.

>       before insert on clin.vacc_def
>       for each row execute procedure 
> clin.f_ins_booster_must_have_base_immunity();
> -- crashes and burns (syntax error at char 19) --- end *****

> I  will try and dissect this file into smaller subunits myself and let you 
> know the result in a more specific manner, but if you could give thought to 
> putting it into smaller files that would be appreciated.
Sure.

> Another couple of questions  - in the code you often have on error start/stop 
> statements when creating a function or table. Is there any reason you do this 
> rather than using
The reason is this:

The script is *supposed* to work in its entirety. If it
doesn't it contains a bug. That's why we do "\set
ON_ERROR_STOP 1" at the top. This means any error will stop
the script execution. Unfortunately, some things are treated
as errors by PostgreSQL which aren't really errors, for example:

- to create a view it must not yet exist
- there is no way to *test* whether it exists
- hence we need to first DROP the view
- however, if the view actually didn't exist dropping it would be an error
- hence we temporarily allow the dropping to fail

Now, one could just as well allow the creation to fail and
continue but that would not allow to differentiate the
*reason* for that failure while the above procedure
guarantuees that the failure is NOT due to the view already
existing.

> Create or replace function/table?
"create or replace" only works for functions, any functions
not yet using "create or replace" just has not been
converted yet

> Lastly sql file naming conventions - It. would be nice to have same 
> capitalisation, hyphenation schemata for the files.
The convention is:

- start with "gm"
- end with ".sql"
- name it for the content it handles (yes, gmClinial.sql sucks)
- add -dynamic/-static/-data to differentiate

Hence:

 gmProviderInbox-static.sql
 gmProviderInbox-dynamic.sql

Any files not following this convention just haven't been
converted yet. If you think it is *really* important I'll
rename them ASAP.

Karsten
-- 
GPG key ID E4071346 @ wwwkeys.pgp.net
E167 67FD A291 2BEA 73BD  4537 78B9 A9F9 E407 1346