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

Add support for create / drop database propagation from non-main databases #7439

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

halilozanakgul
Copy link
Contributor

@halilozanakgul halilozanakgul commented Jan 23, 2024

DESCRIPTION: Adds support for distributed CREATE/DROP DATABASE commands from the databases where Citus is not installed

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Merging #7439 (e0d0660) into main (b3ef1b7) will decrease coverage by 0.54%.
The diff coverage is 94.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7439      +/-   ##
==========================================
- Coverage   89.59%   89.06%   -0.54%     
==========================================
  Files         282      282              
  Lines       60354    60369      +15     
  Branches     7517     7521       +4     
==========================================
- Hits        54076    53766     -310     
- Misses       4126     4401     +275     
- Partials     2152     2202      +50     

Copy link
Contributor

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Looks good overall, nice and simple. Left a few comments though.

Copy link
Contributor

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Looks good overall, nice and simple. Left a few comments though.

@onurctirtir
Copy link
Member

DESCRIPTION: Adds support for CREATE/DROP DATABASE queries from Citus non-main databases

Did we agree on calling out other databases as "non-main dbs" in user-facing errors / docs, or should we instead say ".. from databases where Citus is not installed".

What do you think @halilozanakgul, @JelteF?

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.

This looks very good but given the refactor done in #7404, I'd rather prefer merging this after #7404.

src/backend/distributed/commands/utility_hook.c Outdated Show resolved Hide resolved
src/test/regress/expected/other_databases.out Show resolved Hide resolved
src/test/regress/sql/other_databases.sql Show resolved Hide resolved
@onurctirtir onurctirtir changed the title Create DB from Non-main DB Add support for create / drop database propagation from non-main databases Feb 21, 2024
@onurctirtir onurctirtir changed the base branch from main to grant_database_2pc February 21, 2024 10:05
Base automatically changed from grant_database_2pc to main February 21, 2024 10:15
Copy link
Contributor

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

there's something strange going on with the commit list. It's containing a ton of commits from Gurkan.

@onurctirtir
Copy link
Member

there's something strange going on with the commit list. It's containing a ton of commits from Gurkan.

pushed again now

@onurctirtir onurctirtir enabled auto-merge (squash) February 21, 2024 10:40
@onurctirtir onurctirtir merged commit 852bcc5 into main Feb 21, 2024
157 checks passed
@onurctirtir onurctirtir deleted the create_db_from_other_db branch February 21, 2024 10:44
onurctirtir added a commit that referenced this pull request Feb 23, 2024
…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
…bases (#7439)

DESCRIPTION: Adds support for distributed `CREATE/DROP DATABASE `
commands from the databases where Citus is not installed

---------

Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>
emelsimsek pushed a commit that referenced this pull request Mar 11, 2024
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants