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.

Sign up for GitHub

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

Jump to bottom

Adds create / drop database propagation support #7240

Merged
merged 194 commits into from
Nov 21, 2023
Merged

Adds create / drop database propagation support #7240

merged 194 commits into from
Nov 21, 2023

Conversation

gurkanindibay
Copy link
Contributor

@gurkanindibay gurkanindibay commented Sep 28, 2023 •
edited
Loading

DESCRIPTION: Adds support for propagating CREATE/DROP database

In this PR, create and drop database support is added.

For CREATE DATABASE:

  • "oid" option is not supported
  • specifying "strategy" to be different than "wal_log" is not supported
  • specifying "template" to be different than "template1" is not supported

The last two are because those are not saved in pg_database and when activating a node, we cannot assume what parameters were provided when creating the database.

And "oid" is not supported because whether user specified an arbitrary oid when creating the database is not saved in pg_database and we want to avoid from oid collisions that might arise from attempting to use an auto-assigned oid on workers.

Finally, in case of node activation, GRANTs for the database are also propagated.

@codecov
Copy link

codecov bot commented Oct 4, 2023 •
edited
Loading

Codecov Report

Merging #7240 (72a9c22) into main (cedcc22) will decrease coverage by 0.02%.
The diff coverage is 86.82%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7240      +/-   ##
==========================================
- Coverage   89.57%   89.56%   -0.02%     
==========================================
  Files         277      278       +1     
  Lines       59691    59973     +282     
  Branches     7438     7469      +31     
==========================================
+ Hits        53471    53713     +242     
- Misses       4084     4108      +24     
- Partials     2136     2152      +16

@gurkanindibay gurkanindibay changed the title Adds create /drop propagation Adds create /drop database propagation support Oct 9, 2023
@gurkanindibay gurkanindibay marked this pull request as ready for review October 10, 2023 08:02
@onurctirtir onurctirtir self-requested a review October 10, 2023 13:44
onurctirtir
onurctirtir reviewed Oct 12, 2023
View reviewed changes
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/commands/database.c Show resolved Hide resolved
src/backend/distributed/commands/utility_hook.c Show resolved Hide resolved
src/backend/distributed/commands/distribute_object_ops.c Outdated Show resolved Hide resolved
halilozanakgul
halilozanakgul suggested changes Oct 12, 2023
View reviewed changes
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/commands/database.c Outdated Show resolved Hide resolved
src/backend/distributed/deparser/deparse_database_stmts.c Outdated Show resolved Hide resolved
onurctirtir
onurctirtir reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@onurctirtir onurctirtir left a comment

Choose a reason for hiding this comment

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

LGTM, let's rebase & merge this after syncing offline tomorrow morning.

src/backend/distributed/commands/database.c
Comment on lines +268 to +269
PreprocessCreateDatabaseStmt(Node *node, const char *queryString,
ProcessUtilityContext processUtilityContext)
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the case but lastly we agreed on doing as much as we can in Postprocess?

However, I'm also okay with having the checks in Preprocess for CREATE DATABASE as it feels safer to ensure them before creating the database. This is because, we're not so sure about what does it mean to throw an error after creating a database in the same (implicit) transaction.

So let's keep it as is then.

@onurctirtir onurctirtir mentioned this pull request Nov 21, 2023
Open
@onurctirtir
Copy link
Member

onurctirtir commented Nov 21, 2023 •
edited
Loading

DESCRIPTION: Adds create/drop database propagation support for Citus

In this PR, create and drop database support is added with all options.

While developing this PR, I noticed that if a database is being created with a template database, the template database which is being used to create the actual database is not being saved. Therefore, when adding a new node we can not send this parameter as well as other parameters depend on the template; i.e. strategy. I can not presume all the cases but both these strategy and template fields can be saved in a new table and then could be sent afterwards when a new is added. However, this task needs a careful design IMO. Therefore, I haven't implemented it

Please update the PR description before merging the PR to be more descriptive about what we support:

DESCRIPTION: Adds support for propagating `CREATE`/`DROP` database

In this PR, create and drop database support is added.

For CREATE DATABASE:
* "oid" option is not supported
* specifying "strategy" to be different than "wal_log" is not supported
* specifying "template" to be different than "template1" is not supported

The last two are because those are not saved in `pg_database` and when activating a node, we cannot assume what parameters were provided when creating the database.

And "oid" is not supported because whether user specified an arbitrary oid when creating the database is not saved in pg_database and we want to avoid from oid collisions that might arise from attempting to use an auto-assigned oid on workers.

Finally, in case of node activation, GRANTs for the database are also propagated.

@onurctirtir onurctirtir self-requested a review November 21, 2023 13:24
@onurctirtir onurctirtir removed the request for review from halilozanakgul November 21, 2023 13:26
@gurkanindibay gurkanindibay removed the request for review from marcoslot November 21, 2023 13:26
@onurctirtir onurctirtir self-requested a review November 21, 2023 13:27
@onurctirtir onurctirtir enabled auto-merge (squash) November 21, 2023 13:28
halilozanakgul
halilozanakgul reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@halilozanakgul halilozanakgul left a comment •
edited
Loading

Choose a reason for hiding this comment

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

halilozanakgul
halilozanakgul reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@halilozanakgul halilozanakgul left a comment

Choose a reason for hiding this comment

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

Looks like my change requests are addressed.

halilozanakgul
halilozanakgul previously approved these changes Nov 21, 2023
View reviewed changes
@halilozanakgul halilozanakgul dismissed their stale review November 21, 2023 13:42

Not going to review

@gurkanindibay gurkanindibay merged commit 3b556cb into main Nov 21, 2023
157 checks passed
@gurkanindibay gurkanindibay deleted the create_alter_database branch November 21, 2023 13:43
@onurctirtir onurctirtir mentioned this pull request Nov 29, 2023
Open
57 tasks
@onurctirtir onurctirtir mentioned this pull request Feb 22, 2024
Merged
onurctirtir added a commit that referenced this pull request Feb 23, 2024
@onurctirtir
Test that we check unsupported options for CREATE DATABASE from non-m…
9ddee5d 
…ain dbs (#7532)

When adding CREATE/DROP DATABASE propagation in #7240, luckily
we've added EnsureSupportedCreateDatabaseCommand() check into
deparser too just to be on the safe side. That way, today CREATE
DATABASE commands from non-main dbs don't silently allow unsupported
options.

I wasn't aware of this when merging #7439 and hence wanted to add
a test so that we don't mistakenly remove that check from deparser
in future.
emelsimsek pushed a commit that referenced this pull request Mar 11, 2024
@onurctirtir @emelsimsek
Test that we check unsupported options for CREATE DATABASE from non-m…
089243d 
…ain dbs (#7532)

When adding CREATE/DROP DATABASE propagation in #7240, luckily
we've added EnsureSupportedCreateDatabaseCommand() check into
deparser too just to be on the safe side. That way, today CREATE
DATABASE commands from non-main dbs don't silently allow unsupported
options.

I wasn't aware of this when merging #7439 and hence wanted to add
a test so that we don't mistakenly remove that check from deparser
in future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thanodnl thanodnl thanodnl left review comments

@marcoslot marcoslot marcoslot left review comments

@JelteF JelteF JelteF left review comments

@naisila naisila naisila left review comments

@onurctirtir onurctirtir onurctirtir approved these changes

@halilozanakgul halilozanakgul halilozanakgul left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gurkanindibay @onurctirtir @thanodnl @marcoslot @JelteF @halilozanakgul @naisila