Timestamp on dob can make date appear off by a day

[Bill Ott]

A discrepancy on timezone between client and server can make a birthdate created as '2011-01-01 00:00:00-4' end up as '2010-12-31 23:00:00-5' server side. This can make the dob appear as 2010-12-31 or 2011-01-01 depending on where it's viewed.

As a temporary work around, I'm adding 1 hour in register.js, to allow for the difference. This makes some dob's appear with a time of 01:00:00-05, but at least its on the same day, and at a time that's rarely an issue for patron interactions.

I can't think of an argument for the the dob not being a "date" type, instead of "timestamp with timezone".
I've identified the following functions and views that would need to be dropped/created to facilitate this change, and have done very minor testing, with no ill effects.
  FUNCTION auditor.create_auditor_lifecycle
  VIEW reporter.demographic
  VIEW action.all_circulation

[Jason Etheridge (phasefx)]

> As a temporary work around, I'm adding 1 hour in register.js, to allow
> for the difference.  This makes some dob's appear with a time of
> 01:00:00-05, but at least its on the same day, and at a time that's
> rarely an issue for patron interactions.

I have vague recollection of adding 8 or 12 hours; maybe in the original editor?

-- Jason

[Michael Peters (mrpeters)]

Hi Bill,

We reported a similar (if not exact same) issue to our support vendor in January 2011. This was the response, in hopes it helps to resolve this bug.

"In short, older versions of Evergreen are flawed, but the flaws weren't made apparent until we started getting more consortia with branches spread across timezones. Trying to backport these fixes (to 1.6) amounts to development work; a good bit of it.

A simple example of a date handling bug:
Database sends a timestamp over the internet as a textual string:
"2011-01-01 00:00:00-0500"

Staff client naively displays the first 10 characters:
"2011-01-01"

This can only be correct if the staff client happens to be in the -0500 timezone.

If for some reason, the database sends this equivalent timestamp:
"2010-12-31 23:00:00-0600"

Then the client will incorrectly show:
"2010-12-31"

Newer versions of the code do not do this."

It seems, however, that the "newer versions" haven't totally resolved the problem. We're on 2.0.4 and we find we still have to make sure birth dates don't get "too close" to midnight. An issue came up last week with a new migration where 'dob' was loaded simply as '2011-01-01' rather than '2011-01-01 11:00:00-5' and since we have libraries in 2 time zones in Indiana different locations were seeing a different DOB.

[Michael Peters (mrpeters)]

A bit more background here, as well...

"Alright, this is what we know. Older versions of the staff client didn't handle timezones very well. There are a lot of moving parts and changesets here, and it's probably not feasible to patch things piecemeal. The changesets we involved ourselves previously in this ticket would affect the newer patron editor, but not the one Indiana is actually using (nor would it help with the patron summary sidebar). In the past, bad timezone handling may not have mattered because of the actual timezone being used when sending dates over the wire. However, now we're seeing cases like

1960-07-11T00:00:00-0400 being represented as 1960-07-10T23:00:00-0500, which while mathematically correct, runs afoul of the bugs in the older code (where dates are, for example, treated as text strings and truncated to the first 10 characters).

The dates and timezones on the servers involved all seem correct, and I don't know why Indiana's older brick presents the problem dates one way and the newer bricks present them another.

So barring upgrades, or any insight on middle layer/server timezone handling, one kludge I can think of would be to set a trigger on the actor.usr table to intercept updates to the date of birth field, and change the time component of such dates to mid-day. Such a timestamp would be resistant to change of day rollovers based on time zone."

With that, we then created the trigger attached here, though this may not be needed on 2.x. It's also possible we have some misconfiguration so that the database is seeing the wrong timezone.

[Mike Rylander (mrylander)]

Some /very minor and inconclusive/ testing shows that simply changing that field to a date (and doing /nothing else/, such as changing the fm_IDL.xml) MIGHT be enough to squash this forever. HOWEVER, lots of testing would be required, and it may be that we will, indeed, need to teach lots of other parts of the system about the DATE type. That, of course, is the most correct solution, and the source of the above referenced "good bit of development work" that Michael Peters pasted from ESI's internal support system.

To wit:

# select '2011-09-01 12:44:16.602564-04'::date;
    date
------------
 2011-09-01
(1 row)

This shows that we should be able to pass a timestamptz to Postgres and it will cast appropriately. I have not tested the other direction (in particular, cstore, which uses libdbd-pg to do some timezone mangling), though.

Someone wanting to test this could issue something along the lines of:

BEGIN;

DROP VIEW action.all_circulation;
DROP VIEW reporter.demographic;
DROP VIEW auditor.actor_usr_lifecycle;

ALTER TABLE actor.usr ALTER dob TYPE date USING dob::date;

CREATE VIEW auditor.actor_usr_lifecycle AS
         SELECT (-1), now() AS audit_time, '-' AS audit_action, usr.id, usr.card, usr.profile, usr.usrname, usr.email, usr.passwd, usr.standing, usr.ident_type, usr.ident_value, usr.ident_type2, usr.ident_value2, usr.net_access_level, usr.photo_url, usr.prefix, usr.first_given_name, usr.second_given_name, usr.family_name, usr.suffix, usr.alias, usr.day_phone, usr.evening_phone, usr.other_phone, usr.mailing_address, usr.billing_address, usr.home_ou, usr.dob, usr.active, usr.master_account, usr.super_user, usr.barred, usr.deleted, usr.juvenile, usr.usrgroup, usr.claims_returned_count, usr.credit_forward_balance, usr.last_xact_id, usr.alert_message, usr.create_date, usr.expire_date, usr.claims_never_checked_out_count
           FROM actor.usr
UNION ALL
         SELECT actor_usr_history.audit_id AS "?column?", actor_usr_history.audit_time, actor_usr_history.audit_action, actor_usr_history.id, actor_usr_history.card, actor_usr_history.profile, actor_usr_history.usrname, actor_usr_history.email, actor_usr_history.passwd, actor_usr_history.standing, actor_usr_history.ident_type, actor_usr_history.ident_value, actor_usr_history.ident_type2, actor_usr_history.ident_value2, actor_usr_history.net_access_level, actor_usr_history.photo_url, actor_usr_history.prefix, actor_usr_history.first_given_name, actor_usr_history.second_given_name, actor_usr_history.family_name, actor_usr_history.suffix, actor_usr_history.alias, actor_usr_history.day_phone, actor_usr_history.evening_phone, actor_usr_history.other_phone, actor_usr_history.mailing_address, actor_usr_history.billing_address, actor_usr_history.home_ou, actor_usr_history.dob, actor_usr_history.active, actor_usr_history.master_account, actor_usr_history.super_user, actor_usr_history.barred, actor_usr_history.deleted, actor_usr_history.juvenile, actor_usr_history.usrgroup, actor_usr_history.claims_returned_count, actor_usr_history.credit_forward_balance, actor_usr_history.last_xact_id, actor_usr_history.alert_message, actor_usr_history.create_date, actor_usr_history.expire_date, actor_us...

[Dan Wells (dbw2)]

Here is another wrinkle to be aware of. It doesn't change the fact that we should simply be using a DATE type, but it might affect some other areas where dates and timezones enter in. Try this code on a page with dojo loaded (you can try it easily here - http://dojo-sandbox.net/ ):

dojo.require("dojo.date.stamp");
alert(dojo.date.stamp.toISOString(new Date(2011,2,1)));
alert(dojo.date.stamp.toISOString(new Date(2011,2,21)));
alert(dojo.date.stamp.toISOString(new Date(1979,2,21)));

The three alerts I get (I am in EST/EDT) are:

2011-03-01T00:00:00-05:00
2011-03-21T00:00:00-04:00
1979-03-21T00:00:00-04:00

The first two cross the DST boundary, hence the change in timezone, and are correct. The last, however, is wrong. In 1979, DST did not start until the end of April, so it should read '1979-03-21T00:00:00-05:00'. toISOString() is incorrectly applying current DST rules to past dates. I have not looked into whether this is a dojo bug or a bug in the underlying Mozilla toISOString(), but I suspect the latter.

Postgres, on the other hand, handles the local timezone quirks correctly, and therefore "corrects" this incorrect data. If I set the 'dob' column to either of the top two, they store as entered, as they 'make sense' in my local context. If I try to set the third, Postgres recognizes that the given day and timezone combination would have never occurred historically at my location, and therefore needs adjustment. Of course, we end up with '1979-03-20 23:00:00-05' getting stored in the DB.

The takeaway here is that we cannot currently rely on toISOString to create locally correct strings for past dates. Thankfully, most of our day-to-day date needs are dealing with present and future dates.

[Thomas Berezansky (tsbere)]

For what it is worth, I get the same 3 alerts across multiple browsers on my windows system, so it isn't Mozilla/Firefox.

It also happens in Firefox with Windows and Linux, it appears, so it isn't Windows.

Thus I would say it is an issue with the javascript layer in general or an issue with dojo. Dunno which.

[Dan Wells (dbw2)]

Some unfortunate information directly from the ECMA standard:

"The implementation of ECMAScript should not try to determine whether the exact time was subject to daylight saving time, but just whether daylight saving time would have been in effect if the current daylight saving time algorithm had been used at the time. This avoids complications such as taking into account the years that the locale observed daylight saving time year round.

If the host environment provides functionality for determining daylight saving time, the implementation of ECMAScript is free to map the year in question to an equivalent year (same leap-year-ness and same starting week day for the year) for which the host environment provides daylight saving time information. The only restriction is that all equivalent years should produce the same result."

So, Javascript (ehem, I mean ECMAScript) Date objects are half-baked for anything but the recent past.

http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf

[Thomas Berezansky (tsbere)]

For the "change the column type to date" fix....

I think that alone makes the problem *worse* in the client. I did that on our training system, then added my DOB to it (because it wasn't there previously). End result was it showing the DOB as the day before in the client. Dojo interfaces or otherwise.

The database, on the other hand, seems to have no problems with it and has the correct value there.

Thus, if that is to be the fix, a number of places in the system may need to be changed.

I also tried timestamp without time zone and got the same results (predictably, as dates have no timezone either).

[Robert J Jackson (rjackson-deactivatedaccount)]

Our time zone recently changed and this appears to still be an issue.

Example: through the staff client set the DOB to 1956-08-18 and the dob column in actor.usr ends up with:
 dob | 1956-08-17 23:00:00-05

[Jason Etheridge (phasefx)]

It looks like how we use dates in Template Toolkit is also susceptible to this. I added the following to home.tt2 as a test:

            hello world:
            <br/>ctx.parse_datetime('1967-04-02 23:00:00-05') = [% ctx.parse_datetime('1967-04-02 23:00:00-05') %]
            <br/>ctx.parse_datetime('1967-04-03 00:00:00-04') = [% ctx.parse_datetime('1967-04-03 00:00:00-04') %]
            <br/>date.format(ctx.parse_datetime('1967-04-02 23:00:00-05'), '%m/%d/%Y') = [% date.format( ctx.parse_datetime('1967-04-02 23:00:00-05'), '%m/%d/%Y') %]
            <br/>date.format(ctx.parse_datetime('1967-04-03 00:00:00-04'), '%m/%d/%Y') = [% date.format( ctx.parse_datetime('1967-04-03 00:00:00-04'), '%m/%d/%Y') %]
            <br/>date.format('1967-04-02 23:00:00-05', '%m/%d/%Y') = [% date.format( '1967-04-02 23:00:00-05', '%m/%d/%Y') %]
            <br/>date.format('1967-04-03 00:00:00-04', '%m/%d/%Y') = [% date.format( '1967-04-03 00:00:00-04', '%m/%d/%Y') %]

and it rendered like so:

hello world:
ctx.parse_datetime('1967-04-02 23:00:00-05') = 23:00:00 02-04-1967
ctx.parse_datetime('1967-04-03 00:00:00-04') = 00:00:00 03-04-1967
date.format(ctx.parse_datetime('1967-04-02 23:00:00-05'), '%m/%d/%Y') = 04/02/1967
date.format(ctx.parse_datetime('1967-04-03 00:00:00-04'), '%m/%d/%Y') = 04/03/1967
date.format('1967-04-02 23:00:00-05', '%m/%d/%Y') = 04/02/1967
date.format('1967-04-03 00:00:00-04', '%m/%d/%Y') = 04/03/1967

Both of these dates are identical, but expressed with different timezones.

[Bill Erickson (berick)]

I was getting the off-by-one day issue when testing this as well. I tracked it (for starters, anyway) to how we were extracting dates from libdbi. Like JS, libdbi appears to assume that if a date has no timezone, it's GMT. So, we were reading a GMT date, then translating into local time, thus shifting it back by a day.

I've pushed a branch that contains the SQL changes Mike posted (plus one more that cropped up) and another commit changing how we read dates (without times) in oils_sql.c and oils_execsql.c. The end result being that the date stored in the DB is the date returned by cstore. It's at least a first step.

The Dojo patron editor and various interfaces in the browser client are showing the correct date for me w/ patrons whose DoB crosses the daylight savings boundary.

Incidentally, open-ils.storage is returning the correct date as well when called from srfsh without modification.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp838525-dob-as-date

[Bill Erickson (berick)]

Something else to consider when modifying the DoB column to be a date. It truncates the time/timezone information, which means two equivalent timestamptz's can be cast to different dates.

evergreen=# select '1984-03-17 23:00:00-08'::timestamptz = '1984-03-18 00:00:00-07'::timestamptz;
 ?column?
----------
 t
(1 row)

evergreen=# select '1984-03-17 23:00:00-08'::date = '1984-03-18 00:00:00-07'::date;
 ?column?
----------
 f
(1 row)

We have a number of patrons in our data set whose DoB is set with PST (-08) with the hour set to 23. This can happen when the date is set on the other side of the daylight savings boundary, using the current local timezone. E.g.

evergreen=# select '1947-03-12 00:00:00-07'::timestamptz;
      timestamptz
------------------------
 1947-03-11 23:00:00-08

It's not clear what Evergreen process would cause this, but a quick IRC survey suggests it's not that unusual to have some patrons whose DoB is stored this way. We may have to tailor the SQL upgrade script to find such dates and bump them forward a day when translating them from a timestamptz into a date.

[Thomas Berezansky (tsbere)]

Perhaps change

ALTER TABLE actor.usr ALTER dob TYPE date USING dob::date;

to

ALTER TABLE actor.usr ALTER dob TYPE date USING (dob + '12 hours')::date;

or similar?

[Bill Erickson (berick)]

Re: Thomas' comment above, as discussed in IRC, it would be better to use dob + '3 hours' (maybe 6?) instead of + '12 hours', since some patrons birth hour may be set to 12 as a way of manually avoiding the problems described in this ticket. 3 hours covers the daylight savings jitter, plus a little fudge room. Otherwise, I think this will solve the problem of some DoB's sitting just on the wrong side of the date boundary.

[Bill Erickson (berick)]

I pushed a commit to my branch (user/berick/lp838525-dob-as-date) to do the above. It also includes updated versions of all of the views that are recreated to make sure they pick up any columns that may have changed since Mike's original SQL, which is now 4 years old (wow). Updated defs come from psql \d+ output on a master database.

[Bill Erickson (berick)]

Now w/ pgtap test.

[Bill Erickson (berick)]

Did more testing. I have not been able to find a place where the DoB displays incorrectly. Cleaned up and force-pushed to the same branch. This might cover it... Adding pullrequest. Also, since there is no visible change in the client, I was not intending to write release notes, but can if requested.

Am I missing anything?

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp838525-dob-as-date

[Mike Rylander (mrylander)]

I'm about to sign off on this, but wanted to confirm that others don't want this to be backported. IMO, because the fix makes this an aesthetic issue, I prefer to avoid db churn in a minor upgrade.

Comments?

[Dan Wells (dbw2)]

+1 to no backport.
Makes sense to me.

[Bill Erickson (berick)]

+1 to no backport.

[Mike Rylander (mrylander)]

Signed off and pushed to master. Thanks, Bill!

[Ben Shum (bshum)]

Setting this back to unfinished...

When we went to run 0945 upgrade, we couldn't complete it due to problems with the DROP VIEW reporter.demographic

DETAIL: view reporter.classic_current_billing_summary depends on view reporter.demographic
view reporter.classic_current_circ depends on view reporter.demographic

It looks like because our database uses the extend-reporter views from PINES, it'll break without first dropping those and then re-creating them later.

See IRC logs: http://irc.evergreen-ils.org/evergreen/2015-10-09#i_208649

[Jason Stephenson (jstephenson)]

Trying to update a user also blows up after this change:

ERROR: type of parameter 39 (date) does not match that when preparing the plan (timestamp with time zone)
CONTEXT: SQL statement "INSERT INTO auditor.actor_usr_history ( audit_id, audit_time, audit_action, audit_user, audit_ws, id, card, profile, usrname, email, passwd, standing, ident_type, ident_value, ident_type2, ident_value2, net_access_level, photo_url, prefix, first_given_name, second_given_name, family_name, suffix, alias, day_phone, evening_phone, other_phone, mailing_address, billing_address, home_ou, dob, active, master_account, super_user, barred, deleted, juvenile, usrgroup, claims_returned_count, credit_forward_balance, last_xact_id, alert_message, create_date, expire_date, claims_never_checked_out_count, last_update_time )
                SELECT nextval('auditor.actor_usr_pkey_seq'),
                    now(),
                    SUBSTR(TG_OP,1,1),
                    eg_user,
                    eg_ws,
                    OLD.id, OLD.card, OLD.profile, OLD.usrname, OLD.email, OLD.passwd, OLD.standing, OLD.ident_type, OLD.ident_value, OLD.ident_type2, OLD.ident_value2, OLD.net_access_level, OLD.photo_url, OLD.prefix, OLD.first_given_name, OLD.second_given_name, OLD.family_name, OLD.suffix, OLD.alias, OLD.day_phone, OLD.evening_phone, OLD.other_phone, OLD.mailing_address, OLD.billing_address, OLD.home_ou, OLD.dob, OLD.active, OLD.master_account, OLD.super_user, OLD.barred, OLD.deleted, OLD.juvenile, OLD.usrgroup, OLD.claims_returned_count, OLD.credit_forward_balance, OLD.last_xact_id, OLD.alert_message, OLD.create_date, OLD.expire_date, OLD.claims_never_checked_out_count, OLD.last_update_time
                FROM auditor.get_audit_info()"
PL/pgSQL function "audit_actor_usr_func" line 3 at SQL statement
SQL statement "UPDATE actor.usr
SET profile = 2
WHERE id = usr_id
AND profile = 22"
PL/pgSQL function "mvlc_remove_privilege" line 29 at SQL statement

[Jason Stephenson (jstephenson)]

running select auditor.update_auditors() repairs the auditor damage.

That should be added as part of the upgrade script, IMHO.

[Bill Erickson (berick)]

Interestingly, I don't get the same error when updating a patron. (PL/pgSQL function "mvlc_remove_privilege"?). Updating the auditor is still clearly the right thing to do, though. I'll push a fix for that and the "reporter.demographic" issue Ben mentioned above. Thanks for the reports.

[Bill Erickson (berick)]

Repair branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp838525-dob-as-date-sql-repairs

The optional reporter views are only dropped and recreated if they are present to begin with.

[Jason Stephenson (jstephenson)]

In response to comment #25, that is a helper function that we wrote to remove elevated privileges from individual staff. We have a similar one to grant elevated privileges.

As you can see the offending statement was a simple update on the actor.usr to change the profile from 22 to 2. The error message spells out rather plainly that the problem was with the user's dob having a different datatype from the auditor table. This occurred on Pg 9.3, fwiw.

In response to comment #26, I'll see about giving your changes a go ASAP. Thanks, Bill!

[Jason Stephenson (jstephenson)]

oops. Stupid me. It happend on Pg 9.1.

[Bill Erickson (berick)]

It occurs to me that calling auditor.update_auditors() will simply truncate the DoB value as discussed above. For consistency, we should probably manually change the DoB type on auditor.actor_usr_history and apply the same "dob + '3 hours'" logic used on actor.usr. We should still call update_auditors(), because it does other stuff, but after the manual column change. I'll look at fixing that and force-pushing.

[Bill Erickson (berick)]

Changes to above force-pushed to same branch (user/berick/lp838525-dob-as-date-sql-repairs).

[Jason Stephenson (jstephenson)]

Assigned it to myself while I test it.

[Jason Stephenson (jstephenson)]

Works with my scenario and looks like it will cover Ben's comment #22 above also.

Signed off and pushed to master.

Thanks, Bill!