Patch from Thomas Berezansky addressing circ/hold config constraints, and their lack...

The config.circ_matrix_matchpoint and config.hold_matrix_matchpoint tables have unique constraints that are intended to ensure that you have at most one entry in the table for each set of input conditions.

These constraints operate over a set of fields that can (in whole or part) be set to null, and thus don't do what they are intended to in most (if not basically all) cases.

[This patch] replaces the unique constraints with unique indexes. Nullable fields are coalesced into empty strings so that they can be matched against, and the index only applies to those rows set as active.

The included upgrade script cleans up the tables by taking each set of non-unique rows the unique index would fail on and setting all but the first (in id order) to active=false. If an administrator determines that the wrong row was left active they can toggle active off on the one that was left and back on for the correct one.

git-svn-id: svn://svn.open-ils.org/ILS/trunk@19408 dcc99617-32d9-48b4-a31d-7c20da2025e4

diff --git a/Open-ILS/src/sql/Pg/002.schema.config.sql b/Open-ILS/src/sql/Pg/002.schema.config.sql
index b0c37b8..d497ff3 100644 (file)
--- a/Open-ILS/src/sql/Pg/002.schema.config.sql
+++ b/Open-ILS/src/sql/Pg/002.schema.config.sql
@@ -70,7 +70,7 @@ CREATE TABLE config.upgrade_log (
     install_date    TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW()
 );
 
-INSERT INTO config.upgrade_log (version) VALUES ('0481'); -- dbs
+INSERT INTO config.upgrade_log (version) VALUES ('0482'); -- miker
 
 CREATE TABLE config.bib_source (
        id              SERIAL  PRIMARY KEY,

diff --git a/Open-ILS/src/sql/Pg/100.circ_matrix.sql b/Open-ILS/src/sql/Pg/100.circ_matrix.sql
index c16adaf..ed70f9d 100644 (file)
--- a/Open-ILS/src/sql/Pg/100.circ_matrix.sql
+++ b/Open-ILS/src/sql/Pg/100.circ_matrix.sql
@@ -79,6 +79,7 @@ INSERT INTO config.videorecording_format_map VALUES ('z','Other');
 CREATE TABLE config.circ_matrix_matchpoint (
     id                   SERIAL    PRIMARY KEY,
     active               BOOL    NOT NULL DEFAULT TRUE,
+    -- Match Fields
     org_unit             INT        NOT NULL REFERENCES actor.org_unit (id) DEFERRABLE INITIALLY DEFERRED,    -- Set to the top OU for the matchpoint applicability range; we can use org_unit_prox to choose the "best"
     grp                  INT     NOT NULL REFERENCES permission.grp_tree (id) DEFERRABLE INITIALLY DEFERRED,    -- Set to the top applicable group from the group tree; will need descendents and prox functions for filtering
     circ_modifier        TEXT    REFERENCES config.circ_modifier (code) DEFERRABLE INITIALLY DEFERRED,
@@ -93,6 +94,7 @@ CREATE TABLE config.circ_matrix_matchpoint (
     is_renewal           BOOL,
     usr_age_lower_bound  INTERVAL,
     usr_age_upper_bound  INTERVAL,
+    -- "Result" Fields
     circulate            BOOL    NOT NULL DEFAULT TRUE,    -- Hard "can't circ" flag requiring an override
     duration_rule        INT     NOT NULL REFERENCES config.rule_circ_duration (id) DEFERRABLE INITIALLY DEFERRED,
     recurring_fine_rule  INT     NOT NULL REFERENCES config.rule_recurring_fine (id) DEFERRABLE INITIALLY DEFERRED,
@@ -100,14 +102,11 @@ CREATE TABLE config.circ_matrix_matchpoint (
     hard_due_date        INT     REFERENCES config.hard_due_date (id) DEFERRABLE INITIALLY DEFERRED,
     script_test          TEXT,                           -- javascript source 
     total_copy_hold_ratio     FLOAT,
-    available_copy_hold_ratio FLOAT,
-    CONSTRAINT ep_once_per_grp_loc_mod_marc UNIQUE (
-        grp, org_unit, circ_modifier, marc_type, marc_form, marc_vr_format, ref_flag,
-        juvenile_flag, usr_age_lower_bound, usr_age_upper_bound, is_renewal, copy_circ_lib,
-        copy_owning_lib
-    )
+    available_copy_hold_ratio FLOAT
 );
 
+-- Nulls don't count for a constraint match, so we have to coalesce them into something that does.
+CREATE UNIQUE INDEX ccmm_once_per_paramset ON config.circ_matrix_matchpoint (org_unit, grp, COALESCE(circ_modifier, ''), COALESCE(marc_type, ''), COALESCE(marc_form, ''), COALESCE(marc_vr_format, ''), COALESCE(copy_circ_lib::TEXT, ''), COALESCE(copy_owning_lib::TEXT, ''), COALESCE(user_home_ou::TEXT, ''), COALESCE(ref_flag::TEXT, ''), COALESCE(juvenile_flag::TEXT, ''), COALESCE(is_renewal::TEXT, ''), COALESCE(usr_age_lower_bound::TEXT, ''), COALESCE(usr_age_upper_bound::TEXT, '')) WHERE active;
 
 -- Tests for max items out by circ_modifier
 CREATE TABLE config.circ_matrix_circ_mod_test (

diff --git a/Open-ILS/src/sql/Pg/110.hold_matrix.sql b/Open-ILS/src/sql/Pg/110.hold_matrix.sql
index 84c6b11..ae5d742 100644 (file)
--- a/Open-ILS/src/sql/Pg/110.hold_matrix.sql
+++ b/Open-ILS/src/sql/Pg/110.hold_matrix.sql
@@ -32,6 +32,7 @@ CREATE TABLE config.hold_matrix_matchpoint (
     id                      SERIAL    PRIMARY KEY,
     active                  BOOL    NOT NULL DEFAULT TRUE,
     strict_ou_match         BOOL    NOT NULL DEFAULT FALSE,
+    -- Match Fields
     user_home_ou            INT        REFERENCES actor.org_unit (id) DEFERRABLE INITIALLY DEFERRED,    -- Set to the top OU for the matchpoint applicability range; we can use org_unit_prox to choose the "best"
     request_ou              INT        REFERENCES actor.org_unit (id) DEFERRABLE INITIALLY DEFERRED,    -- Set to the top OU for the matchpoint applicability range; we can use org_unit_prox to choose the "best"
     pickup_ou               INT        REFERENCES actor.org_unit (id) DEFERRABLE INITIALLY DEFERRED,    -- Set to the top OU for the matchpoint applicability range; we can use org_unit_prox to choose the "best"
@@ -45,16 +46,19 @@ CREATE TABLE config.hold_matrix_matchpoint (
     marc_vr_format          TEXT    REFERENCES config.videorecording_format_map (code) DEFERRABLE INITIALLY DEFERRED,
     juvenile_flag           BOOL,
     ref_flag                BOOL,
+    -- "Result" Fields
     holdable                BOOL    NOT NULL DEFAULT TRUE,                -- Hard "can't hold" flag requiring an override
     distance_is_from_owner  BOOL    NOT NULL DEFAULT FALSE,                -- How to calculate transit_range.  True means owning lib, false means copy circ lib
     transit_range           INT        REFERENCES actor.org_unit_type (id) DEFERRABLE INITIALLY DEFERRED,        -- Can circ inside range of cn.owner/cp.circ_lib at depth of the org_unit_type specified here
     max_holds               INT,                            -- Total hold requests must be less than this, NULL means skip (always pass)
     include_frozen_holds    BOOL    NOT NULL DEFAULT TRUE,                -- Include frozen hold requests in the count for max_holds test
     stop_blocked_user       BOOL    NOT NULL DEFAULT FALSE,                -- Stop users who cannot check out items from placing holds
-    age_hold_protect_rule   INT        REFERENCES config.rule_age_hold_protect (id) DEFERRABLE INITIALLY DEFERRED,    -- still not sure we want to move this off the copy
-    CONSTRAINT hous_once_per_grp_loc_mod_marc UNIQUE (user_home_ou, request_ou, pickup_ou, item_owning_ou, item_circ_ou, requestor_grp, usr_grp, circ_modifier, marc_type, marc_form, marc_vr_format, ref_flag, juvenile_flag)
+    age_hold_protect_rule   INT        REFERENCES config.rule_age_hold_protect (id) DEFERRABLE INITIALLY DEFERRED    -- still not sure we want to move this off the copy
 );
 
+-- Nulls don't count for a constraint match, so we have to coalesce them into something that does.
+CREATE UNIQUE INDEX chmm_once_per_paramset ON config.hold_matrix_matchpoint (COALESCE(user_home_ou::TEXT, ''), COALESCE(request_ou::TEXT, ''), COALESCE(pickup_ou::TEXT, ''), COALESCE(item_owning_ou::TEXT, ''), COALESCE(item_circ_ou::TEXT, ''), COALESCE(usr_grp::TEXT, ''), COALESCE(requestor_grp::TEXT, ''), COALESCE(circ_modifier, ''), COALESCE(marc_type, ''), COALESCE(marc_form, ''), COALESCE(marc_vr_format, ''), COALESCE(juvenile_flag::TEXT, ''), COALESCE(ref_flag::TEXT, '')) WHERE active;
+
 CREATE OR REPLACE FUNCTION action.find_hold_matrix_matchpoint(pickup_ou integer, request_ou integer, match_item bigint, match_user integer, match_requestor integer)
   RETURNS integer AS
 $func$

diff --git a/Open-ILS/src/sql/Pg/upgrade/0482.schema.fix_matchpoint_unique.sql b/Open-ILS/src/sql/Pg/upgrade/0482.schema.fix_matchpoint_unique.sql
new file mode 100644 (file)
index 0000000..7382605
--- /dev/null
+++ b/Open-ILS/src/sql/Pg/upgrade/0482.schema.fix_matchpoint_unique.sql
@@ -0,0 +1,82 @@
+BEGIN;
+
+INSERT INTO config.upgrade_log (version) VALUES ('0482');
+
+-- Drop old (non-functional) constraints
+
+ALTER TABLE config.circ_matrix_matchpoint
+    DROP CONSTRAINT ep_once_per_grp_loc_mod_marc;
+
+ALTER TABLE config.hold_matrix_matchpoint
+    DROP CONSTRAINT hous_once_per_grp_loc_mod_marc;
+
+-- Clean up tables before making normalized index
+
+CREATE OR REPLACE FUNCTION action.cleanup_matrix_matchpoints() RETURNS void AS $func$
+DECLARE
+    temp_row    RECORD;
+BEGIN
+    -- Circ Matrix
+    FOR temp_row IN
+        SELECT org_unit, grp, circ_modifier, marc_type, marc_form, marc_vr_format, copy_circ_lib, copy_owning_lib, user_home_ou, ref_flag, juvenile_flag, is_renewal, usr_age_lower_bound, usr_age_upper_bound, COUNT(id) as rowcount, MIN(id) as firstrow
+        FROM config.circ_matrix_matchpoint
+        WHERE active
+        GROUP BY org_unit, grp, circ_modifier, marc_type, marc_form, marc_vr_format, copy_circ_lib, copy_owning_lib, user_home_ou, ref_flag, juvenile_flag, is_renewal, usr_age_lower_bound, usr_age_upper_bound
+        HAVING COUNT(id) > 1 LOOP
+
+        UPDATE config.circ_matrix_matchpoint SET active=false
+            WHERE id > temp_row.firstrow
+                AND org_unit = temp_row.org_unit
+                AND grp = temp_row.grp
+                AND circ_modifier       IS NOT DISTINCT FROM temp_row.circ_modifier
+                AND marc_type           IS NOT DISTINCT FROM temp_row.marc_type
+                AND marc_form           IS NOT DISTINCT FROM temp_row.marc_form
+                AND marc_vr_format      IS NOT DISTINCT FROM temp_row.marc_vr_format
+                AND copy_circ_lib       IS NOT DISTINCT FROM temp_row.copy_circ_lib
+                AND copy_owning_lib     IS NOT DISTINCT FROM temp_row.copy_owning_lib
+                AND user_home_ou        IS NOT DISTINCT FROM temp_row.user_home_ou
+                AND ref_flag            IS NOT DISTINCT FROM temp_row.ref_flag
+                AND juvenile_flag       IS NOT DISTINCT FROM temp_row.juvenile_flag
+                AND is_renewal          IS NOT DISTINCT FROM temp_row.is_renewal
+                AND usr_age_lower_bound IS NOT DISTINCT FROM temp_row.usr_age_lower_bound
+                AND usr_age_upper_bound IS NOT DISTINCT FROM temp_row.usr_age_upper_bound;
+    END LOOP;
+
+    -- Hold Matrix
+    FOR temp_row IN
+        SELECT user_home_ou, request_ou, pickup_ou, item_owning_ou, item_circ_ou, usr_grp, requestor_grp, circ_modifier, marc_type, marc_form, marc_vr_format, juvenile_flag, ref_flag, COUNT(id) as rowcount, MIN(id) as firstrow
+        FROM config.hold_matrix_matchpoint
+        WHERE active
+        GROUP BY user_home_ou, request_ou, pickup_ou, item_owning_ou, item_circ_ou, usr_grp, requestor_grp, circ_modifier, marc_type, marc_form, marc_vr_format, juvenile_flag, ref_flag
+        HAVING COUNT(id) > 1 LOOP
+
+        UPDATE config.hold_matrix_matchpoint SET active=false
+            WHERE id > temp_row.firstrow
+                AND user_home_ou        IS NOT DISTINCT FROM temp_row.user_home_ou
+                AND request_ou          IS NOT DISTINCT FROM temp_row.request_ou
+                AND pickup_ou           IS NOT DISTINCT FROM temp_row.pickup_ou
+                AND item_owning_ou      IS NOT DISTINCT FROM temp_row.item_owning_ou
+                AND item_circ_ou        IS NOT DISTINCT FROM temp_row.item_circ_ou
+                AND usr_grp             IS NOT DISTINCT FROM temp_row.usr_grp
+                AND requestor_grp       IS NOT DISTINCT FROM temp_row.requestor_grp
+                AND circ_modifier       IS NOT DISTINCT FROM temp_row.circ_modifier
+                AND marc_type           IS NOT DISTINCT FROM temp_row.marc_type
+                AND marc_form           IS NOT DISTINCT FROM temp_row.marc_form
+                AND marc_vr_format      IS NOT DISTINCT FROM temp_row.marc_vr_format
+                AND juvenile_flag       IS NOT DISTINCT FROM temp_row.juvenile_flag
+                AND ref_flag            IS NOT DISTINCT FROM temp_row.ref_flag;
+    END LOOP;
+END;
+$func$ LANGUAGE plpgsql;
+
+SELECT action.cleanup_matrix_matchpoints();
+
+DROP FUNCTION IF EXISTS action.cleanup_matrix_matchpoints();
+
+-- Create Normalized indexes
+
+CREATE UNIQUE INDEX ccmm_once_per_paramset ON config.circ_matrix_matchpoint (org_unit, grp, COALESCE(circ_modifier, ''), COALESCE(marc_type, ''), COALESCE(marc_form, ''), COALESCE(marc_vr_format, ''), COALESCE(copy_circ_lib::TEXT, ''), COALESCE(copy_owning_lib::TEXT, ''), COALESCE(user_home_ou::TEXT, ''), COALESCE(ref_flag::TEXT, ''), COALESCE(juvenile_flag::TEXT, ''), COALESCE(is_renewal::TEXT, ''), COALESCE(usr_age_lower_bound::TEXT, ''), COALESCE(usr_age_upper_bound::TEXT, '')) WHERE active;
+
+CREATE UNIQUE INDEX chmm_once_per_paramset ON config.hold_matrix_matchpoint (COALESCE(user_home_ou::TEXT, ''), COALESCE(request_ou::TEXT, ''), COALESCE(pickup_ou::TEXT, ''), COALESCE(item_owning_ou::TEXT, ''), COALESCE(item_circ_ou::TEXT, ''), COALESCE(usr_grp::TEXT, ''), COALESCE(requestor_grp::TEXT, ''), COALESCE(circ_modifier, ''), COALESCE(marc_type, ''), COALESCE(marc_form, ''), COALESCE(marc_vr_format, ''), COALESCE(juvenile_flag::TEXT, ''), COALESCE(ref_flag::TEXT, '')) WHERE active;
+
+COMMIT;