-
Notifications
You must be signed in to change notification settings - Fork 252
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
feat: temporary tablespaces #3464
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
429b502
to
6bb2d68
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
6bb2d68
to
4222ca0
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
4222ca0
to
af7ab41
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
af7ab41
to
d132c16
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
I added a basic E2e tests for temporary tablespaces: https://github.com/EnterpriseDB/cloudnative-pg/actions/runs/7116382754 |
d132c16
to
0e29d1a
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
0e29d1a
to
26008d4
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
// Apply the list of temporary tablespaces | ||
if len(info.TemporaryTablespaces) > 0 { | ||
configuration.OverwriteConfig("temp_tablespaces", strings.Join(info.TemporaryTablespaces, ",")) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question here, from yesterday's discussion with Marco.
Should we add the empty string here, so the default temp tablespace is included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Let's involve @mnencia here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, when you create a temporary tablespace, you're doing that because you want to use it as long as possible. This is why I'd leave out the empty string from this set, at least by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO in the initial implementation we should not. The recommended approach is to add one or more tablespaces to dedicate for temporary purposes, freeing PGDATA from that use case. In the future we might want to introduce an option for PGDATA, but I'd leave it out for now.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
This patch allows the user to set a tablespace as temporary, by changing the value of the `temp_tablespaces` PostgreSQL setting. Closes: #11 Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>
389a5e2
to
117cf5b
Compare
This commit introduces the
.spec.tablespaces[*].name.temporary
option todetermine whether a tablespace should be added to the
temp_tablespaces
PostgreSQL parameter, and thus become eligible to store temporary data that
does not have an explicit tablespace assignment.
Closes: #11