Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix now() plantime constification with BETWEEN #6491

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Fix now() plantime constification with BETWEEN #6491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .unreleased/PR_6491_now_between
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes: #6491 Enable now() plantime constification with BETWEEN
3 changes: 3 additions & 0 deletions sql/debug_build_utils.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ AS '@MODULE_PATHNAME@', 'ts_debug_point_release';

CREATE OR REPLACE FUNCTION debug_waitpoint_id(TEXT) RETURNS BIGINT LANGUAGE C VOLATILE STRICT
AS '@MODULE_PATHNAME@', 'ts_debug_point_id';

CREATE OR REPLACE FUNCTION ts_now_mock()
RETURNS TIMESTAMPTZ AS '@MODULE_PATHNAME@', 'ts_now_mock' LANGUAGE C STABLE STRICT PARALLEL SAFE;
12 changes: 3 additions & 9 deletions src/planner/constify_now.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ get_hypertable_dimension(Oid relid, int flags)
return hyperspace_get_open_dimension(ht->space, 0);
}

static bool
bool
is_valid_now_func(Node *node)
{
if (IsA(node, FuncExpr) && castNode(FuncExpr, node)->funcid == F_NOW)
Expand Down Expand Up @@ -249,17 +249,11 @@ ts_constify_now(PlannerInfo *root, List *rtable, Node *node)

foreach (lc, be->args)
{
if (IsA(lfirst(lc), OpExpr) && is_valid_now_expr(lfirst_node(OpExpr, lc), rtable))
{
OpExpr *op = lfirst_node(OpExpr, lc);
additions = lappend(additions, constify_now_expr(root, op));
}
additions = lappend(additions, ts_constify_now(root, rtable, (Node *) lfirst(lc)));
}

if (additions)
{
be->args = list_concat(be->args, additions);
}
be->args = additions;
Copy link
Member

@akuzm akuzm Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, this test currently fails...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


break;
}
Expand Down
56 changes: 56 additions & 0 deletions src/planner/planner.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <optimizer/plancat.h>
#include <parser/analyze.h>
#include <tcop/tcopprot.h>
#include <utils/fmgrprotos.h>
#include "compat/compat-msvc-exit.h"

#include <math.h>
Expand Down Expand Up @@ -279,6 +280,47 @@ typedef struct
PlannerInfo *root;
} PreprocessQueryContext;

void
replace_now_mock_walker(PlannerInfo *root, Node *clause, Oid funcid)
{
/* whenever we encounter a FuncExpr with now(), replace it with the supplied funcid */
switch (nodeTag(clause))
{
case T_FuncExpr:
{
if (is_valid_now_func(clause))
{
FuncExpr *fe = castNode(FuncExpr, clause);
fe->funcid = funcid;
return;
}
break;
}
case T_OpExpr:
{
ListCell *lc;
OpExpr *oe = castNode(OpExpr, clause);
foreach (lc, oe->args)
{
replace_now_mock_walker(root, (Node *) lfirst(lc), funcid);
}
break;
}
case T_BoolExpr:
{
ListCell *lc;
BoolExpr *be = castNode(BoolExpr, clause);
foreach (lc, be->args)
{
replace_now_mock_walker(root, (Node *) lfirst(lc), funcid);
}
break;
}
default:
return;
}
}

/*
* Preprocess the query tree, including, e.g., subqueries.
*
Expand Down Expand Up @@ -310,6 +352,20 @@ preprocess_query(Node *node, PreprocessQueryContext *context)
{
from->quals =
ts_constify_now(context->root, context->current_query->rtable, from->quals);
#ifdef TS_DEBUG
/*
* only replace if GUC is also set. This is used for testing purposes only,
* so no need to change the output for other tests in DEBUG builds
*/
if (ts_current_timestamp_mock != NULL && strlen(ts_current_timestamp_mock) != 0)
{
Oid funcid_mock;
const char *funcname = "ts_now_mock()";
funcid_mock = DatumGetObjectId(
DirectFunctionCall1(regprocedurein, CStringGetDatum(funcname)));
replace_now_mock_walker(context->root, from->quals, funcid_mock);
}
#endif
}
/*
* We only amend space constraints for UPDATE/DELETE and SELECT FOR UPDATE
Expand Down
21 changes: 21 additions & 0 deletions src/time_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,3 +567,24 @@
return res;
}
#endif

TS_FUNCTION_INFO_V1(ts_now_mock);

/* return mock time for testing */
Datum
ts_now_mock(PG_FUNCTION_ARGS)
{
Datum res;
#ifdef TS_DEBUG
if (ts_current_timestamp_mock != NULL && strlen(ts_current_timestamp_mock) != 0)
{
res = DirectFunctionCall3(timestamptz_in,
CStringGetDatum(ts_current_timestamp_mock),
0,
Int32GetDatum(-1));
return res;
}
#endif
res = TimestampTzGetDatum(GetCurrentTimestamp());
return res;

Check warning on line 589 in src/time_utils.c

View check run for this annotation

Codecov / codecov/patch

src/time_utils.c#L588-L589 

Added lines #L588 - L589 were not covered by tests
}
2 changes: 2 additions & 0 deletions src/time_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,5 @@ extern TSDLLEXPORT int64 ts_subtract_integer_from_now_saturating(Oid now_func, i
#ifdef TS_DEBUG
extern TSDLLEXPORT Datum ts_get_mock_time_or_current_time(void);
#endif

bool is_valid_now_func(Node *node);
2 changes: 2 additions & 0 deletions src/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,3 +246,5 @@ ts_get_relation_relid(char const *schema_name, char const *relation_name, bool r
return InvalidOid;
}
}

void replace_now_mock_walker(PlannerInfo *root, Node *clause, Oid funcid);
Loading
Loading