valid_z3950_attr_type check constraint can cause errors during pg_restore

[Galen Charlton]

The valid_z3950_attr_type check constraint on the config.z3950_index_field_map table can cause czifm rows to not be loaded during a pg_restore. In particular, if czifm gets loaded *before* config.z3950_attr is populated, any row in czifm that has a non-null z3950_attr_type would violate the check constraint, causing czifm to not be populated by pg_restore.

Changing the check constraint to a trigger should avoid the problem.

Evergreen master

[Ben Shum (bshum)]

Marking this issue confirmed. We've seen this occur during our pg_restores where it would randomly fail on this.

[Jason Boyer (jboyer)]

I'm interested in getting some discussion going on this. (There's no time to worry about it for the upgrade I'm doing in 3 days which is the only time I remember it, so I'm just dropping the constraint temporarily) Does a trigger need to do anything more than raise an exception to properly emulate our use of a check here? (i.e. is there an attendant code change required to make this transition complete?) I don't mind throwing some code/testing at it once the end of year pressure is off, but I want to make sure I fully understand what needs doing rather than going off blindly in this direction or that.

[Galen Charlton (gmc)]

It would be sufficient for the trigger to throw an exception.

[Jason Boyer (jboyer)]

I found some spare tuits on the shelf while a database rebuilds and have a branch to test here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1506534_z3950_attr_check

It removes the check, replaces the existing function with a trigger function, and applies a constraint trigger to the table so it can be deferred. It worked as expected for me and a simple test method is described below. I didn't know if this warranted a pgtap test since it's so basic (Could be used as an example of the simplest possible constraint trigger impl). I can work on a simple test, but I wanted to get this out there in case anyone wanted to throw some manual testing at it.

Testing:
Pre-patch:
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr) VALUES ('Title2', 5, 5); -- Sucess, z3950_attr_type is null, z3950_attr defined
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr_type) VALUES ('Title3', 5, 'title'); -- Success, z3950_attr_type exists in config.z3950_attr seed data
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr_type) VALUES ('Title4', 5, 'sadface'); -- Failure! z3950_attr_type does not exist in config.z3950_attr seed data

Post-patch:
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr) VALUES ('Title2', 5, 5); -- Sucess, z3950_attr_type is null, z3950_attr defined, trigger isn't called in this case.
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr_type) VALUES ('Title3', 5, 'title'); -- Success, z3950_attr_type exists in config.z3950_attr seed data
INSERT INTO config.z3950_index_field_map (label, metabib_field, z3950_attr_type) VALUES ('Title4', 5, 'sadface'); -- Failure! z3950_attr_type does not exist in config.z3950_attr seed data. If in a transaction, does not fail until COMMIT;

[Jason Boyer (jboyer)]

Do to my inability to get git (someday I'll remember that commit -a is not like add . ; commit) there was no upgrade script to test initially. Now there's an upgrade script, 002.schema... is updated, and there's a pgtap test that has been force-pushed to that branch.

[Ben Shum (bshum)]

Pushed to master, but added a small followup commit to fix a typo in the 002.schema file

"z3950attr_name_is_valid" should be "z3950_attr_name_is_valid" with an underscore between.