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

Allow DROP NOT NULL on column of compressed table #7455

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Nov 18, 2024

Remove the restriction on ALTER TABLE ... ALTER COLUMN SET/DROP NOT NULL on compressed tables.

Dropping or setting the NOT NULL constraint on a column of a compressed table is safe since it does not require re-writing any data. Setting NOT NULL will require a full table scan to verify that there are no NULL in the table.

Fixes #5776

@mkindahl mkindahl self-assigned this Nov 18, 2024
@mkindahl mkindahl force-pushed the compressed-alter-column-drop-null branch from c5338ca to 744ac62 Compare November 18, 2024 19:27
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.11%. Comparing base (59f50f2) to head (601c367).
Report is 601 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7455      +/-   ##
==========================================
+ Coverage   80.06%   82.11%   +2.04%     
==========================================
  Files         190      230      +40     
  Lines       37181    43095    +5914     
  Branches     9450    10830    +1380     
==========================================
+ Hits        29770    35386    +5616     
- Misses       2997     3391     +394     
+ Partials     4414     4318      -96     

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


🚨 Try these New Features:

@mkindahl
Copy link
Contributor Author

It is actually fine to use SET NOT NULL as well. It will generate a full table scan to investigate if there are any NULL present, but otherwise it should not require any dramatic changes.

@mkindahl mkindahl force-pushed the compressed-alter-column-drop-null branch from 75b8809 to 3bbca7f Compare November 19, 2024 08:20
@mkindahl mkindahl marked this pull request as ready for review November 19, 2024 08:21
Copy link
Contributor

@antekresic antekresic left a comment

Choose a reason for hiding this comment

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

Setting NOT NULL on a column which has compressed NULL values doesn't work properly. Maybe we should just limit this change to dropping NOT NULL since setting requires decompression which isn't done in the current change.

Also, lets add a test for this, trying to set NOT NULL on a column which has NULL values and the hypertable is full compressed (not partial).

@mkindahl
Copy link
Contributor Author

It is actually fine to use SET NOT NULL as well. It will generate a full table scan to investigate if there are any NULL present, but otherwise it should not require any dramatic changes.

This turned out to not be the case. If executed on a compressed chunk, it works for hypercore-compressed tables but not for heap-compressed tables. It seems like hypercore-compressed tables check for the presence of NULL using table_beginscan, which works for hypercore-compressed tables but not for heap-compressed tables.

@mkindahl
Copy link
Contributor Author

Setting NOT NULL on a column which has compressed NULL values doesn't work properly. Maybe we should just limit this change to dropping NOT NULL since setting requires decompression which isn't done in the current change.

Yeah, I discovered that it does not work for compressed tables using the heap access method, but this works fine for chunks with the hypercore access method (since it looks like a normal hypertable). Added checks so that we only allow this for compressed chunks that are using hypercore table access method.

Also, lets add a test for this, trying to set NOT NULL on a column which has NULL values and the hypertable is full compressed (not partial).

There is an existing test in compression_errors, but that does not compress the table, so the question is if we should check the flag on the hypertable (and ignore the status of the chunks), or if we should allow this with the compression flag set and chunk fully decompressed.

@feikesteenbergen
Copy link
Member

Could we get the simplest thing merged the DROP NOT NULL? the rest is also useful, but easier to workaround for users

@mkindahl mkindahl force-pushed the compressed-alter-column-drop-null branch from 3bbca7f to 2964f82 Compare November 19, 2024 17:04
@mkindahl
Copy link
Contributor Author

Could we get the simplest thing merged the DROP NOT NULL? the rest is also useful, but easier to workaround for users

Ok, split out the code to deal with SET NOT NULL into #7463 and left the other pull request here. Still awaiting reviews.

Remove the restriction on `ALTER TABLE ... ALTER COLUMN DROP NOT NULL`
on compressed hypertables.

Dropping the `NOT NULL` constraint on a column of a compressed
hypertable is safe since it does not require re-writing any data.
@mkindahl mkindahl force-pushed the compressed-alter-column-drop-null branch from 2964f82 to 601c367 Compare November 19, 2024 17:09
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.

[Enhancement]: Remove columns not null on compressed tables
4 participants