Pass join related structs to the cagg rte #5441

RafiaSabih · 2023-03-14T15:40:48Z

In case of joins in the continuous aggregates, pass the required structs to the new rte created. These values are required by the planner to finally query the materialized view.

Fixes #5433, https://github.com/timescale/Support-Dev-Collab/issues/886

github-actions · 2023-03-14T15:41:15Z

@konskov, @svenklemm: please review this pull request.

Powered by pull-review

fabriziomello

Need to add an update step to rebuild caggs definitions in order to fix already stored QueryTree in pg_rewrite catalog table.

codecov · 2023-03-17T12:05:37Z

Codecov Report

Merging #5441 (f11b704) into main (09565ac) will decrease coverage by 0.09%.
The diff coverage is 49.39%.

@@            Coverage Diff             @@
##             main    #5441      +/-   ##
==========================================
- Coverage   90.48%   90.40%   -0.09%     
==========================================
  Files         229      229              
  Lines       53870    53926      +56     
==========================================
+ Hits        48746    48750       +4     
- Misses       5124     5176      +52

Impacted Files	Coverage Δ
tsl/src/continuous_aggs/create.c	`84.68% <49.39%> (-1.35%)`	⬇️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

scripts/test_updates_pg12.sh

scripts/test_updates_pg15.sh

test/sql/updates/cleanup.continuous_aggs.v9.sql

test/sql/updates/cleanup.v9.sql

test/sql/updates/post.v9.sql

test/sql/updates/setup.continuous_aggs.v9.sql

test/sql/updates/setup.v9.sql

test/sql/updates/post.continuous_aggs.v9.sql

sql/updates/post-update.sql

tsl/src/continuous_aggs/create.c

fabriziomello · 2023-04-11T22:18:02Z

sql/updates/post-update.sql

+    IF ts_version >= '2.10.0' THEN
+            CREATE PROCEDURE _timescaledb_internal.post_update_cagg_try_repair(
+                cagg_view REGCLASS, force_rebuild boolean
+            ) AS '@MODULE_PATHNAME@', 'ts_cagg_try_repair' LANGUAGE C;
+    END IF;
+    FOR vname, materialized_only IN select format('%I.%I', cagg.user_view_schema, cagg.user_view_name)::regclass, cagg.materialized_only from _timescaledb_catalog.continuous_agg cagg
+    LOOP
+        IF ts_version >= '2.10.0' THEN
+            SET log_error_verbosity TO VERBOSE;
+            CALL _timescaledb_internal.post_update_cagg_try_repair(vname, true);
+        END IF;
+    END LOOP;
+    IF ts_version >= '2.10.0' THEN
+            DROP PROCEDURE IF EXISTS _timescaledb_internal.post_update_cagg_try_repair;
+    END IF;


Suggested change

IF ts_version >= '2.10.0' THEN

CREATE PROCEDURE _timescaledb_internal.post_update_cagg_try_repair(

cagg_view REGCLASS, force_rebuild boolean

) AS '@MODULE_PATHNAME@', 'ts_cagg_try_repair' LANGUAGE C;

END IF;

FOR vname, materialized_only IN select format('%I.%I', cagg.user_view_schema, cagg.user_view_name)::regclass, cagg.materialized_only from _timescaledb_catalog.continuous_agg cagg

LOOP

IF ts_version >= '2.10.0' THEN

SET log_error_verbosity TO VERBOSE;

CALL _timescaledb_internal.post_update_cagg_try_repair(vname, true);

END IF;

END LOOP;

IF ts_version >= '2.10.0' THEN

DROP PROCEDURE IF EXISTS _timescaledb_internal.post_update_cagg_try_repair;

END IF;

IF ts_version >= '2.10.0' THEN

CREATE PROCEDURE _timescaledb_internal.post_update_cagg_try_repair(

cagg_view REGCLASS, force_rebuild BOOLEAN

) AS '@MODULE_PATHNAME@', 'ts_cagg_try_repair' LANGUAGE C;

FOR vname, materialized_only IN select format('%I.%I', cagg.user_view_schema, cagg.user_view_name)::regclass, cagg.materialized_only from _timescaledb_catalog.continuous_agg cagg

LOOP

IF ts_version >= '2.10.0' THEN

SET log_error_verbosity TO VERBOSE;

CALL _timescaledb_internal.post_update_cagg_try_repair(vname, true);

END IF;

END LOOP;

DROP PROCEDURE IF EXISTS _timescaledb_internal.post_update_cagg_try_repair(REGCLASS, BOOLEAN);

END IF;

fabriziomello · 2023-04-11T22:20:13Z

test/sql/updates/setup.repair.sql

+\if :has_cagg_joins
+     \ir setup.repair.cagg_joins.sql
+\endif
 \ir setup.repair.cagg.sql


Suggested change

\if :has_cagg_joins

\ir setup.repair.cagg_joins.sql

\endif

\ir setup.repair.cagg.sql

\ir setup.repair.cagg.sql

\if :has_cagg_joins

\ir setup.repair.cagg_joins.sql

\endif

We're adding a new repair test so make sense to be added at the end.

fabriziomello · 2023-04-11T22:23:01Z

test/sql/updates/setup.repair.sql

+     );
+
+     -- Break the first time dimension on each table. These are different
+     -- depending on the time type for the table and we need to check all
+     -- versions.
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_int'::regclass AND column_name = 'time'
+     ORDER BY dimension_slice_id LIMIT 1
+     );
+
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_timestamp'::regclass AND column_name = 'time'
+     ORDER BY dimension_slice_id LIMIT 1
+     );
+
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_timestamptz'::regclass AND column_name = 'time'
+     ORDER BY dimension_slice_id LIMIT 1
+     );
+
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_date'::regclass AND column_name = 'time'
+     ORDER BY dimension_slice_id LIMIT 1
+     );
+
+     -- Delete all dimension slices for one table to break it seriously. It
+     -- should still be repaired.
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_extra'::regclass
+     );
+
+     -- Break the partition constraints on some of the tables. The
+     -- partition constraints look the same in all tables so we create a
+     -- mix of tables with no missing partition constraint slices, just one
+     -- missing partition constraint dimension slice, and several missing
+     -- partition constraint dimension slices.
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_timestamp'::regclass AND column_name = 'tag'
+     ORDER BY dimension_slice_id LIMIT 1
+     );
+
+     DELETE FROM _timescaledb_catalog.dimension_slice WHERE id IN (
+     SELECT dimension_slice_id FROM slices
+     WHERE hypertable = 'repair_test_date'::regclass AND column_name = 'tag'
+     ORDER BY dimension_slice_id
+     );
+
+     \echo **** Expected repairs ****
+     WITH unparsed_slices AS (
+     SELECT dimension_id,
+          dimension_slice_id,
+          hypertable,
+          constraint_name,
+          column_type,
+          column_name,
+          (SELECT SUBSTRING(constraint_expr, $$>=\s*'?([\w\d\s:+-]+)'?$$)) AS range_start,
+          (SELECT SUBSTRING(constraint_expr, $$<\s*'?([\w\d\s:+-]+)'?$$)) AS range_end
+          FROM slices
+     )
+     SELECT DISTINCT
+          dimension_slice_id,
+          dimension_id,
+          CASE
+          WHEN column_type = 'timestamptz'::regtype THEN
+               EXTRACT(EPOCH FROM range_start::timestamptz)::bigint * 1000000
+          WHEN column_type = 'timestamp'::regtype THEN
+               EXTRACT(EPOCH FROM range_start::timestamp)::bigint * 1000000
+          WHEN column_type = 'date'::regtype THEN
+               EXTRACT(EPOCH FROM range_start::date)::bigint * 1000000
+          ELSE
+               CASE
+               WHEN range_start IS NULL
+               THEN (-9223372036854775808)::bigint
+               ELSE range_start::bigint
+               END
+          END AS range_start,
+          CASE
+          WHEN column_type = 'timestamptz'::regtype THEN
+               EXTRACT(EPOCH FROM range_end::timestamptz)::bigint * 1000000
+          WHEN column_type = 'timestamp'::regtype THEN
+               EXTRACT(EPOCH FROM range_end::timestamp)::bigint * 1000000
+          WHEN column_type = 'date'::regtype THEN
+               EXTRACT(EPOCH FROM range_end::date)::bigint * 1000000
+          ELSE
+               CASE WHEN range_end IS NULL
+               THEN 9223372036854775807::bigint
+               ELSE range_end::bigint
+               END
+          END AS range_end
+     FROM unparsed_slices
+     WHERE dimension_slice_id NOT IN (SELECT id FROM _timescaledb_catalog.dimension_slice);
+
+     DROP VIEW slices;
+\endif


IMHO don't need to indent this code... it just make the review harder due to a big diff output

scripts/test_updates_pg13.sh

scripts/test_updates_pg14.sh

scripts/test_updates_pg15.sh

fabriziomello

Left some minor reviews but LGTM. Approved!

mkindahl · 2023-04-12T07:10:01Z

test/sql/updates/post.repair.cagg_joins.sql

+-- This file and its contents are licensed under the Apache License 2.0.
+-- Please see the included NOTICE for copyright information and
+-- LICENSE-APACHE for a copy of the license.
+
+SELECT current_setting('server_version_num')::int >=  130000 AS has_cagg_join_using \gset
+    \d+ cagg_joins_upgrade_test_with_realtime
+    SELECT * FROM cagg_joins_upgrade_test_with_realtime ORDER BY bucket;
+    \d+ cagg_joins_upgrade_test
+    SELECT * FROM cagg_joins_upgrade_test ORDER BY bucket;
+    \d+ cagg_joins_where
+    SELECT * FROM cagg_joins_where ORDER BY bucket;
+    \if :has_cagg_join_using
+        \d+ cagg_joins_upgrade_test_with_realtime_using
+        SELECT * FROM cagg_joins_upgrade_test_with_realtime_using ORDER BY bucket;
+        \d+ cagg_joins_upgrade_test_using
+        SELECT * FROM cagg_joins_upgrade_test_using ORDER BY bucket;
+\endif


Nit: indentation is strange. Hard to read and understand.

test/sql/updates/post.repair.sql

mkindahl · 2023-04-12T07:14:26Z

test/sql/updates/setup.repair.cagg_joins.sql

+WHERE ht_cagg_joins.city = nt_cagg_joins.location
+GROUP BY 1,3;
+
+-- Only test joins with cusing clause for postgresql versions above 12


Suggested change

-- Only test joins with cusing clause for postgresql versions above 12

-- Only test joins with using clause for postgresql versions above 12

mkindahl · 2023-04-12T07:19:02Z

test/sql/updates/setup.repair.sql

-CREATE TABLE repair_test_int(time integer not null, temp float8, tag integer, color integer);
-CREATE TABLE repair_test_timestamptz(time timestamptz not null, temp float8, tag integer, color integer);
-CREATE TABLE repair_test_extra(time timestamptz not null, temp float8, tag integer, color integer);
-CREATE TABLE repair_test_timestamp(time timestamp not null, temp float8, tag integer, color integer);
-CREATE TABLE repair_test_date(time date not null, temp float8, tag integer, color integer);
-
-- We only break the dimension slice table if there is repair that is
-- going to be done, but we create the tables regardless so that we
-- can compare the databases.
-SELECT create_hypertable('repair_test_int', 'time', 'tag', 2, chunk_time_interval => '3'::bigint);
-SELECT create_hypertable('repair_test_timestamptz', 'time', 'tag', 2, chunk_time_interval => '1 day'::interval);
-SELECT create_hypertable('repair_test_extra', 'time', 'tag', 2, chunk_time_interval => '1 day'::interval);
-SELECT create_hypertable('repair_test_timestamp', 'time', 'tag', 2, chunk_time_interval => '1 day'::interval);
-SELECT create_hypertable('repair_test_date', 'time', 'tag', 2, chunk_time_interval => '1 day'::interval);


No need to change the indentation of the code, it just makes the review harder. The \if should be used sparingly, so readability should not be an issue in these cases. Now I have no chance of seeing if you have made any other changes to the code apart from changing the indentation.

mkindahl · 2023-04-12T07:22:00Z

test/sql/updates/setup.repair.sql

+FROM pg_extension
+WHERE extname = 'timescaledb' \gset
+
+\if :test_repair_dimension


Why are you excluding the repair tables when you are not testing the dimension repair? The comment says that the tables are created regardless, but that they are only "broken" if repair should be tested.

tsl/src/continuous_aggs/create.c

mkindahl · 2023-04-12T08:35:27Z

tsl/src/continuous_aggs/create.c

+	if (!finalized)
+		mattablecolumninfo_addinternal(&mattblinfo);


Why only when the table is not finalized? Please add a comment.

This is also part of existing code not something done in this PR.

But the previous line did not have that check?

You mean for mattablecomuninfo_init?
The same sequence of steps are done in cagg_create(), the explicit comments are also there.
Basically we need internal column info only when not finalized, mattbl will there in any case.

@RafiaSabih we've changed the behavior of the cagg_rebuild_view_definition because before it rebuild just then the view is in the old format (aka finalized=false). Now we enable rebuild the view definition for both finalized and not finalized caggs and also added a parameter to force the rebuild... so is good to add comments about it.

mkindahl · 2023-04-12T08:37:55Z

tsl/src/continuous_aggs/create.c

+		view_query = finalizequery_get_select_query(&fqi,
+													mattblinfo.matcollist,
+													&mataddress,
+													mat_ht->fd.table_name.data);


Nit

Suggested change

mat_ht->fd.table_name.data);

NameStr(mat_ht->fd.table_name));

tsl/src/continuous_aggs/create.c

mkindahl · 2023-04-12T08:57:02Z

tsl/test/sql/cagg_joins.sql.in

+   MAX(temperature),
+   MIN(temperature),
+   name
+FROM conditions JOIN devices ON conditions.device_id = devices.device_id


Did we support joins with several conditions? If so, should you add a test for it?

I don't understand this comment, this is a test for caggs with joins with more conditions. What we support is additional condition in where clause. Did you mean to add this also in repair test?

Yes, I was thinking about joins with multiple ON conditions, for example, FROM conditions JOIN devices ON conditions.device_id = devices.device_id AND conditions.building_id = devices.building_id. Since this is a quite drastic change and the checks are not easy to get right, it would be good to ensure that we haven't missed the obvious cases.

@mkindahl currently we support only 1 (one) join condition... for explicit INNER JOIN extra conditions should be placed into the WHERE clause, but not for JOIN...

@RafiaSabih would be good to add a failing test like FROM conditions JOIN devices ON conditions.device_id = devices.device_id WHERE conditions.building_id = devices.building_id

Multiple ON conditions is not supported.

Ok, then it would be good to have a test, as @fabriziomello mention.

yes i have added that test in regression suite.

~~Where? I do not see a cagg with several join expressions in this pull request.~~

Never mind, found it.

fabriziomello

Remove unnecessary formatting changes in test/sql/updates/setup.repair.sql

fabriziomello · 2023-04-12T17:11:55Z

test/sql/updates/setup.repair.sql

@@ -55,10 +63,10 @@ INSERT INTO repair_test_date VALUES
 -- repaired properly, we will notice them when restoring the
 -- constraint.
 ALTER TABLE _timescaledb_catalog.chunk_constraint
-      DROP CONSTRAINT chunk_constraint_dimension_slice_id_fkey;
+    DROP CONSTRAINT chunk_constraint_dimension_slice_id_fkey;


Revert unnecessary formating!

fabriziomello · 2023-04-12T17:12:01Z

test/sql/updates/setup.repair.sql


 CREATE VIEW slices AS (
-      SELECT ch.hypertable_id,
+       SELECT ch.hypertable_id,


Revert unnecessary formating!

fabriziomello · 2023-04-12T17:12:19Z

test/sql/updates/setup.repair.sql

@@ -143,6 +152,7 @@ WITH unparsed_slices AS (
 	   (SELECT SUBSTRING(constraint_expr, $$<\s*'?([\w\d\s:+-]+)'?$$)) AS range_end
      FROM slices
 )
+


Revert unnecessary formating!

fabriziomello · 2023-04-12T17:40:07Z

tsl/test/sql/cagg_joins.sql.in

+WHERE conditions.city = devices.location AND
+      conditions.temperature > 28


This should error out, no??? because you're adding another JOIN condition in the WHERE clause or am I missing something?

In case of joins in the continuous aggregates, pass the required structs to the new rte created. These values are required by the planner to finally query the materialized view. Fixes timescale#5433

timescale-automation · 2023-04-13T02:59:07Z

Automated backport to 2.10.x not done: cherry-pick failed.

Git status

HEAD detached at origin/2.10.x
You are currently cherry-picking commit 3f9cb3c2.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   CHANGELOG.md
	modified:   sql/updates/post-update.sql
	new file:   test/sql/updates/post.repair.cagg_joins.sql
	modified:   test/sql/updates/post.repair.sql
	new file:   test/sql/updates/setup.repair.cagg_joins.sql
	modified:   test/sql/updates/setup.repair.sql
	modified:   tsl/test/sql/cagg_joins.sql.in

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   scripts/test_updates_pg13.sh
	both modified:   scripts/test_updates_pg14.sh
	both modified:   scripts/test_updates_pg15.sh
	both modified:   tsl/src/continuous_aggs/create.c
	both modified:   tsl/test/expected/cagg_joins-12.out
	both modified:   tsl/test/expected/cagg_joins-13.out
	both modified:   tsl/test/expected/cagg_joins-14.out
	both modified:   tsl/test/expected/cagg_joins-15.out

Job log

RafiaSabih self-assigned this Mar 14, 2023

github-actions bot requested review from konskov and svenklemm March 14, 2023 15:41

RafiaSabih force-pushed the cagg_joins_fix branch from 07cd13d to 6715845 Compare March 14, 2023 15:42

fabriziomello requested changes Mar 16, 2023

View reviewed changes

RafiaSabih force-pushed the cagg_joins_fix branch from 6715845 to 1138c09 Compare March 17, 2023 11:51

RafiaSabih force-pushed the cagg_joins_fix branch 9 times, most recently from 002b73c to aa23684 Compare March 22, 2023 11:08