money.transaction_billing_summary view displays incorrect billing_type and billing_note for the actual last transaction

Bug #1206936 reported by Chris Sharp on 2013-07-31
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
High
Unassigned
2.8
Undecided
Unassigned
2.9
Undecided
Unassigned

Bug Description

Evergreen 2.3.6
OpenSRF 2.1.2
PostgreSQL 9.1.9
Ubuntu 12.04

The money.transcation_billing_summary view returnss the correct billing date, but incorrect billing type and billing note for the chronologically last transaction. Illustrated here:

View definition:

CREATE OR REPLACE VIEW money.transaction_billing_summary AS
 SELECT billing.xact, last(billing.billing_type) AS last_billing_type, last(billing.note) AS last_billing_note, max(billing.billing_ts) AS last_billing_ts, sum(COALESCE(billing.amount, 0::numeric)) AS total_owed
   FROM money.billing
  WHERE billing.voided IS FALSE
  GROUP BY billing.xact
  ORDER BY max(billing.billing_ts);

Running that query with a specific transaction:

 SELECT billing.xact, last(billing.billing_type) AS last_billing_type, last(billing.note) AS last_billing_note, max(billing.billing_ts) AS last_billing_ts, sum(COALESCE(billing.amount, 0::numeric)) AS total_owed
   FROM money.billing
  WHERE billing.voided IS FALSE
  AND billing.xact = 51632427
  GROUP BY billing.xact
  ORDER BY max(billing.billing_ts);

   xact | last_billing_type | last_billing_note | last_billing_ts | total_owed
----------+-------------------+-------------------------------+-------------------------------+------------
 51632427 | Overdue materials | System Generated Overdue Fine | 2010-08-31 16:10:55.392861-04 | 64.95
(1 row)

but, selecting all billings from that transaction shows this:

    id | xact | billing_ts | voided | voider | void_time | amount | billing_type | note | btype
-----------+----------+-------------------------------+--------+--------+-----------+--------+-------------------+-------------------------------+-------
 112214932 | 51632427 | 2010-08-31 16:10:55.392861-04 | f | | | 59.95 | Lost Materials | SYSTEM GENERATED | 3
  91178266 | 51632427 | 2009-11-15 23:00:00-05 | f | | | 0.10 | Overdue materials | System Generated Overdue Fine | 1
  91115555 | 51632427 | 2009-11-13 23:00:00-05 | f | | | 0.10 | Overdue materials | System Generated Overdue Fine | 1
  90955863 | 51632427 | 2009-11-12 23:00:00-05 | f | | | 0.10 | Overdue materials | System Generated Overdue Fine | 1
  90955862 | 51632427 | 2009-11-11 23:00:00-05 | f | | | 0.10 | Overdue materials | System Generated Overdue Fine | 1
  90777700 | 51632427 | 2009-11-10 23:00:00-05 | f | | | 0.10 | Overdue materials | System Generated Overdue Fine | 1

The billing_ts matches the view, but the note and billing_type do not.

Chris Sharp (chrissharp123) wrote :

Thank you to Mike Rylander who proposed this solution to the problem with that particular view:

CREATE OR REPLACE VIEW money.transaction_billing_summary AS
    SELECT DISTINCT xact,
        LAST_VALUE(billing_type) OVER w AS last_billing_type,
        LAST_VALUE(note) OVER w AS last_billing_note,
        MAX(billing_ts) OVER w AS last_billing_ts,
        SUM(COALESCE(amount,0)) OVER w AS total_owed
      FROM money.billing
      WHERE voided IS FALSE
      WINDOW w AS (PARTITION BY xact ORDER BY billing_ts ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING);

As he mentioned, there are many others to convert to using window functions. I have identified a list:

-- money.billable_xact_summary_new
-- money.billable_xact_with_void_summary
-- money.open_transaction_billing_summary
-- money.open_transaction_billing_type_summary
-- money.open_transaction_payment_summary
-- money.transaction_billing_type_summary
-- money.transaction_billing_with_void_summary
-- money.transaction_payment_summary
-- money.transaction_payment_with_void_summary

I'm not sure how involved changing all of those would be, but it sounds like project.

Chris Sharp (chrissharp123) wrote :

working branch on this: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/update_money_views_with_window_functions

I have tested the upgrade script on a PINES test DB, but not installation.

Ben Shum (bshum) wrote :

Pulling this out of limbo for review during the next 2.6 rc1.

Changed in evergreen:
milestone: none → 2.6.0-rc1
status: New → Triaged
importance: Undecided → Medium
tags: added: billing pullrequest
Changed in evergreen:
milestone: 2.6.0-rc1 → 2.next
Mike Rylander (mrylander) wrote :

Chris, it looks like the first hunk of the patch at commit 765aa89f6 is missing a "LAST() OVER w" wrapper on last_billing_type. Is that correct?

no longer affects: evergreen/2.4
Kathy Lussier (klussier) wrote :

So is there additional work that Chris needs to do before this branch is ready for testing?

Kathy Lussier (klussier) on 2014-11-10
tags: removed: pullrequest
no longer affects: evergreen/2.5
Dan Scott (denials) wrote :
Download full text (4.0 KiB)

Changing to High priority because it relates pretty directly to patrons and it has to do with money. Both very important and potentially painful things.

That said, trying the correct view resulted in terrible performance on our 9.1 database:

Here's the incorrect, original view:

EXPLAIN ANALYZE SELECT * FROM money.transaction_billing_summary WHERE xact = 1055743;
                                                              QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------
 Sort (cost=27.31..27.32 rows=1 width=72) (actual time=0.427..0.427 rows=1 loops=1)
   Sort Key: (max(billing.billing_ts))
   Sort Method: quicksort Memory: 25kB
   -> GroupAggregate (cost=0.00..27.30 rows=1 width=72) (actual time=0.412..0.412 rows=1 loops=1)
         -> Index Scan using m_b_xact_idx on billing (cost=0.00..17.65 rows=19 width=72) (actual time=0.059..0.145 rows=32 loops=1)
               Index Cond: (xact = 1055743)
               Filter: (voided IS FALSE)
 Total runtime: 0.514 ms
(8 rows)

Here's the correct, windowed view:

EXPLAIN ANALYZE SELECT * FROM money.transaction_billing_summary WHERE xact = 1055743;
                                                                   QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------
 Subquery Scan on transaction_billing_summary (cost=389206.01..397046.79 rows=1742 width=112) (actual time=16954.467..17031.868 rows=1 loops=1)
   Filter: (transaction_billing_summary.xact = 1055743)
   -> HashAggregate (cost=389206.01..392690.80 rows=348479 width=72) (actual time=16930.022..17007.854 rows=135940 loops=1)
         -> WindowAgg (cost=302984.69..362261.85 rows=2155533 width=72) (actual time=8766.521..14677.489 rows=2161543 loops=1)
               -> Sort (cost=302984.69..308373.53 rows=2155533 width=72) (actual time=8766.454..10138.737 rows=2161543 loops=1)
                     Sort Key: billing.xact, billing.billing_ts
                     Sort Method: external merge Disk: 170368kB
                     -> Seq Scan on billing (cost=0.00..76226.79 rows=2155533 width=72) (actual time=0.014..1353.749 rows=2161543 loops=1)
                           Filter: (voided IS FALSE)
 Total runtime: 17091.181 ms
(10 rows)

After creating an index such as CREATE INDEX CONCURRENTLY m_b_xact_time_idx ON money.billing (xact, billing_ts);, that drops to the following:

EXPLAIN ANALYZE SELECT * FROM money.transaction_billing_summary WHERE xact = 1055743;
                                                                            QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Subquery Scan on transaction_billing_summary (cost=234806.53..242647.30 rows=1742 width=112) (actual time=8516.624..8587.78...

Read more...

Changed in evergreen:
importance: Medium → High
Dan Scott (denials) wrote :

Alternate approach suggested by Mike Rylander: build on the materialized summary table, which will have way fewer rows, and also not require gnarly SQL. So for example:

CREATE OR REPLACE VIEW money.transaction_billing_summary AS SELECT id AS xact, last_billing_type, last_billing_note, last_billing_ts, total_owed FROM money.materialized_billable_xact_summary;

Performance:

EXPLAIN ANALYZE SELECT * FROM money.transaction_billing_summary WHERE xact = 1055743;
                                                                                 QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Index Scan using materialized_billable_xact_summary_pkey on materialized_billable_xact_summary (cost=0.00..6.45 rows=1 width=72) (actual time=0.070..0.070 rows=1 loops=1)
   Index Cond: (id = 1055743)
 Total runtime: 0.094 ms
(3 rows)

Seems good!

Ben Shum (bshum) wrote :

Removing milestone targets while this one is put into a working branch for proper review and inclusion. Looks good though dbs! (faster is better!)

Chris Sharp (chrissharp123) wrote :

I've created a signed-off working branch based on Mike's and Dan's approach, which I've confirmed works on PINES test data. While this doesn't address the consistency issue I brought up in comment #1 above, it does solve the problem reported in the original bug report.

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

no longer affects: evergreen/2.6
no longer affects: evergreen/2.7
tags: added: pullrequest
Dan Wells (dbw2) on 2016-02-24
Changed in evergreen:
assignee: nobody → Dan Wells (dbw2)
Dan Wells (dbw2) wrote :

Added a (somewhat convoluted) test and pushed to master through 2.8. Thanks, Chris et al.!

Changed in evergreen:
milestone: 2.next → 2.10-beta
assignee: Dan Wells (dbw2) → nobody
status: Triaged → Fix Committed
Bill Erickson (berick) wrote :

Heads up.. Pushed working/user/berick/lp1206936-base-schema-repair to fix a problem with the ordering in the base SQL schema.

money.materialized_billable_xact_summary was referenced before it was defined.

Kathy Lussier (klussier) wrote :

And I've pushed Bill's fix to master. Thanks Bill and Dan and Chris!

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