Re: [PATCH] Don't block HOT update by BRIN index

Hi,

I've polished the patch a bit, with the goal to get it committed. I've 
added the new amhotblocking flag to indexam.sgml (and I'm wondering if 
there's another place in docs for more details).

But then I realized there's an issue in handling the predicate. Consider 
this example:

   drop table if exists t;
   create table t (a int, b int);
   insert into t values (1, 100);
   create index on t using brin (b) where a = 2;

   update t set a = 2;

   explain analyze select * from t where a = 2 and b = 100;
   set enable_seqscan = off;
   explain analyze select * from t where a = 2 and b = 100;

With the previous version of the patch, the explains are this:

                                 QUERY PLAN
   ----------------------------------------------------------------------
    Seq Scan on t  (cost=0.00..1.01 rows=1 width=8)
                   (actual time=0.006..0.007 rows=1 loops=1)
      Filter: ((a = 2) AND (b = 100))
    Planning Time: 0.040 ms
    Execution Time: 0.018 ms
   (4 rows)

                                 QUERY PLAN
   ----------------------------------------------------------------------
    Bitmap Heap Scan on t  (cost=12.03..16.05 rows=1 width=8)
                                (actual time=0.007..0.009 rows=0 loops=1)
      Recheck Cond: ((b = 100) AND (a = 2))
      ->  Bitmap Index Scan on t_b_idx  (cost=0.00..12.03 rows=1 width=0)
                                (actual time=0.006..0.006 rows=0 loops=1)
            Index Cond: (b = 100)
    Planning Time: 0.041 ms
    Execution Time: 0.026 ms
   (6 rows)

Notice that the second plan (using the brin index) produces 0 rows, 
which is obviously wrong. Clearly, the index was not updated.

I think this is caused by simple thinko in RelationGetIndexAttrBitmap, 
which did this:

     /* Collect all attributes in the index predicate, too */
     if (indexDesc->rd_indam->amhotblocking)
         pull_varattnos(indexPredicate, 1, &hotblockingattrs);

I think this is wrong - we should not ignore the predicate based on 
amhotblocking, because then we'll fail to notice an update making the 
tuple match the index predicate (as in the example).

The way I understand heap_update() it does not try to determine if the 
update makes the tuple indexable, it just disables HOT when it might 
happen. The attached patch just calls pull_varattnos every time.

I wonder if we might be a bit smarter about the predicates vs. HOT, and 
disable HOT only when the tuple becomes indexable after the update.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company