Forum OpenACS Q&A: There's a problem with education/class/one.tcl

In displaying the assignments, there is a complex SQL statement. It has not been ported correctly, yet I am at odds at interpreting exactly how it works. Here's the origianal text:


# set sql "select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             sol.url as sol_url,
#             sol.file_extension as sol_file_extension,
#             sol.version_id as sol_version_id,
#             answers.url as ans_url,
#             answers.file_extension as ans_file_extension,
#             answers.file_title as ans_filename,
#             answers.version_id as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             (select * from fs_versions_latest 
#               where
ad_general_permissions.user_has_row_permission_p($user_id, 'read',
version_id, 'FS_VERSIONS') = 't') pset,
#             (select file_extension, url, version_id, task_id 
#                     from fs_versions_latest ver,
#                          edu_task_solutions solutions
#                    where ver.file_id = solutions.file_id
#                      and
ad_general_permissions.user_has_row_permission_p($user_id, 'read',
version_id, 'FS_VERSIONS') = 't') sol,
#             (select file_extension, file_title, url, version_id,
task_id
#                     from fs_versions_latest ver,
#                          edu_student_answers ans,
#                          fs_files
#                    where ver.file_id = ans.file_id 
#                      and fs_files.file_id = ver.file_id
#                      and
ad_general_permissions.user_has_row_permission_p($user_id, 'read',
version_id, 'FS_VERSIONS') = 't'
#                      and student_id = $user_id) answers
#       where edu_assignments.class_id = $class_id 
#         and edu_assignments.file_id = pset.file_id(+)
#         and edu_assignments1.assignment_id =
edu_assignments.assignment_id
#         and edu_assignments1.assignment_id = answers.task_id(+)
#         and edu_assignments.assignment_id = sol.task_id(+)
#      order by due_date"

I know that it's saying give me those assignments that the user is allowed to see and that is the latest version, but I'm going to need a clearer understanding of what's going on to get it fixed.

Right now, it doesn't come up with any assignments, although it ought to.

I guess I ought to be more specific.

  1. What does ad_general_permissions.user_has_row_permissions_p($user_id,'read',version_id,'FS_VERSIONS') = 't' do? And why can't we use the same thing in Postgres, or write something that works the same in the SQL statment?
  2. It looks like answers, pset, and sol are the effects of these select statements. It also looks like:
    • pset is joining ad_general_permissions to fs_versions_latest.
    • sol is joinging edu_tasks_solutions with fs_versions_latest.
    • answers is joining fs_versions_latest, edu_student_answers, and fs_files.
    If we really can't get something to work like asked above, what's your feedback to building three views for the case? I know, they probably couldn't be reused very easily. But, it would probably work.
  3. Why is there edu_assignments1? What is the benefit of referencing a second instance of the table? Can't we just join answers to edu_assignments? If so, that would leave us with edu_answers, pset, answers, and sol to join. Is that right? Possible?
Collapse
Posted by Don Baccus on
1.What does ad_general_permissions.user_has_row_permissions_p($user_id,'read',version_id,'FS_VERSIONS') = 't' do? And why can't we use
          the same thing in Postgres, or write something that works the same in the SQL statment? 
Postgres doesn't have packages, but there are equivalents available in the port. Look at the general permissions SQL file for more info.

I just spent the day digging through intranet, portals, etc weeding out bugs so am not up to tackling this one at the moment, but wanted to mention that. Also, a big clue - get your head around what the query really wants to do, then look into re-writing it within the limitations of PG rather than trying a mechanical translation. I just simplified and fixed some complex unfoldings of outer joins in the intranet module based on doing this (which is not a criticism of our team member who did the original unfolding, it was an early big effort on his part and he had no real help from me or others who'd been down that path before).

Along with this, I've found that some of the more complex queries I've dealt with can easily be restructured to avoid some of the porting hassle. No knock on aD here (though I'm not surrendering my right to knock 'me right and left as I see fit!) but taking a straightforward approach in Oracle that doesn't necessarily port easily is no sin on their part.

O.k. I admit it. I ported the original query, and yes it is wrong. When I first ported it, I didn't quite understand how to handle those on-the-fly views, and in this case I got it wrong. The approch here was to convert the on-the-fly views to actual views. The problem with doing that is the on-the-fly views use tcl variables that aren't known until run-time. To solve that problem I created partial views that mimicked the original on-the-fly views and added the remaining where clauses into the where clause for the main query. This works o.k., but in the case of this query I didn't compensate each of the sub-clauses that use a not exits statement to emulate the outer-joins. The main problem was that I added "and answers.student_id = $user_id" to each of the unioned sub-selects. This and clause needs to be removed for the sub-clauses that use not exists on the student_answers table. I've tried to post the corrected query, but I keep getting a dml error message.
Here is the corrected query: Here is the corrected query (sorry about the long post):
# set sql "select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             sol.url as sol_url,
#             sol.file_extension as sol_file_extension,
#             sol.version_id as sol_version_id,
#             answers.url as ans_url,
#             answers.file_extension as ans_file_extension,
#             answers.file_title as ans_filename,
#             answers.version_id as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             solution_files sol,
#             student_answer_files answers
#       where edu_assignments.class_id = $class_id 
#         and answers.student_id = $user_id
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and edu_assignments.file_id = pset.file_id
#         and edu_assignments1.assignment_id = answers.task_id
#         and edu_assignments.assignment_id = sol.task_id
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             '' as sol_url,
#             '' as sol_file_extension,
#             '' as sol_version_id,
#             answers.url as ans_url,
#             answers.file_extension as ans_file_extension,
#             answers.file_title as ans_filename,
#             answers.version_id as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             student_answer_files answers
#       where edu_assignments.class_id = $class_id 
#         and answers.student_id = $user_id
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and edu_assignments.file_id = pset.file_id
#         and edu_assignments1.assignment_id = answers.task_id
#         and not exists (select 1 from solution_files 
#                         where task_id = edu_assignments.assignment_id)
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             sol.url as sol_url,
#             sol.file_extension as sol_file_extension,
#             sol.version_id as sol_version_id,
#             '' as ans_url,
#             '' as ans_file_extension,
#             '' as ans_filename,
#             '' as ans_version_id,
#            edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             solution_files sol
#       where edu_assignments.class_id = $class_id 
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and edu_assignments.file_id = pset.file_id
#         and not exists (select 1 from student_answers
#                         where task_id = edu_assignments1.assignment_id
#                         and student_id = $user_id)
#         and edu_assignments.assignment_id = sol.task_id
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             '' as sol_url,
#             '' as sol_file_extension,
#             '' as sol_version_id,
#             '' as ans_url,
#             '' as ans_file_extension,
#             '' as ans_filename,
#             '' as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset
#       where edu_assignments.class_id = $class_id 
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and edu_assignments.file_id = pset.file_id
#         and not exists (select 1 from student_answers
#                         where task_id = edu_assignments1.assignment_id
#                         and student_id = $user_id)
#         and not exists (select 1 from solution_files 
#                         where task_id = edu_assignments.assignment_id)
# union

I think the dml failure was due to the length of the post. Here's the rest of the corrected query:
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             sol.url as sol_url,
#             sol.file_extension as sol_file_extension,
#             sol.version_id as sol_version_id,
#             answers.url as ans_url,
#             answers.file_extension as ans_file_extension,
#             answers.file_title as ans_filename,
#             answers.version_id as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             solution_files sol,
#             student_answer_files answers
#       where edu_assignments.class_id = $class_id 
#         and answers.student_id = $user_id
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and not exists (select 1 from fs_versions_latest
#                         where file_id = edu_assignments.file_id)
#         and edu_assignments1.assignment_id = answers.task_id
#         and edu_assignments.assignment_id = sol.task_id
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             '' as sol_url,
#             '' as sol_file_extension,
#             '' as sol_version_id,
#             answers.url as ans_url,
#             answers.file_extension as ans_file_extension,
#             answers.file_title as ans_filename,
#             answers.version_id as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             student_answer_files answers
#       where edu_assignments.class_id = $class_id 
#         and answers.student_id = $user_id
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and not exists (select 1 from fs_versions_latest
#                         where file_id = edu_assignments.file_id)
#         and edu_assignments1.assignment_id = answers.task_id
#         and not exists (select 1 from solution_files 
#                         where task_id = edu_assignments.assignment_id)
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             sol.url as sol_url,
#             sol.file_extension as sol_file_extension,
#             sol.version_id as sol_version_id,
#             '' as ans_url,
#             '' as ans_file_extension,
#             '' as ans_filename,
#             '' as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset,
#             solution_files sol
#       where edu_assignments.class_id = $class_id 
#         and user_has_row_permission_p($user_id, 'read', pset.version_id, 'FS_VERSIONS') = 't'
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and not exists (select 1 from fs_versions_latest
#                         where file_id = edu_assignments.file_id)
#         and not exists (select 1 from student_answers
#                         where task_id = edu_assignments1.assignment_id
#                         and student_id = $user_id)
#         and edu_assignments.assignment_id = sol.task_id
# union
# select edu_assignments.assignment_name, 
#             edu_assignments.assignment_id, 
#             edu_assignments.due_date, 
#             pset.version_id, 
#             pset.file_extension,
#             pset.url,
#             '' as sol_url,
#             '' as sol_file_extension,
#             '' as sol_version_id,
#             '' as ans_url,
#             '' as ans_file_extension,
#             '' as ans_filename,
#             '' as ans_version_id,
#             edu_assignments.electronic_submission_p
#        from edu_assignments,
#             edu_assignments edu_assignments1,
#             fs_versions_latest pset
#       where edu_assignments.class_id = $class_id 
#         and edu_assignments1.assignment_id = edu_assignments.assignment_id
#         and not exists (select 1 from fs_versions_latest
#                         where file_id = edu_assignments.file_id)
#         and not exists (select 1 from student_answers
#                         where task_id = edu_assignments1.assignment_id
#                         and student_id = $user_id)
#         and not exists (select 1 from solution_files 
#                         where task_id = edu_assignments.assignment_id)
#       order by due_date"
Sorry about the tcl comments in the corrected query.  I was trying to figure out why my post wasen't working.  I originally thought that somehow the tcl code was trying to interpret the $ symbols in the original post.  It would be nice to add a check in the bboard to warn the user if their post is too long instead of giving a generic dml failed message.
Collapse
Posted by Don Baccus on
It will be even nicer when Ben upgrades to our new release with its beautiful lztext bboard module and the resultant 32KB posts we'll be able to make :)

Dan, did you commit this query to CVS?

Also, you might check out the way I rewrote /intranet/employees/admin/index.tcl and friends.  I was able to get away from the horribly complex set of unions that resulted from mechanical translation.  The result might be a bit slower, using subselects in the select clause of the main query, but I at least find  it a heck of a lot easier to read.  And it might not be slower, I suspect huge unfolded unions might slow down the optimizer as much as the subselects in my rewrite slow down the executor.

My rewrite trick won't apply to all complex outer joins, mechanically unfolded horrors like your example here are still going to be necessary at times.  I haven't systematically gone through intranet yet to see if I can do similar simplifications (I was originally motivated by the fact that a couple of your big unions didn't quite work right, but I suspect most of them do).

In the case of the intranet module, it's all bogus anyway.  If the turkey who wrote it always created an im_employee_info entry when you add an employee it wouldn't be necessary to outer-join to the table in the first place.  Given that org-chart.tcl breaks in ACS Classic until  you manually add "none" as a supervisor for your CEO (as opposed to the "none" created by magic via outer joins), aD doing it right in the first place sounds like it would've been a winner to me...

Dan, did you commit this query to CVS?

I'll commit it tonight. I'm at work now, and I'm stuck behind a firewall, so I don't have access to cvs.

Also, you might check out the way I rewrote /intranet/employees/admin/index.tcl and friends. I was able to get away from the horribly complex set of unions that resulted from mechanical translation.

I'll take a look at it. Ben fixed one of the intranet queries for me that had 16 unioned selects. It turned out that all of the outer-joined components were non-null, so in actuality, only two of the 16 outer join cases would actually occur. The fixed query had only two select clauses.

My rewrite trick won't apply to all complex outer joins, mechanically unfolded horrors like your example here are still going to be necessary at times.

When I was porting intranet, I suspected that many of the queries could be simplified, but given the size of the task I opted for the mechanical porting approach. Usually it went copy, paste, paste, paste, add not exits clauses and remove dangling tables in the from clause. Then test and bang it was done! I did alot of these in less that five minutes.

I haven't systematically gone through intranet yet to see if I can do similar simplifications (I was originally motivated by the fact that a couple of your big unions didn't quite work right, but I suspect most of them do).

That might be a could thing to do for all of the modules once they stabilize. I didn't want to spend too much time on intranet as my impression was that it was still a work in progress from aD. Major changes occured from 3.1 to 3.2, and there still seem to be some design issues that could be fixed. Have you looked at intranet in the 3.2.2 release? There might be some new fixes there.

Collapse
Posted by Don Baccus on
Oh, I'm not criticizing the fact that you went for mechanical expansion in the least.  They almost all work fine.  My point's more along the line that those that don't quite work as intended can sometimes be more easily fixed by studying the original query, and simplifying rather than going on and on through the huge union expansion trying to figure out which dangling table name didn't get dropped from a particular clasue and is causing a cartesian product, etc.
Well, I got it working.  Thank you for your help.  I hope that my posts in these discussion forums don't rub people the wrong way.  Let me know if I offend you when I say something isn't working.

I used the new sql statement that was submitted, and then had to spend the night last night trying to tweak it to work right.  I actually have a final version that we should probably submit.  I also had to do a major rework of the sql statement in /education/class/assignments-info.tcl.  This too is available if you want it.

Which brings me to my next question.  I'm spending 6-8 hours a day learning what I can about this system, and fixing errors as I go.  I know that you have an organized development lifecycle.  I also understand I'm not an official part of that group.  How would you like me to work with you on this?  Since I'm new, and prone to errors (even though I've been programming for a few years, all this web stuff is only about 8 months old to me now) maybe I ought to submit files to someone for a peer review.  Maybe later I can actually make additions to the CVS tree.  (I'll need someone to point me to a URL or two so that I can train myself with this tool when we get to that point).

A lot of what I've changed in the system has been in regard to karl goldstein's DPS.  If you decide to port 3.3, let me know.  Also, I've gone through the whole system and cleaned out all the nextval stuff.  There were 300 some odd illegal references left in beta 3.  I know that this kind of change blows your CVS tree apart (don't you have to check out and add one file at a time?).  We'll need to work out some plan on using the hours I'm putting into the system for the general good, and not just for my own port of the system.

Finally, it looks like I'm going to venture out from under the corporate umbrella and beat my own path to success in about two or three weeks now.  I start graduate school in about a month, and I've decided that I'd rather put in 60 hours a week working for myself than 80 hours a week working for someone else and myself.  That means I'll have all kinds of time with this system.

Can you tell my mind is going in a million directions?  Sorry.

Collapse
Posted by Ben Adida on
Dave,

It's good to see this much interest, but I'm worried that you're working on stuff that doesn't necessarily need fixing! For example, the nextval stuff is fixed by some automatic filtering of SQL queries in tcl/postgres.tcl. I'm sure a couple of errors were still there, but certainly not 300....

So, we're open to having more people on the team, definitely, we're not a closed development team. I recommend you not hesitate to post more of your thoughts, so that you can get the feedback from the whole team, and we can solve these issues together! I hope we can get your help on this in the long run, this is the kind of enthusiasm we need!

Stay posted, I'm in the process of upgrading the way we do CVS and development in preparation for 4.0 development. We won't be porting 3.3, since it's more experimental than anything else.

300 ??? Err... Maybe not. I don't think OpenACS would be working with 300 nextvals to fix. Some of these things are done through postgres.tcl in some magic triggers/functions that people in the team wrote.

My experience in the team has been that it is very open to contributions, and a very fun team to work with. Lots of good learning involved. Just sign up at Sourceforge (we should move to SDM-only shortly) and let Ben know.

What you are doing here, of sharing your thoughts is very welcome. Because there's a lot of code, make sure you let the rest of the team know what you want to work with and before doing major changes or changing something that is going to drift us away too much from the aD code.

I've often times thought that something was broken, when it actually had a reason to be that way (e.g. decripting passwords from the DB). Thanks for the help Dave !

That is good news if the nextval statements weren't broken.  I was running into trouble with a few pages that used sequence.nextval.  So, I figured out the three or four ways you suggest converting it, and worked out the issues.  Then, I greped the system, found 28 pages of nextvals.  I refined my list, and came just short of 300 files where nextval was being used in the Oracle fashion--sequence.nextval.  So, I just worked through the whole list.  I know it fixed my immediate problems.

I'm glad that such a gaping hole wasn't actually left open.  However, I know that the whole system hasn't felt nearly as flakey since I worked through this issue.  (I used to run into a lot of buggy pages which I would ignore if I were just touring the toolkit, but would hammer through if it was important to me.)

Anyway, thanks for your encouragement and added insight into the issue.

Collapse
Posted by Don Baccus on
Oh, without doubt there have been some dangling nextval's left in that  weren't being subjected to the Tcl clean-up routines (4.0 will run queries through a central db handle manager, giving us a hook to fix ALL simple stuff like nextval, which will make our life much easier).

I think the concern's that you spend time hacking through 300 of them when we could've gotten more work out of you if we'd known what you were doing :)

OpenACS needs lots and lots of testing.  Not only do we have our own porting errors to worry about, but as distributed by aD the toolkit's got plenty of bugs that we'd like to weed out, too.

education/class/one.tcl
/education/class/assignments-info.tcl
where is it available??

do it needs some extra work or just copy the files....

thanx for your help!

Um, they're two files.  I have some fixed versions, but I haven't seen what's in the latest openACS to see if it's necessary to send those around or not.  Give me a couple of days, and I'll either get them up to the openACS guys, or I'll send them to you, or I'll redirect you to download the latest version of the openACS.

P.S. I noticed you actually visited my site!  Two people that aren't blood relatives or long-time friends have actually logged into my site!  I'm exstatic.  It needs a lot of work, doesn't it?