Forum OpenACS Development: Audit of postgres plpgsql procs...
I went through and audited all the core to check which things
could be declared strict and stable or immutable and here are
my results.
Entries in column volatile are 's' = stable, 'i' = immutable, column strict 't' means it could be isstrict (if they are blank it means no change from whatever it is now).
I am not sure how many of these will be addressed pre 5.0 but if anyone is doing scaling work I can provide a script that twiddles these in the pg_proc table w/o recreating the functions.
proname | volatile | strict | comment
---------------------------------------------+----------+--------+--------------------------------------
acs__remove_user | | t | except returns 0
acs_attribute__drop_attribute | | t | returns 0, but deletes nothing if null
acs_attribute__drop_description | | t | returns 0, but deletes nothing if null
acs_group__check_representation | s | | logging side effect, should check null return f immediately
acs_group__delete | | t | returns 0, but deletes nothing if null
acs_group__member_p | s | |
acs_group__name | s | t |
acs_mail_body__body_p | s | |
acs_mail_body__delete | | t | returns 1, but no action if null
acs_mail_gc_object__delete | | t | returns 1, but no action if null
acs_mail_link__delete | | t | returns 1, but no action if null
acs_mail_link__link_p | s | |
acs_mail_multipart__delete | | t | returns 1, but no action if null
acs_mail_multipart__multipart_p | s | |
acs_mail_nt__cancel_request | | t | returns 0, but deletes nothing if null
acs_mail_nt__expand_requests | | | nuke in 5.1
acs_mail_nt__process_queue | | | nuke in 5.1
acs_mail_nt__schedule_process | | | nuke in 5.1
acs_mail_nt__update_requests | | | nuke in 5.1
acs_mail_queue_message__delete | | t | returns 1, but no action if null
acs_message__delete | | t | returns 1, but no action if null
acs_message__delete_extlink | | t | returns 0, but deletes nothing if null
acs_message__delete_file | | t | returns 1, but no action if null
acs_message__delete_image | | t | fix image__delete, returns 1 no action if null
acs_message__first_ancestor | s | t |
acs_message__message_p | s | | returns f on null
acs_message__name | s | t |
acs_message_get_tree_sortkey | s | t |
acs_object__check_context_index | s | | check for null inputs, returns f on null
acs_object__check_object_ancestors | s | | returns f on null, check null inputs , logging sideeffect
acs_object__check_object_descendants | s | | retiurns f on null, check for null inputs logging side effect.
acs_object__check_path | s | |
acs_object__check_representation | | | need check for null input
acs_object__default_name | s | t | bug with null
acs_object__delete | | t | returns 0, but deletes nothing if null
acs_object__get_attr_storage_column | i | t | strict, but exception on null
acs_object__get_attr_storage_sql | i | | strict but exception on null, add output for exception.
acs_object__get_attr_storage_table | i | t | strict but exception on null, add output for exception.
acs_object__get_attribute | s | t | check acs_object__get_attribute_storage is stable strict
acs_object__get_attribute_storage | s | | exception on null in
acs_object__initialize_attributes | | | bug: raise exception on null input.
acs_object__name | s | t |
acs_object__set_attribute | | | bug exception on null
acs_object_type__drop_type | | | bug exception on null? check null anyway cascade_p ignored.
acs_object_type__is_subtype_p | s | | returns f on null, check for null inputs logging side effect.
acs_object_type__pretty_name | s | t |
acs_object_type_get_tree_sortkey | s | t | maybe immutable
acs_object_util__get_object_type | | | bug exception after return
acs_object_util__object_ancestor_type_p | s | | check acs_object_util__type_ancestor_type_p maybe strict
acs_object_util__object_type_exist_p | s | |
acs_object_util__object_type_p | s | |
acs_object_util__type_ancestor_type_p | s | | exceptions on null inputs
acs_objects_get_tree_sortkey | s | |
acs_permission__grant_permission | | t | bug lock doesn't work here? exception on null input?
acs_permission__permission_p | s | | returns 0 on null
acs_permission__revoke_permission | | | bug lock table? exception on null?
acs_privilege__drop_privilege | | | returns 0, but deletes nothing if null
acs_privilege__remove_child | | | returns 0, but deletes nothing if null
acs_reference__delete | | | returns 0, but deletes nothing if null
acs_reference__is_expired_p | | | returns f on null
acs_rel__delete | | | returns 0, but deletes nothing if null
acs_rel_type__drop_role | | | returns 0, but deletes nothing if null
acs_rel_type__drop_type | | | bug: XXX do cascade_p comment?
acs_rel_type__role_pretty_name | s | t |
acs_rel_type__role_pretty_plural | s | t |
acs_sc_binding__delete | | | 2 procs, returns 0, but deletes nothing if null
acs_sc_binding__exists_p | s | |
acs_sc_contract__delete | | | returns 0, but deletes nothing if null
acs_sc_contract__get_id | s | t |
acs_sc_contract__get_name | s | t |
acs_sc_contract__new | | | bug: no contract context
acs_sc_impl__delete | | | returns 0, but deletes nothing if null
acs_sc_impl__get_id | s | t |
acs_sc_impl__get_name | s | t |
acs_sc_impl__new | | | bug no context
acs_sc_impl_alias__delete | | | returns null on null contract or impname but returns impl_id if one exists.
acs_sc_msg_type__delete | | | 2 procs, first 0 on null, second strict
acs_sc_msg_type__get_id | s | t |
acs_sc_msg_type__get_name | s | t |
acs_sc_msg_type__new | | | maybe a bug: msg_type_name can be null.
acs_sc_msg_type__parse_spec | | | bug: exception on null input
acs_sc_operation__delete | | | 2 procs first 0 on ull, 2nd null on null
acs_sc_operation__get_id | s | t |
acs_user__approve_email | | | returns 0, but no update if null
acs_user__delete | | | returns 0, but deletes nothing if null
acs_user__receives_alerts_p | s | | returns f on null
acs_user__unapprove_email | | | returns 0, but no update if null
admin_rel__delete | | | returns 0, but deletes nothing if null
apm__get_value | s | t | 2 procs, both stable strict
apm__id_for_name | s | t |
apm__parameter_p | s | |
apm__register_p | s | |
apm__unregister_package | | | bug says cascade_p default t but code disagrees
apm__unregister_parameter | | t | returns 0, but deletes nothing if null
apm__unregister_service | | | bug no default
apm_application__delete | | t | returns 0, but deletes nothing if null
apm_package__delete | | t | returns 0, but deletes nothing if null
apm_package__highest_version | s | | returns 0 on null (bug?)
apm_package__initial_install_p | s | | returns 0 on null
apm_package__name | s | t |
apm_package__num_instances | s | | bug not found superflous.
content_folder__is_root | s | | bug root = 0?
content_folder__is_sub_folder | | | bug grotesque port I think...
content_folder__unregister_content_type | | | bug no default for include_subtypes
content_item__abs_cursor_next_pos | | | BUG: isn't this just a sequence? its messed up anyway since no locking
content_item__cleanup_cursors | | t | another gross function!
content_item__create_abs_cursor | | | ugh what is this!
content_item__create_rel_cursor | | | ugh what is this!
content_item__delete | | | need to fix this I think since it is broken
content_item__get_best_revision | s | t | not found is superflous.
content_item__get_content_type | s | t | not found is superflous.
content_item__get_context | s | | exception on null
content_item__get_id | s | |
content_item__get_latest_revision | s | | not found is superflous.
content_item__get_live_revision | s | t | not found is superflous.
content_item__get_parent_folder | | | ugh more messed up query stuff
content_item__get_path | s | | does this work? I think thiscreate_abs_cursor stuff might not be safe.
content_item__get_publish_date | s | |
content_item__get_revision_count | s | | maybe should return null on null
content_item__get_root_folder | s | | bug parent_id 0 hard coded.
content_item__get_template | s | t |
content_item__get_title | s | t | 2procs, 1st strict 2nd not
content_item__get_virtual_path | s | | bug second arg ignored.
content_item__is_index_page | s | |
content_item__is_publishable | | | bug: returns t on null and invalid items.
content_item__is_published | s | |
content_item__is_subclass | s | | bug should exit loop (or just be an exists query).
content_item__is_valid_child | s | |
content_item__move | | | bug: silently ignores move to null target, generally sucks
content_item__new | | | 5 function! fooey!
content_item__rel_cursor_next_pos | | | argh. cursor foo
content_item__unrelate | | | returns 0, but deletes nothing if null
content_item__unset_live_revision | | | returns 0, but no update if null
content_item__write_to_file | | | bug raise exception instead.
content_keyword__delete | | | returns 0, but deletes nothing if null
content_keyword__get_description | s | t |
content_keyword__get_heading | s | t |
content_keyword__get_path | s | t |
content_revision__copy | | | BUG: inserts directly into acs_objects rather than using __new
content_revision__copy_attributes | | | bug should exception on nulls.
content_revision__delete | | | returns 0, but deletes nothing if null
content_revision__export_xml | | | bug: does this work? No! exception probably
content_symlink__copy | | | bug silently doesnt move if target is not a folder
content_symlink__delete | | | returns 0, but deletes nothing if null
content_symlink__is_symlink | s | |
content_symlink__resolve | s | t |
content_symlink__resolve_content_type | s | t |
content_template__delete | | | returns 0, but deletes nothing if null
content_template__get_path | s | |
content_template__get_root_folder | i | |
content_template__is_template | s | |
doc__get_proc_header | s | t |
get_func_header | s | |
group_contains_p | s | |
image__delete | | | returns 0, but deletes nothing if null
journal_entry__delete | | | returns 0, but deletes nothing if null
journal_entry__delete_for_object | | | returns 0, but deletes nothing if null
lob_copy | | | BUG: exception on null from_id
lob_get_data | | | BUG: did this ever work?
lob_length | s | |
membership_rel__approve | | | returns 0, but no update if null
membership_rel__ban | | | returns 0, but no update if null
membership_rel__check_index | | | stable except for logging
membership_rel__check_representation | | | stable except for logging
membership_rel__delete | | | returns 0, but deletes nothing if null
membership_rel__deleted | | | returns 0, but no update if null
membership_rel__reject | | | returns 0, but no update if null
apm_package__parent_id | s | | returns -1 on not found BUG?
apm_package__singleton_p | s | | bug count(*), remove not found
apm_package_type__num_parameters | s | |
apm_package_version__delete | | | returns 0, but deletes nothing if null
apm_package_version__disable | | | returns 0, but no update if null
apm_package_version__enable | | | returns 0, but no update if null
apm_package_version__remove_dependency | | | bug 2 procs, second ignores version_id input returns 0, but deletes nothing if null
apm_package_version__remove_interface | | | bug 2nd proc ignores arg3 version_id returns 0, but deletes nothing if null
apm_package_version__sortable_version_name | i | | bug: exception on null? (currently returns -1)
apm_package_version__upgrade | | | returns 0 on null (bug maybe exception)?
apm_package_version__upgrade_p | i | |
apm_package_version__version_name_greater | i | |
apm_parameter_value__delete | | | returns 0, but deletes nothing if null
apm_service__delete | | | returns 0, but deletes nothing if null
application_group__delete | | | returns 0, but deletes nothing if null
application_group__group_id_from_package_id | s | | exception on null
authority__del | | | returns 0, but deletes nothing if null
bitfromint4 | i | t |
bittoint4 | i | t |
column_exists | s | |
composition_rel__check_index | | | stable except for logging
composition_rel__check_path_exists_p | | | stable except for logging
composition_rel__check_representation | | | stable except for logging in children
composition_rel__delete | | | returns 0, but deletes nothing if null
content_extlink__delete | | | returns 0, but deletes nothing if null
content_extlink__is_extlink | s | |
content_folder__delete | | | returns 0, but deletes nothing if null
content_folder__get_index_page | s | t |
content_folder__get_label | s | t |
content_folder__is_empty | s | | should it return null or f on null?
content_folder__is_folder | s | | null on null? currently its f
content_folder__is_registered | s | | bug no default in code for include_subtypes
content_keyword__is_assigned | s | | bug no default for recurse...
content_keyword__is_leaf | s | |
content_keyword__item_assign | | | bug: concurrency
content_keyword__item_unassign | | | returns 0, but deletes nothing if null
content_revision__content_copy | | | probably should exception on null revision_id
content_revision__get_content | s | t |
content_revision__get_number | s | t | bug: not a terribly efficient way to do this I think.
content_revision__import_xml | | | bug: exception needed
content_revision__index_attributes | | | bug: exception needed.
content_revision__is_latest | s | | bug: maybe should exception on null or not a revision
content_revision__is_live | s | | bug: exception on null/not a revision
content_revision__new | | | ugh 4 procs!
content_revision__revision_name | s | | should exception on null maybe? currently returns nonsensical string
content_revision__to_html | | | bug: does this work?
content_revision__to_temporary_clob | | | bug: remove this?
content_type__drop_attribute | | | BUG: 7.3 we can in fact drop columns so turn that on
content_type__drop_type | | | BUG: probably needs testing
content_type__get_template | s | t |
content_type__is_content_type | s | |
content_type__trigger_insert_statement | s | | bug: should exception on null
content_type__unregister_child_type | | | bug: says defalt null for relation_tag but isn't really
content_type__unregister_mime_type | | | returns 0, but deletes nothing if null
content_type__unregister_relation_type | | | returns 0, but deletes nothing if null
cr_items_get_tree_sortkey | s | t |
cr_keywords_get_tree_sortkey | s | t |
doc__get_package_header | i | t |
membership_rel__unapprove | | | returns 0, but no update if null
number_src | | | BUG: infinite loop on null
party__delete | | | returns 0, but deletes nothing if null
party__email | s | t |
party__name | i | t |
party_approved_member__remove_one | | | returns 1, but deletes nothing if null
person__delete | | | returns 0, but deletes nothing if null
person__first_names | s | t |
person__last_name | s | t |
person__name | s | t |
rdbms_date | i | t |
rel_constraint__delete | | | returns 0, but deletes nothing if null
rel_constraint__get_constraint_id | s | t |
rel_constraint__violation | s | t |
rel_constraint__violation_if_removed | s | t |
rel_segment__delete | | | returns 0, but deletes nothing if null
rel_segment__get | s | t |
rel_segment__get_or_new | | | yuck!
rel_segment__name | s | t |
rule_exists | s | | BUG: case fold?
search_observer__dequeue | | | this looks fishy to me. deleting based on times.
site_node__delete | | | returns 0, but deletes nothing if null
site_node__find_pattern | | | this is fishy too. what exactly does pattern_p mean?
site_node__node_id | | | this is also fishy.
site_node__url | | | sad this is done with recursion
site_node_get_tree_sortkey | s | t |
site_node_object_map__del | | | returns 0, but deletes nothing if null
subsite_callback__delete | | | returns 0, but deletes nothing if null
table_exists | s | |
timezone__convert_to_local | s | |
timezone__convert_to_utc | s | |
timezone__delete | | | returns 0, but deletes nothing if null
timezone__get_abbrev | s | t |
timezone__get_date | s | |
timezone__get_id | s | t |
timezone__get_offset | s | |
timezone__get_rawoffset | s | |
timezone__isdst_p | s | |
tree_ancestor_keys | | | Can this be immutable?
trigger_exists | s | |
util__logical_negation | i | t |
(266 rows)
"stable" corresponds to "iscachable" in PG < 7.3, and "iscachable" has been kept for backwards compatibility. I assume you've just listed those that weren't set "iscachable" ...
Since 5.0 is dropping support for 7.2 we might as well adopt the new terminology. Also we can just drop bittoint4 etc since these are implemented by casts in 7.3 (these bit functions were defined by PG itself in 7.2).
Should we do this for 5.0?
iscachable = immutable rather than stable (eg acs_objects_get_tree_sortkey is declared iscachable which makes it immutable in the pg_proc table but it's really
stable instead).
Yeah, I think we should fix them for 5.0 (some of them anyway) although I think we should write the update script to manipulate pg_proc directly rather than trying to reload all the changed functions.
In 7.2 iscachable was only used to determine if a function could be pre-executed in the context of a single SQL statement.
Iscachable indicates that the function always returns the same result when given the same argument values (i.e., it does not do database lookups or otherwise use information not directly present in its parameter list). The optimizer uses iscachable to know whether it is safe to pre-evaluate a call of the function.So in practice it was the same as stable, because the optimizer only examines one SQL statement at a time and the function was always executed and then cached by the executor for each SQL statement. They added immutable and cross-statement function result caching for 7.3.
I'm not familiar with the actual implementation in 7.3 but it's clear from the documentation that relying on the (documented, I might add) implementation of "iscachable" in 7.2 might result in errors in PG 7.3. We'd better look into fixing these ... I wish they hadn't done that, we probably aren't the only people with potential bugs here.
I think these should be changed to stable:
trigger_type acs_objects_get_tree_sortkey forums_message__root_message_id file_storage__get_root_folder file_storage__get_package_idI don't see any others that should be a problem.
Strictly speaking acs__magic_object_id maybe should be but I think it's safe to leave as immutable...
I committed the changes for the core plpgsql procs and provided an upgrade script for the procs which changed internally (bug fixes), but no upgrade script to change the
strict/volatile flags. I will try to get one together
in the next day or two.
