Adds alter database support on non-main database and on any node #7563
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7563 +/- ##
==========================================
- Coverage 89.69% 89.61% -0.08%
==========================================
Files 283 283
Lines 60505 60541 +36
Branches 7538 7540 +2
==========================================
- Hits 54270 54256 -14
- Misses 4081 4113 +32
- Partials 2154 2172 +18 |
…a/citus into alter_database_nonmain2
|
The PR adds ability to propagate "ALTER DATABASE ..." statement from any node with the last commits. It would be good to update the PR description to reflect this. |
gurkanindibay
changed the title
Added some more details to express the any node support |
| alter database "altered_database!'2" owner to test_owner_non_main_db; | ||
| NOTICE: issuing SELECT citus_internal.execute_command_on_remote_nodes_as_user('ALTER DATABASE "altered_database!''2" OWNER TO test_owner_non_main_db;', 'postgres') | ||
| DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx | ||
| \c test_alter_db_from_nonmain_db - - :worker_1_port |
There was a problem hiding this comment.
Can you also add checks to see the result, for example, after the alter database ... owner to ... , connect to another db / worker node and check the result, e.g. select db.datname, pg_catalog.pg_get_userbyid(db.datdba) FROM pg_catalog.pg_database db where db.datname = ....;
There was a problem hiding this comment.
Alternatively, can use the functions that we used before to collect settings of a database from all nodes (via run_command_on_all_nodes etc.), but yes, we definitely somehow need to check database options on all nodes after issuing those commands from a non-main db.
There was a problem hiding this comment.
Added value checks for tests
| EnsureCoordinator(); | ||
|
|
||
| EnsurePropagationToCoordinator(); | ||
| EnsureAllObjectDependenciesExistOnAllNodes(list_make1(dbAddress)); |
There was a problem hiding this comment.
If we have some bugs caused by not calling EnsureAllObjectDependenciesExistOnAllNodes in the functions defined in this file, I think we should not fix them in this PR.
There was a problem hiding this comment.
So can we remove them from the PR?
There was a problem hiding this comment.
You pointed out that we should not fix them in this PR. And I approved it and assumed that there is nothing to do with this comment.
I need more clarification here on what needs to be done here. Are you asking if we can remove selected changes? If so, this block is necessary to propagate non-main db operations, therefore we need to keep it
There was a problem hiding this comment.
let delete trailing newlines
There was a problem hiding this comment.
#7563 is the PR link at all. Do you intend to give a link of comment? If so could you give the exact link/clarify the request?
Thanks
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
I redesigned the sql file after some review comments. Those are the newly formed empty lines removed all of them
| EnsureCoordinator(); | ||
|
|
||
| EnsurePropagationToCoordinator(); | ||
| EnsureAllObjectDependenciesExistOnAllNodes(list_make1(dbAddress)); |
There was a problem hiding this comment.
So can we remove them from the PR?
Co-authored-by: Onur Tirtir <[email protected]>
DESCRIPTION: Adds alter database support on non-main database and on any node
This PR introduces the ability to alter database on main and non-main databases on any node expanding the existing alter database support.
During the implementation, we encountered an issue related to the initialization of the IsMainDB parameter. As a result, we had to turn off enable_ddl_propagation in pg_regress.pl and reactivate it post-creation of the extension in the test environment.
We've added two tests to the test schedules to turn on ddl propagation:
In each schedule, we've placed enable_ddl_propagation (or the other file) at the top to activate ddl propagation system-wide once. This is necessary for the propagation of helper functions.
Above tests are mostly used at the start of each schedule.
Through extensive testing, we discovered that the tests minimal_cluster_management and multi_cluster_management inadvertently disable enable_ddl_propagation on worker nodes. This was causing multi_test_catalog_views to stuck. Upon further investigation, we identified that the function run_command_on_master_and_workers_temp was causing this problem. To address this, we've arranged schedule files such that enable_ddl_propagation is executed inside the minimal_cluster_management and multi_cluster_management schedules.