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 hypertable constraint creation USING INDEX #7040

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

Conversation

arajkumar
Copy link
Member

@arajkumar arajkumar commented Jun 18, 2024

Before this commit, the following operation was disallowed:

ALTER TABLE foo ADD CONSTRAINT foo_pkey
PRIMARY KEY USING INDEX foo_pkey

pgcopydb, the tool that powers our migration process[1], handles primary key
and unique constraint migrations in two steps:

  1. Build unique indexes.
  2. Create the unique constraint or primary key using the unique index created
    in step 1.

This multi-step approach is useful for reducing the time spent on
EXCLUSIVE LOCKS while building unique constraints or primary keys.

[1] https://docs.timescale.com/migrate/latest/live-migration/

Fixes DAT-36

https://linear.app/timescale/issue/DAT-36

@arajkumar arajkumar self-assigned this Jun 18, 2024
@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch 2 times, most recently from 32859da to 5878a37 Compare June 18, 2024 14:49
@arajkumar arajkumar marked this pull request as ready for review June 18, 2024 14:52
@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch 2 times, most recently from 5e03859 to 9c82c96 Compare June 19, 2024 11:54
@svenklemm
Copy link
Member

Hmm ideally we remove the pg constraint tracking from chunk_constraint then we wouldnt need to do anything special for constraint creation with index.

Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

I dont think the functions that copies constraints to chunks works like this. It is missing handling for constraint columns. None of the tests you added actually utilize the code you added in that function.

@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch 3 times, most recently from 99090d7 to aa519a8 Compare June 25, 2024 11:36
@arajkumar arajkumar requested a review from svenklemm June 25, 2024 13:52
@nikkhils
Copy link
Contributor

nikkhils commented Jul 2, 2024

A lot of tests are failing.

@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch 8 times, most recently from 5d11129 to 346d57d Compare August 21, 2024 13:52
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.78%. Comparing base (59f50f2) to head (ad5ef73).
Report is 300 commits behind head on main.

Files Patch % Lines
src/process_utility.c 86.20% 1 Missing and 3 partials ⚠️
src/chunk_constraint.c 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7040      +/-   ##
==========================================
+ Coverage   80.06%   81.78%   +1.71%     
==========================================
  Files         190      204      +14     
  Lines       37181    38144     +963     
  Branches     9450     9892     +442     
==========================================
+ Hits        29770    31197    +1427     
+ Misses       2997     2964      -33     
+ Partials     4414     3983     -431     

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

@@ -2020,7 +2021,7 @@ process_rename_index(ProcessUtilityArgs *args, Cache *hcache, Oid relid, RenameS
Chunk *chunk = ts_chunk_get_by_relid(tablerelid, false);

if (NULL != chunk)
ts_chunk_index_rename(chunk, relid, stmt->newname);
ts_chunk_index_rename(chunk, indexname, stmt->newname);
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why we don't have coverage for this code path

Copy link
Member Author

Choose a reason for hiding this comment

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

@fabriziomello , IIUC, this is not a problem with this PR? should we address it separately?

@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch from 346d57d to 26d40f0 Compare August 27, 2024 14:49
Before this commit, the following operation was disallowed:

```sql
ALTER TABLE foo ADD CONSTRAINT foo_pkey
PRIMARY KEY USING INDEX foo_pkey
```

pgcopydb, the tool that powers our migration process[1], handles primary key
and unique constraint migrations in two steps:

1. Build unique indexes.
2. Create the unique constraint or primary key using the unique index created
   in step 1.

This multi-step approach is useful for reducing the time spent on
EXCLUSIVE LOCKS while building unique constraints or primary keys.

[1] https://docs.timescale.com/migrate/latest/live-migration/

Signed-off-by: Arunprasad Rajkumar <[email protected]>
@arajkumar arajkumar force-pushed the arajkumar/constraint-using-index branch 2 times, most recently from 1546932 to ad5ef73 Compare August 28, 2024 02:23
@arajkumar
Copy link
Member Author

@fabriziomello Thanks for reviewing this PR. I've addressed your concerns, PTAL.

@mkindahl mkindahl removed their request for review November 20, 2024 07:39
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.

4 participants