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.

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

Error on create continuous aggregate with data #2389

Merged
merged 1 commit into from Sep 14, 2020

Conversation

mkindahl
Copy link
Contributor

If a continuous aggregate is created using CREATE MATERIALIZED VIEW
using the WITH DATA option, it will fail with a segmentation fault if
executed inside a transaction or block. This is because starting a new
transaction in the middle of the statement will reset the SPI stack.

This commit fixes this by raising an error if CREATE MATERIALIZED VIEW is not executed on top-level and WITH DATA is used either
directly or indirectly.

Closes #2371

@mkindahl mkindahl self-assigned this Sep 14, 2020
@mkindahl mkindahl marked this pull request as ready for review September 14, 2020 14:29
@mkindahl mkindahl requested a review from a team as a code owner September 14, 2020 14:29
@mkindahl mkindahl requested review from pmwkaa and removed request for a team September 14, 2020 14:29
@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #2389 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2389      +/-   ##
==========================================
+ Coverage   88.51%   88.53%   +0.01%     
==========================================
  Files         212      212              
  Lines       34924    34928       +4     
==========================================
+ Hits        30912    30922      +10     
+ Misses       4012     4006       -6     
Impacted Files Coverage Δ
src/process_utility.c 91.77% <100.00%> (+0.67%) ⬆️
tsl/src/fdw/shippable.c 82.85% <0.00%> (-11.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f74262...dc625a2. Read the comment docs.

FROM conditions
GROUP BY 1,2 WITH DATA;
END $$;
ERROR: CREATE MATERIALIZED VIEW ... WITH DATA cannot be executed from a function
Copy link
Contributor

@pmwkaa pmwkaa Sep 14, 2020

Choose a reason for hiding this comment

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

This is slightly odd message, why we had to wrap it into the DO statement?

Copy link
Contributor Author

@mkindahl mkindahl Sep 14, 2020

Choose a reason for hiding this comment

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

Agree, but it's the standard message that PreventInTransactionBlock prints. It digs out the context itself, and uses the format %s cannot be .....

\set ON_ERROR_STOP 0
DO
LANGUAGE PLPGSQL
$$ DECLARE ts_version TEXT;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this DECLARE do? Seem like a copy-paste leftover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

-- This should fail since we do not allow refreshing inside a
-- transaction, not even as part of CREATE MATERIALIZED VIEW.
\set ON_ERROR_STOP 0
DO
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just do a regular BEGIN--- COMMIT; transaction block instead of creating a function block. At least it would be good to test that case as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it will likely change the error message as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done that as well.

-- aggregate.
DO
LANGUAGE PLPGSQL
$$ DECLARE ts_version TEXT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Another declare block and variable that is never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

If a continuous aggregate is created using `CREATE MATERIALIZED VIEW`
using the `WITH DATA` option, it will fail with a segmentation fault if
executed inside a transaction or block. This is because starting a new
transaction in the middle of the statement will reset the SPI stack.

This commit fixes this by raising an error if `CREATE MATERIALIZED
VIEW` is not executed on top-level and `WITH DATA` is used either
directly or indirectly.

Closes timescale#2371
@mkindahl mkindahl merged commit 61b5d91 into timescale:master Sep 14, 2020
@mkindahl mkindahl deleted the cagg_crash branch September 14, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create cagg using new syntax crashes in plpgsql block
4 participants