biblio.record_entry.vis_attr_vector not updated upon adding a locate URI

Bug #1730758 reported by Galen Charlton on 2017-11-07
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned

Bug Description

If a bibliographic record that has no reason to be OPAC-visible (e.g., not transcendent, no copies, etc.) is edited to add an 856 field specifying a located URI, vis_attr_vector on the bib is not updated to reflect the change in visibility.

Evergreen 3.0.1

Galen Charlton (gmc) on 2017-11-07
Changed in evergreen:
milestone: none → 3.0.2
importance: Undecided → Medium
summary: - biblio.record_entry.vis_attr_cache not updated upon adding a locate URI
+ biblio.record_entry.vis_attr_vector not updated upon adding a locate URI
Mike Rylander (mrylander) wrote :

Without looking at the code, I suspect we're blindly appending to the vector, but that doesn't work well when it's currently NULL. I can look tomorrow morning.

Andrea Neiman (aneiman) wrote :

Confirmed, saw this on a test system running 3.0.0

Changed in evergreen:
status: New → Confirmed
Changed in evergreen:
milestone: 3.0.2 → 3.0.3
Mike Rylander (mrylander) wrote :

I've tracked this down to the visibility caching trigger. Here's a branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-1730758-luri_vis_cache

From the commit:

This commit simplifies the handling of Located URI call numbers by separating
them from normal call numbers. This will allow proper updating of bib-level
visibility attributes for all actions that may happen to a Located URI.

We also insure that we don't return a NULL INT[] as an attribute set for
either copies or bibs. This was always impossible for copies, but was
possible for bibs. Now both are future-proof.

The upgrade script contains a post-transaction command to forcibly update the
visibility attributes of all bibs that make use of Located URIs. It may take
a while to run on large datasets. If it it running too long, it can be
canceled and the following psql commands will create and run a script that
will perform the same action serially over time without blocking writes to
bibs:

\t
\o /tmp/luri_visibility_update.sql
SELECT 'UPDATE biblio.record_entry SET ' ||
  'vis_attr_vector = biblio.calculate_bib_visibility_attribute_set(id) ' ||
  'WHERE id = ' || id || '; SELECT ' || id || ';'
FROM biblio.record_entry
  WHERE id IN (
            SELECT DISTINCT cn.record
              FROM asset.call_number cn
              WHERE NOT cn.deleted
                    AND cn.label = '##URI##'
                    AND EXISTS (
                        SELECT 1
                          FROM asset.uri_call_number_map m
                          WHERE m.call_number = cn.id
                    )
        );
\o
\t
\i /tmp/luri_visibility_update.sql

It will output the id of each updated bib so that the script can be killed
and then edited to remove completed bibs. The remainder can be run at a
later time.

NOTE: When the internal flag 'ingest.reingest.force_on_same_marc' is enabled,
we do NOT update the bib's visibility attributes, as doing so causes a loop
and an eventual trigger stack violation. This flag should ONLY be used when
forcing reingest of record attributes (NOT visibility attributes), search,
facet, and display fields, so if using this flag under normal operation,
proceed at your own risk and know that Located URI and bib source changes
will not be reflected in the visibility attributes of the record.

tags: added: pullrequest
Jeff Davis (jdavis-sitka) wrote :

I applied Mike's fix on a 3.0.2 test server. Previously, I had bib records with located URIs which had a null vis_attr_vector; after running the upgrade script, that field was now populated. In addition, manually setting a record's vis_attr_vector to null and then adding a located URI to the record caused the vis_attr_vector to be correctly (re)populated.

I still can't retrieve these records in the public OPAC, as noted in bug 1736419, but the fix does appear to resolve the narrower issue described in this bug.

Michele Morgan (mmorgan) wrote :

Echoing a comment from lp 1736419 here:

I have applied the patch to an upgraded system and am seeing the following for a record with a located uri:

In the staff client:

- The record is retrieved in the scope of the located uri
- The record is retrieved in the cons scope
- The record is retrieved in another branch's scope but the link does not display

In the public catalog:

- The record is retrieved in the scope of the located uri
- The record is retrieved in the cons scope
- The record is not retrieved in another branch's scope

This behavior is the same for existing, new, and edited records.

The problem I see with the patched behavior is the staff client behavior where the record appears in a scope that does not match the located uri. This will result in a lot of irrelevant hits in scoped searches. Otherwise, the visibility is behaving as expected.

We have the opac.located_uri.act_as_copy flag enabled.

Michele Morgan (mmorgan) wrote :

Adding a note to the above comment. When the record is retrieved in the cons scope, the located uri should display in the record, but no link displays, the bib record appears empty.

Michele Morgan (mmorgan) wrote :

Adding a clarification to the above comments that the visibility of the located uri in the record at the cons scope is directly related to the opac.located_uri.act_as_copy flag.

With the flag set to TRUE, the link in the record displays at the cons scope.

With the flag set to FALSE, the link in the record does not display at the cons scope.

The link display behavior at the cons level is consistent with previous releases.

Michele Morgan (mmorgan) wrote :

Adding a needsrepatch tag to address the staff client behavior of all records with located uris being retrieved out of scope. This can result in highly diluted search results in the staff client.

tags: added: needsrepatch
removed: pullrequest
Kathy Lussier (klussier) wrote :

I finally had a chance to apply this patch to a test system running clean master on a new database.

As Jeff noted, the patch does seem to address the narrow issue identified in this bug. It also addresses a small part of the search issues I identified in bug 1736419, but there are still significant search issues with this patch applied:

1. I still cannot retrieve any records in the public catalog.
2. Searching a record with located URIs for BR1, BR2 & BR3 in the web client, we fixed the issue where a search scoped to BR4, SYS1 or SYS2 was retrieving the records. However, I'm still able to retrieve it with the scope set to the Consortium.
3. I see no difference in which records are retrieved when the opac.located_uri.act_as_copy global flag is enabled.

I can outline my test results more thoroughly on bug 1736419. At this point, though, I'm thinking these two issues are separate bugs and that Mike's current code should be merged since it does fix the issue with vis_attr_vector not updating as expected. However, Mike, if you think it's better to fix both issues with the same branch, let me know and I'll hold off on merging it.

Needless to say, we consider this issue to be fairly critical. We made a decision today to hold off on a 3.0 upgrade scheduled for next week due to the problems identified in bug 1736419 and in bug 1736992.

Kathy Lussier (klussier) wrote :

We've found another instance where biblio.record_entry.vis_attr_vector is not updating. If you add or change a bib source when either editing or creating a MARC record, this field does not update. Existing records with a bib source will get a value in this field as a result of the 3.0 upgrade script, but these values don't change with any future changes of the bib source.

Here are some further notes from mmorgan:

On insert or update on biblio.record_entry, the function asset.cache_copy_visibility() should run
asset.cache_copy_visibility() says that if the table that changed is biblio.record_entry, then run the function biblio.calculate_bib_visibility_attribute_set(id). That function should update biblo.record_entry.vis_attr_vector
But it looks like that if loop pertaining to a change in record_entry is stuck inside an if loop pertaining to a change in copy or unit. So I don't see that the part that updates biblo.record_entry.vis_attr_vector is ever getting called

Mike Rylander (mrylander) wrote :

Michele,

Looking at XXXX.function.luri_vis_cache.sql in working/user/miker/lp-1730758-luri_vis_cache, each of the ELSIF's at lines 92 and 189 that tests the TG_TABLE_NAME variable are peer branches of IF's that separates out actions on (copy|unit), call_number, and record_entry.

Can you confirm that the version you're using in this test is exactly the same as the one in that file? Or, are you referring to some other part of that function?

Thanks!

Mike Rylander (mrylander) wrote :

Ah ... I see what's happening here. No need to check that, I know what's going on and will address it shortly.

Mike Rylander (mrylander) wrote :

I've force-pushed an update to the branch to address this, but the substantive change is simple to apply by hand. Just run the following:

DROP TRIGGER z_opac_vis_mat_view_tgr ON biblio.record_entry;
CREATE TRIGGER z_opac_vis_mat_view_tgr BEFORE INSERT OR UPDATE ON biblio.record_entry FOR EACH ROW EXECUTE PROCEDURE asset.cache_copy_visibility();

The important part of that is that we need to use a BEFORE trigger rather than an AFTER trigger.

Kathy Lussier (klussier) on 2018-01-09
tags: added: pullrequest
removed: needsrepatch
Kathy Lussier (klussier) wrote :

Thanks Mike! After some continued digging, I think my initial assessment of the bib source problem was wrong.

I think biblo.record_entry.vis_attr_vector was updating when the record saved, but the problem was actually that biblo.record_entry.vis_attr_vector updates before the source updates. The vector, therefore, is based on the previous value in the source field, not on the new one that's being applied.

In my original testing, I probably only edited each record once. It therefore appeared as if the value wasn't changing but it was just calculating the visibility based on the wrong information. In the case of the Concerto records, it was looking at the old null value in the source field and maintained the null value in the vis_attr_vector field. When I tested it on the upgraded database, the vis_attr_vector stayed the same because the calculation was using the same source that was on the record during the upgrade.

I applied the changes from your new branch, and this problem continues. On my test record, I did the following:
1. Changed the bib source of a record from oclc to gutenberg. vis_attr_vector updated to {268435457}
2. Running the biblo.calculate_bib_visibility_attribute_set function against that same record, I get {268435459}
3. I then changed the bib source of the same record to local system. vis_attr_vector updated to {268435459} (the result we got when running the calculation with the previous source.)

If you continue changing the source, you'll continue to see the same pattern.

Mike Rylander (mrylander) wrote :

Kathy,

My previous fix was just a wee bit short of complete. The problem is that the asset.cache_copy_visibility() trigger is called before the update has hit the table, and biblio.calculate_bib_visibility_attribute_set() reads the table to get the source value ... which is not yet updated. Doh! However, we /do/ need the trigger to run in BEFORE mode so that it can just adjust the NEW row rather than using UPDATE, or we get an update loop.

So, you will find a new, force-pushed version of the branch which accounts for this. It does so by providing a new version of biblio.calculate_bib_visibility_attribute_set() that can accept the value of NEW.source as an optional parameter, avoiding the read of the (soon-to-be) stale bre row.

The XXXX upgrade script can simply be run again to get the new code in place, and the baseline is updated to match the upgrade script now.

I was seeing the behavior you describe before re-running the upgrade script (always one source behind), and it is properly updating the vis_attr_cache column after re-running it.

Thanks!

Kathy Lussier (klussier) wrote :

Thanks Mike!

The new patch mostly works for me except in one case.

If I edit an existing record with a null bib source and null vis_attr_vector and apply a new source, vis_attr_vector remains null.

In my testing, vis_attr_vector updates appropriately if I'm creating a new record or if I am editing a record that previously had a value in the bib source field.

Also, after running the upgrade scripts, I noticed the old, stale values did not recalculate as part of the upgrade. Should we forcibly update records that contain a bib source as we're doing with the Located URIs? I know it will make for a long-running upgrade script, but I imagine that sites already on 3.0 have incorrect values in this field for any new or edited records that need to be recalculated.

Mike Rylander (mrylander) wrote :

Thanks, Kathy. I am addressing the null-to-value issue now (just need to test a bit differently for a changed source) and I will modify the batch update to include source-y records. Update coming soon.

Mike Rylander (mrylander) wrote :

The branch is now updated. Thanks again for testing, Kathy!

Kathy Lussier (klussier) wrote :

I think we've got it! This tested fine when using the upgrade script. I just want to test it on a clean install before merging the fix.

Thanks for your work on this!

Kathy Lussier (klussier) wrote :

It all looks good Mike! I amended the commit message to include the source-y records in the script (the one to run if you cancel the upgrade script before it's done) and merged the code to master and release 3.0.

Thanks again!

Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers