citusdata / citus

Distributed PostgreSQL as an extension
https://www.citusdata.com
GNU Affero General Public License v3.0
10.64k stars 670 forks source link

Use current user for the object propagation #5139

Open onderkalaci opened 3 years ago

onderkalaci commented 3 years ago

With the recent changes, we made metadata syncing to use the current user instead of superuser. Now, the last major operation that uses superuser is the object propagation. If we can make object propagation to use current user, Citus can do more things fully transactional. Plus, it’d remove one of the hard-to-follow tech debts in the code.

And, I was wondering what would it take to achieve that. I changed the code to use the current user (aware of the fact that we don’t have many non-super user tests). And, it seems we don’t need much to do in order to support regular user object propagation. See the changes: https://github.com/citusdata/citus-enterprise/compare/current_user_ensure_dep

However, I think from the users perspective, two things would change (e.g., in a sense breaking change and divergent user experience from Postgres)

From Citus perspective, the above items makes a lot. From Postgres perspective, the above items sound unusual as you do not need this.

I think we might consider guiding users with proper error messages to adjust the ownerships properly, and a GUC to switch back to the current behavior if needed.

onderkalaci commented 2 years ago

Note that to by-pass these issues with citus_add_node, we require citus_add_node be called by superuser via https://github.com/citusdata/citus/pull/5609

Still, this issue should be fixed at some point for other cases, and not requiring superuser for citus_add_node

agedemenli commented 2 years ago

We should consider doing ALTER OWNER in EnsureDependenciesExistOnAllNodes to the current user, for all dependent objects. After everyhing is done using the current user, we can set back the right owners for each object.

However, this approach would be useless if we will have to switch to superuser for executing the ALTER OWNER commands and will face the deadlock issues anyway.

agedemenli commented 2 years ago

https://github.com/citusdata/citus/commit/9b4316434c987810a0532c3cdc2d2be3f7984379

Link to my commit in a stale branch. We might need it if we consider having owner checks for different types of objects