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

[docs] CTAS on PostgreSQL needs commit to apply #8367

Merged
merged 6 commits into from
Oct 17, 2019

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Oct 10, 2019

CATEGORY

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

#8362 CREATE TABLE AS... on PostgreSQL needs a commit to apply (transactional DDL).
This means that the engine parameters need to have AUTOCOMMIT so that SQLLab CTAS can work. This is a simple PR to document this exception

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

@mistercrunch @villebro

@villebro
Copy link
Member

I think adding the option under Database would be a unintrusive way of fixing this, having it right under "Allow DML" along with a clear and concise description.

@dpgaspar
Copy link
Member Author

Update on the issue (Thanks: @rc-ontruck), this can be simply solved by using:

{
"metadata_params": {},
"engine_params": {"isolation_level":"AUTOCOMMIT"},
"metadata_cache_timeout": {},
"schemas_allowed_for_csv_upload": []
}

This checks my second option, that I think it's less intrusive. Going to update docs to refer to this

@dpgaspar dpgaspar marked this pull request as ready for review October 13, 2019 14:10
@dpgaspar dpgaspar changed the title [engine spec] Fix, CTAS needs commit to apply [docs] CTAS on PostgreSQL needs commit to apply Oct 13, 2019
@codecov-io
Copy link

codecov-io commented Oct 13, 2019

Codecov Report

Merging #8367 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8367      +/-   ##
==========================================
+ Coverage   67.57%   67.65%   +0.07%     
==========================================
  Files         448      448              
  Lines       22527    22498      -29     
  Branches     2364     2364              
==========================================
- Hits        15222    15220       -2     
+ Misses       7167     7140      -27     
  Partials      138      138
Impacted Files Coverage Δ
superset/connectors/druid/models.py 82.41% <0%> (ø) ⬆️
superset/views/core.py 71.45% <0%> (+1.29%) ⬆️

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 7e7ea3d...5974027. Read the comment docs.

docs/sqllab.rst Outdated
You can use `CREATE TABLE AS SELECT ...` statements on SQLLab this feature can be toggled on
and off at the database configuration level, note that on PostgreSQL DDL is transactional,
this means that, to properly use this feature you have to set `autocommit` to true on your
engine parameters:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps clarify by explicitly mentioning that CTAS require DML + split up into two sentences to make it easier on the eyes.

Copy link
Member

@villebro villebro Oct 13, 2019

Choose a reason for hiding this comment

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

My bad; I somehow assumed this was being done as DML, not using the button! The text above sounds good. Not sure if this is overkill, but it might be a good idea to also mention this under docs/installation.rst where database-specific quirks are listed, or at least a reference from one to the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

no problem, I would say that a small note is not overkill. Just updated it

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar dpgaspar merged commit c62b2f4 into apache:master Oct 17, 2019
graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019
* [docs] New, document need for PG to use autocommit for CTAS
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants