Autorenewals should not be attempted on circs with no autorenewals allowed

Adding a link to relevant IRC discussion:

http://irc.evergreen-ils.org/evergreen/2019-07-09#i_411573

Adding the NULL check to the filter is most efficient way to handle this. Maybe this is a documentation / sample data issue?

A secondary check in the code to mark such events invalid probably wouldn't hurt, though.

I'd like to suggest a slight tweak for the second case (above) where auto_renewal_remaining = 0. We should teach the AutoRenew reactor to bypass the actual circulation back-end call, but still generate the notification.

As I understand it, autorenew batches can take some time to process. This would allow the server to avoid a lot of unnecessary work processing items that are known in advance to be non-auto-renewable.

Perhaps we should have a dedicated hook for autorenewals which includes the NULL check?

Regarding the second case, I like the idea of teaching the reactor to bypass the autorenew attempt and just send a notification when auto_renewal_remaining = 0

My thoughts as of right now:

1. Modify the validator to look for auto_renewal_remaining is not null.

2. Modify the AutoRenew reactor to check for auto_renewal_remaining = 0, bypass the checkout, and generate the MAX_RENEWALS_REACHED event on its own.

An alternative to #1 in my previous comment occurred to me. We could also check for auto_renewal_remaining of NULL in the reactor and set the reason to something like autorenewal not allowed. We could even make a new event for this.

Doing this will notify the patron that the item can't be autorenewed. This might be preferable to just setting the event state to invalid and then not notifying the patron.

What's the consensus?

I am necro-bumping this bug because I stand by my previous comment even more today. We use the autorenew events as both autorenewal and courtesy notifications for those things that can't be renewed.

I think we should either make the renewal_remaining and auto_renewal_remaining columns "NOT NULL DEFAULT 0" or add code to the AutoRenew.pm handler function to set the variables to 0 when the values from the database are null.

Nothing like having a 50% failure in the autorenew process to get one to pay attention to what's going on.

I have assigned this bug to myself and changed the summary/title to reflect what I think the real problem is. I consider this an architecture implementation bug. In my opinion and the opinion of many others, numeric fields should not be nullable, unless null conveys some useful information that zero does not also convey. In the case of the renewal remaining fields in circulation, null is equivalent to zero.

I'll have a patch commit to fix the circulation table and cir rule tables (if needed) later this week.

I'm updating this bug to report that I have made some progress. I have completed the necessary database schema changes and updated the affected tests and Concerto data load.

I still need to test the changes while using the staff client to make sure that new circulation parameters and new circulations are created correctly.

I have pushed a working branch that is ready for more eyes. It does the following:

1. Sets the auto_renewal_remaining fields of the circulation and aged_circulation tables to not allow nulls.

2. Sets the max_auto_renewals column on config.rule_circ_duration to not allow nulls.

3. Updates the IDL to indicate that the above columns are required, along with the max_renewals column on config.rule_circ_duration.

4. Updates the concerto data load so that the required columns get values.

5. Updates the pgtap and Perl tests as appropriate for the schema change.

6. Adds a regression test to make sure that that columns do not allow null values again in the future.

The branch is working/user/dyrcona/lp1835953-auto-renewal-remaining-not-null: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1835953-auto-renewal-remaining-not-null

Thanks for this patch, Jason -- particularly the tests and docs! On first glance, it looks good. My only concern is that adding the NOT NULL constraint to such a large table as action.circulation in the upgrade script could cause a long ACCESS EXCLUSIVE lock.

I was trying to find some good documentation on the topic. This page from squawk seems okay: https://squawkhq.com/docs/adding-not-nullable-field

They note that on postgres 11 and higher, if you supply a default value, it doesn't have to do a full table scan, which should speed up the amount of time it hangs on to that lock. They also suggest a workaround of the ADD CONSTRAINT... VALIDATE CONSTRAINT dance.

I will do timed tests of the database upgrade using my original script and one modified to use the "not valid" constraints. This will take a few days to a week or so since I'll have to reload the database in between tests. I will test on PostgreSQL versions 10 and 16 since I have those set up on my development db server with multiple copies of production data.

Jane,

I implemented an alternate upgrade script based on the article that you mention in comment #10. I have attached it here.

I have runs some tests on a test database server with production data on both Pg10 and Pg16. The Pg10 database is at Evergreen version 3.10.3 and the Pg16 database was upgraded to main as of commit 991e1664c3.

The Pg16 database has optimized settings for the hardware and the Pg10 database uses the PostgreSQL default settings. I think this is OK because what I'm testing are the differences in run time between the two scripts and not differences in Pg versions.

I then ran this alternate db upgrade and the original against those databases using the time program to output how long they took to complete. My methodology with as follows:

1. Load a dump of the database to be tested.
2. Run a vaccuum analyze on the full database.
3. Run one of the alternate upgrade scripts on the database with time.
4. Record the results in a Google sheet, see below.
5. Repeat steps 1-4 with the other db upgrade alternative.

No other updates or queries were communicating with the database server during the tests.

My results from 1 run each of the alternatives are in this Google shseet: https://docs.google.com/spreadsheets/d/1l7PsPUhTpT2C4eI2J9CIpGup3Zbn_OSbLddumbTk7Wg/edit?usp=sharing

In my testing, the "not valid" constraint was about 12 minutes faster than the "not null" constraint, but still took almost 3 hours, on the optimized Pg16 database.

On the unoptimized Pg10 database the "not null" version of the upgrade was faster by 26 minutes.

Besides the performance of the upgrade scripts there are other factors to consider in changing from a "not null" to a "not valid" constraint. One of these is that the vast majority of our current constraints are "not null." There are presently only a couple of "not valid" constraints in the code. Another is that using a "not valid" constraint could make it harder for an automated tool to determine if a field should be required or not.

I also understand that running each db upgrade one time is not particularly scientific. They ought to be run thousands of times and the results averaged. However it takes approximately 1 working day to get results given that a pg_restore has to happen in between the tests and the tests themselves run for several hours.

I think we should open this up to some discussion to decide which approach to the constraints is preferred.

Jason

I have removed the needsdiscussion tag and added the pullrequest tag.

I have rebased the branch on main and fixed up the release notes correction commit so this is all in 1 commit at the tip of working/user/dyrcona/lp1835953-auto-renewal-remaining-not-null-rebase (https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1835953-auto-renewal-remaining-not-null-rebase).

I suggest that we go with the "NOT NULL" constraints to be consistent with the majority of the constraints in the schema. If anyone wants to discuss that and argue in favor of the 'NOT VALID' constraints, then let's have that conversation, but I'm afraid that "needsdiscussion" has been the death too many patches, so I'd rather this just get some attention and possibly go in.

A belated thank you for collecting all this data, Jason!

I think NOT NULL makes sense. Could we perhaps just include a warning in the release notes that this migration blocks reads/writes on action.circulation for a long time, particularly on old postgres versions?

After today's (Monday, August 25 2024) code review session, I've decided to remove the pullrequest tag and come up with a different approach that does less in the database. It turns out that my changes may affect some site's filters on this event.

It may be that adding a filter to the example/default filters that come with Evergreen could be enough to resolve this.

I forgot about this one with everything that has been going on lately. I also completely forgot what approach I was planning to take after the August 25 meeting.

I'm removing myself from this bug to free it up for someone else who wants to have a look.

I think the approach I decided to take was to modify the Perl code to check for null values coming from the database. This would involve no schema changes.

Adding a link to an IRC discussion relevant to this bug:

http://irc.evergreen-ils.org/evergreen/2025-05-05#i_577467

Even with the AT filter, somehow, on a complex production environment, we're seeing auto-rewnewals being attempted upon circulations where auto_renewal_remaining is a null value.

I understand that this discussion is widely about the database etiquette around integer columns being nullable and they shouldn't be.

I think I'd argue that should be a different bug. This bug, as described is about the auto-renew feature as-written is targeting circulations that it should be targeting.

The hook is checkout.due and with the suggested filter, we're still seeing Evergreen processing circulations that it shouldn't. I think the JSON filter is a reasonable solution, and perhaps we need to include the filter and update the suggested cron job for autorenewals to invoke the stock JSON filter.

Here's the patch that double checks the circulation to make sure it should be reacting:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/lp1835953_autorenew_circ_check

I've tested this patch and it works. Though it does create an event row in action_trigger.event, it does not* attempt an auto-renewal for circulations where auto_renewal_remaining is null.

Some of us have taken advantage of this "bug" as a feature. As it is, the Autorenew event can be used for both auto-renewal and a pre-due courtesy notice with the proper wording in the AutorenewNotify template.

The commit mentioned in Blake's immediately previous comment breaks that workflow. It would be far better to get a working filter that sites can use to skip auto-renewal on these circulations, and I do not think it should be the default filter, either.

In terms of precedent, there are other events that may collect events which are ultimately invalid.

I'm opposed to using the validator to call these events invalid, since that will also break our combined notification workflow.

I accede to the nullability of the auto_renewal_remaining field being a different bug, and I can see how some would view it as a feature, meaning that auto-renewal is not allowed. However, I'd argue that the renewal_remaining field does not work this way.

We've been running this short circuit patch in production since May. It has solved the issue for us.

Jason: I'm trying to understand how and why a library would want to setup an action trigger for circulations that have null auto_renewal_remaining. And specifically a circulation with a null value for that field? The logic is: "Send a notice to the patron that their item is almost due and it will not* be auto renewed"?

Could that be accomplished with a custom at_filter? Instead of traversing the AutoRenew.pm code? In my mind, that code is executed for the sole purpose of (auto)renewing a circulation, and not sending email/texts notifications.

In fact, the branch above only* messes with the logic inside the code that performs the renewal. Therefore, how would a notification AT traverse this code path in the first place?

This is the custom filter that we used prior to using this short circuit branch:

{
    "checkout.due" : {
        "context_org" : "circ_lib",
        "filter" : {
            "checkin_time" : null,
            "-or" : [
                { "stop_fines" : ["MAXFINES", "LONGOVERDUE", "CLAIMSRETURNED"] },
                { "stop_fines" : null }
            ],
            "-not" : [ { "auto_renewal_remaining" : null } ]
        }
     }
}

We were still getting auto renewed circulations for circulations that had a null value for auto_renewal_remaining. It doesn't seem like they system should have included those but it did. Maybe due to group_field? Perhaps the same patron had an autorenewal item as well as a non-autorenewal item, and they were fed into AutoRenew. Or maybe that JSON filter is flawed.

Blake, since there are uses in the wild that make use of the NULL state of the field, I think we need to find a way to allow that.

Your filter is more complicated than it needs to be and may well be logically flawed because of that, since you can't be sure that additional conditions aren't added to the OR list created by the JSON array that you've added under the NOT boolean operator.

Instead of:

 "-not" : [ { "auto_renewal_remaining" : null } ]

how about just:

 "auto_renewal_remaining" : { "<>" : null }

which translates directly to "circ.auto_renewal_remaining IS NOT NULL". To ignore both 0 and NULL auto_renewal_remaining values, you could use a coalesce transform to turn NULL into 0 and test that:

 "auto_renewal_remaining" : { ">" : { "transform" : "COALESCE", "params" : [ 0 ], "value" : 0 } }

which translates to "COALESCE(circ.auto_renewal_remaining,0) > 0".

Alternatively, it seems like it would be trivial to add an event parameter, say "null_auto_remaining_same_as_zero", that could be checked by the validator. Though as Jason says, that seems wasteful.

Blake,

See my previous comment (https://bugs.launchpad.net/evergreen/+bug/1835953/comments/20).

We've combined the autorenewal and courtesy notices into 1 a/t using the Autorenew event. If circulations with no autorenewals are excluded, then we lose the courtesy notice feature. This is very easily done with a rewording of the AutorenewNotify email template.

Why run two triggers when 1 will do?

HtH,
Jason

Thanks for all the time and consideration!

I'll give the new JSON filter idea a whirl. Maybe that's all this bug amounts to!

-Blake-