Support CREATE / DROP database commands from any node

[onurctirtir]

DESCRIPTION: Adds support for issuing CREATE/DROP DATABASE commands from worker nodes

With this commit, we allow issuing CREATE / DROP DATABASE commands from worker nodes too.
As in #7278, this is not allowed when the coordinator is not added to metadata because
we don't ever sync metadata changes to coordinator when adding coordinator to
the metadata via SELECT citus_set_coordinator_host('<hostname>'), or equivalently, via
SELECT citus_add_node(<coordinator_node_name>, <coordinator_node_port>, 0).

We serialize database management commands by acquiring a Citus specific advisory lock
on the first primary worker node if there are any workers in the cluster. As opposed to
what we've done in #7278 for role management
commands, we try to avoid from running into distributed deadlocks as much as possible.
This is because, while distributed deadlocks that can happen around role management
commands can be detected by Citus, this is not the case for database management commands
because most of them cannot be run inside in a transaction block. In that case, Citus
cannot even detect the distributed deadlock because the command is not part of a
distributed transaction at all, then the command execution might not return the control
back to the user for an indefinite amount of time.

[onurctirtir]

Hmm, we have a strange situation where we can't even detect the deadlock?

select citus_add_node('localhost', 9700, 0);
select run_command_on_all_nodes('alter system set citus.enable_create_database_propagation to on');
select run_command_on_all_nodes('select pg_reload_conf()');
select run_command_on_all_nodes('create database onur');

Or equivalently, can use the following script too in order to get rid of the call made to run_command_on_all_nodes():

psql -p 9700 -c "SELECT citus_add_node('localhost', 9700, 0)"
psql -p 9700 -c "SELECT run_command_on_all_nodes('alter system set citus.enable_create_database_propagation to on')"
psql -p 9700 -c "SELECT run_command_on_all_nodes('select pg_reload_conf()')"

dbname="$1"

pids=()

for i in `seq 0 2`; do
    psql -p 970${i} -c "create database $dbname" &
    pids+=($!)
done

for pid in ${pids[*]}; do
    wait $pid
done

[codecov]

Codecov Report

Merging #7359 (11feabd) into main (20dc58c) will decrease coverage by 0.06%.
The diff coverage is 81.13%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7359      +/-   ##
==========================================
- Coverage   89.61%   89.55%   -0.06%     
==========================================
  Files         278      280       +2     
  Lines       60204    60303      +99     
  Branches     7496     7505       +9     
==========================================
+ Hits        53953    54007      +54     
- Misses       4104     4139      +35     
- Partials     2147     2157      +10     

[gurkanindibay]

A good description is needed. AFAIU, Removing coordinator limits the propagation. It should be clearly explained in PR description.

[onurctirtir]

Hmm, we have a strange situation where we can't even detect the deadlock?

select citus_add_node('localhost', 9700, 0);
select run_command_on_all_nodes('alter system set citus.enable_create_database_propagation to on');
select run_command_on_all_nodes('select pg_reload_conf()');
select run_command_on_all_nodes('create database onur');

Or equivalently, can use the following script too in order to get rid of the call made to run_command_on_all_nodes():

psql -p 9700 -c "SELECT citus_add_node('localhost', 9700, 0)"
psql -p 9700 -c "SELECT run_command_on_all_nodes('alter system set citus.enable_create_database_propagation to on')"
psql -p 9700 -c "SELECT run_command_on_all_nodes('select pg_reload_conf()')"

dbname="$1"

pids=()

for i in `seq 0 2`; do
    psql -p 970${i} -c "create database $dbname" &
    pids+=($!)
done

for pid in ${pids[*]}; do
    wait $pid
done

First of all, deadlock detection code doesn't care about the queries
that are not part of a distributed transaction. See the occurrences of
bool onlyDistributedTx = true in distributed_deadlock_detection.c

Setting onlyDistributedTx to true doesn't solve this problem because
AddWaitEdge() then cannot track initiator node for a backend that is not
part of a distributed transaction. This is because, we call
assign_distributed_transaction_id() only for the backends that are part
of a distributed transaction and AddWaitEdge() deduces the initiator
node thanks what assign_distributed_transaction_id() saves in backend data.

And when this info is not provided, distributed deadlock detector cannot
even associate a remote backend to its originating node and cannot detect
the cycles, hence cannot detect the distributed deadlock.

[onurctirtir]

• Improve PR description on SerializeDistributedDDLsOnObjectClass.
• Improve PR description on Support CREATE / DROP database commands from any node #7359 (review).
• Add more tests related to concurrency.

[thanodnl]

Looks good overal

[JelteF]

Sidenote: Should we update this hint message to include a hint about citus.enable_create_database_propagation? In any case this would be better to do in a separate PR.

[onurctirtir]

makes sense, let me update this in a separate PR

[onurctirtir]

#7408, assigned to @gurkanindibay

[onurctirtir]

TODO:

• Re-review changes in database.c
• Add isolation tests:
  Support CREATE / DROP database commands from any node #7359 (comment)
• Add tests around new permission checks.

[JelteF]

super-nit: It would be nice if these errors did not come from another node, but instead the privileges were checked before sending commands there. Now users would get this "confusing" CONTEXT: while executing command on localhost:xxxxx

Probably not important enough to do in this PR. So let's make an issue, seems like a "good first issue".

PS. even when doing this, we'd still want these errors too in case attackers try to call these functions manually.

[onurctirtir]

let me quickly fix that in this PR while we're at it

[JelteF]

Apart from removing (see other comment for details):

case OCLASS_SUBSCRIPTION:

And a few other very minor things I think this is good to merge.