Fix now() plantime constification with BETWEEN

[konskov]

Previously when using BETWEEN ... AND additional constraints
in a WHERE clause, the BETWEEN was not handled correctly because
it was wrapped in a BoolExpr node, which prevented plantime
exclusion.
This commit fixes the handling of this case to allow chunk exclusion
to take place at planning time.
Also, makes sure we use our mock timestamp in all places in tests.
Previously we were using a mix of current_timestamp_mock and now(),
which was returning unexpected/incorrect results.

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (92e9009) 79.74% compared to head (755ae7a) 79.74%.

Files	Patch %	Lines
src/planner/planner.c	87.50%	0 Missing and 3 partials ⚠️
src/time_utils.c	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6491      +/-   ##
==========================================
- Coverage   79.74%   79.74%   -0.01%     
==========================================
  Files         188      188              
  Lines       36739    36722      -17     
  Branches     9290     9278      -12     
==========================================
- Hits        29297    29283      -14     
- Misses       3110     3112       +2     
+ Partials     4332     4327       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

@gayyappan, @erimatnor: please review this pull request.

Powered by pull-review

[akuzm]

It sounds like if the chunks shouldn't be excluded at all, might be more clear to say that they should be excluded at planning time.

[akuzm]

So what are you getting here, something like

(BoolExprAnd
    (OpExpr (= rawtag_id 1))
    (BoolExprAnd
        (>= time now()) (<= time ...)))

Top-level AND should normally be flattened into a list, but ts_constify_now is called before it happens, so this is why we have this problem?

[konskov]

I haven’t yet found where the flattening happens, but yes that is how the expression looks: (pprint(node) in ts_constify_now)

{BOOLEXPR 
   :boolop and 
   :args (
      {BOOLEXPR 
      :boolop and 
      :args (
         {OPEXPR 
         :opno 1325 
         :opfuncid 1156 
         :opresulttype 16 
         :opretset false 
         :opcollid 0 
         :inputcollid 0 
         :args (
            {VAR 
            :varno 1 
            :varattno 1 
            :vartype 1184 
            :vartypmod -1 
            :varcollid 0 
            :varlevelsup 0 
            :varnosyn 1 
            :varattnosyn 1 
            :location 26
            }
            {OPEXPR 
            :opno 1329 
            :opfuncid 1190 
            :opresulttype 1184 
            :opretset false 
            :opcollid 0 
            :inputcollid 0 
            :args (
               {FUNCEXPR 
               :funcid 1299 
               :funcresulttype 1184 
               :funcretset false 
               :funcvariadic false 
               :funcformat 0 
               :funccollid 0 
               :inputcollid 0 
               :args <> 
               :location 39
               }
               {CONST 
               :consttype 1186 
               :consttypmod -1 
               :constcollid 0 
               :constlen 16 
               :constbyval false 
               :constisnull false 
               :location 56 
               :constvalue 16 [ 0 14 39 7 0 0 0 0 0 0 0 0 0 0 0 0 ]
               }
            )
            :location 45
            }
         )
         :location 31
         }
         {OPEXPR 
         :opno 1323 
         :opfuncid 1155 
         :opresulttype 16 
         :opretset false 
         :opcollid 0 
         :inputcollid 0 
         :args (
            {VAR 
            :varno 1 
            :varattno 1 
            :vartype 1184 
            :vartypmod -1 
            :varcollid 0 
            :varlevelsup 0 
            :varnosyn 1 
            :varattnosyn 1 
            :location 26
            }
            {FUNCEXPR 
            :funcid 1299 
            :funcresulttype 1184 
            :funcretset false 
            :funcvariadic false 
            :funcformat 0 
            :funccollid 0 
            :inputcollid 0 
            :args <> 
            :location 68
            }
         )
         :location 31
         }
      )
      :location 31
      }
      {OPEXPR 
      :opno 96 
      :opfuncid 65 
      :opresulttype 16 
      :opretset false 
      :opcollid 0 
      :inputcollid 0 
      :args (
         {VAR 
         :varno 1 
         :varattno 2 
         :vartype 23 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnosyn 1 
         :varattnosyn 2 
         :location 78
         }
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 82 
         :constvalue 4 [ 1 0 0 0 0 0 0 0 ]
         }
      )
      :location 80
      }
   )
   :location 74
   }

[konskov]

I believe the flattening happens in eval_const_expressions, which gets called after ts_constify_now.

[akuzm]

Is this correct? We should keep the old conditions as well. Can you please try to construct a test: first prepare the statement with the mock now() in the middle of the table, so that not all chunks are excluded. Then move the mock now() to be in the future, and check that all the chunks are excluded at run time when you execute the prepared statement. They won't be excluded if you don't keep the original conditions.

[konskov]

you are right, this test currently fails...

[konskov]

However, it seems that this is also the current behavior wrt prepared statements in the main branch, even for the simplest condition (where time > now()) : looking at the constify_now test output, where we test prepared statements https://github.com/timescale/timescaledb/blob/main/tsl/test/shared/expected/constify_now-15.out#L396-L433 I see that two rows are returned when the current timestamp is moved into the future and only one row should actually be returned

[konskov]

the issue appears to be our mock timestamp. When testing with an example actually using now(), the results returned are the expected ones.
Here is the example I used:

create table test2 (time timestamptz, a int, b int);
select create_hypertable('test2', 'time', chunk_time_interval => interval '2 min');

insert into test2 (time, a, b)
select t, a, b from
generate_series(now() - interval '15 min', now() + interval '14 min', interval '30 sec') g(t)
cross join generate_series(1,3,1) a cross join generate_series(1,2,1) b;

prepare p2 as select * from test2 where time between now() - interval '2 min' and now() and a = 1;
-- execute p2 immediately, then after two minutes:
explain execute p2;
                                             QUERY PLAN
----------------------------------------------------------------------------------------------------
 Custom Scan (ChunkAppend) on test2  (cost=4.27..119.60 rows=8 width=16)
   Chunks excluded during startup: 6
   ->  Bitmap Heap Scan on _hyper_1_8_chunk  (cost=4.27..14.95 rows=1 width=16)
         Recheck Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
         Filter: (a = 1)
         ->  Bitmap Index Scan on _hyper_1_8_chunk_test2_time_idx  (cost=0.00..4.27 rows=9 width=0)
               Index Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
   ->  Bitmap Heap Scan on _hyper_1_9_chunk  (cost=4.27..14.95 rows=1 width=16)
         Recheck Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
         Filter: (a = 1)
         ->  Bitmap Index Scan on _hyper_1_9_chunk_test2_time_idx  (cost=0.00..4.27 rows=9 width=0)
               Index Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
(12 rows)

execute p2;
             time              | a | b
-------------------------------+---+---
 2024-01-05 13:03:52.465237+02 | 1 | 1
 2024-01-05 13:03:52.465237+02 | 1 | 2
 2024-01-05 13:04:22.465237+02 | 1 | 1
 2024-01-05 13:04:22.465237+02 | 1 | 2
 2024-01-05 13:04:52.465237+02 | 1 | 1
 2024-01-05 13:04:52.465237+02 | 1 | 2
 2024-01-05 13:05:22.465237+02 | 1 | 1
 2024-01-05 13:05:22.465237+02 | 1 | 2
(8 rows)

-- after a bit:
explain execute p2;
                                             QUERY PLAN
-----------------------------------------------------------------------------------------------------
 Custom Scan (ChunkAppend) on test2  (cost=4.27..119.60 rows=8 width=16)
   Chunks excluded during startup: 6
   ->  Bitmap Heap Scan on _hyper_1_9_chunk  (cost=4.27..14.95 rows=1 width=16)
         Recheck Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
         Filter: (a = 1)
         ->  Bitmap Index Scan on _hyper_1_9_chunk_test2_time_idx  (cost=0.00..4.27 rows=9 width=0)
               Index Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
   ->  Bitmap Heap Scan on _hyper_1_10_chunk  (cost=4.27..14.95 rows=1 width=16)
         Recheck Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
         Filter: (a = 1)
         ->  Bitmap Index Scan on _hyper_1_10_chunk_test2_time_idx  (cost=0.00..4.27 rows=9 width=0)
               Index Cond: (("time" >= (now() - '00:02:00'::interval)) AND ("time" <= now()))
(12 rows)

Time: 4.078 ms
postgres@postgres[63583]=# execute p2;
             time              | a | b
-------------------------------+---+---
 2024-01-05 13:04:22.465237+02 | 1 | 1
 2024-01-05 13:04:22.465237+02 | 1 | 2
 2024-01-05 13:04:52.465237+02 | 1 | 1
 2024-01-05 13:04:52.465237+02 | 1 | 2
 2024-01-05 13:05:22.465237+02 | 1 | 1
 2024-01-05 13:05:22.465237+02 | 1 | 2
 2024-01-05 13:05:52.465237+02 | 1 | 1
 2024-01-05 13:05:52.465237+02 | 1 | 2
(8 rows)

[konskov]

testing with actual now() instead of our mock timestamp, it seems the changes in this PR work as expected, but our current_timestamp_mock doesn’t play well with prepared statements. This needs to be fixed. @akuzm do you think I should do this as part of this PR?

[akuzm]

@akuzm do you think I should do this as part of this PR?

I think it would be good to fix it here because it is needed to properly test your changes.

[akuzm]

This should probably be "ts_now_mock()"? The current "ts_replace_now_mock()" sounds like the function that does the replacement, I was staring at it trying to understand why you need it in SQL 😃

[akuzm]

Also probably we don't need it in the production schema, but not sure where to put it.

[mkindahl]

We have a file debug_build_utils.sql that is used for utilities that should only be available in debug builds.

Not sure you need to have this function in the upgrade/downgrade scripts since it is only going to be used for regression test runs in debug builds.

[mkindahl]

Very nice addition, but suggest that you only add the mock function to debug builds and configure the tests accordingly.

[mkindahl]

We have a file debug_build_utils.sql that is used for utilities that should only be available in debug builds.

Not sure you need to have this function in the upgrade/downgrade scripts since it is only going to be used for regression test runs in debug builds.

[mkindahl]

Not sure you need this in the upgrade/downgrade scripts since it will only be used in regression tests for debug builds.

[konskov]

We don't need this function in latest/reverse-dev, removed.

[mkindahl]

Suggest to move this to debug_build_utils.sql.

[konskov]

done