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

Make the heartbeat writer use 2 pools #9320

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

GuptaManan100
Copy link
Member

Description

Currently the heartbeat writer uses vt_app user to create the database, table and also run the upsert commands. This isn't correct since vt_app might not have the CREATE privilege, causing the create database and table commands to fail. This PR addresses this issue by creating 2 app pools, one for vt_app to run the insert commands and vt_allprivs to run the create commands.

As part of this PR, WithDDL has also changed, and now takes two functions instead of one in its Execute method. The second function, if provided, is used for running the DDL commands. This allows the caller of this function to have two different execution functions for DDLs and other queries. This change will be helpful in the future too, in all the other usages of WithDDL which are using the vt_app user for running the DDLs too.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

… app to run inserts

Signed-off-by: Manan Gupta <manan@planetscale.com>
Copy link
Member

@5antelope 5antelope left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I wonder if it makes sense to do a round of auditing to make sure the create table ddl for the internal tables (e.g., heartbeat, schema migration, shard split, etc) are all using vt_allprivs instead of vt_app? We ran into a few other privilege cases where we end up switching vt_app to vt_dba for schema migration as well.

Related: idk if it would be an overkill, if we decide vt_allprivs will be responsible for creating all the internal meta tables, would it be helpful to add a check and raise errors if user end up not granting CREATE privilege to it?

@GuptaManan100
Copy link
Member Author

@sonne5 you are right in that we need to go over and look at all the uses of vt_app. There are other places as well where we use vt_app for create statements. We will fix it too but it's going to be a larger refactor

Signed-off-by: Manan Gupta <manan@planetscale.com>
…a Schema error in execution

Signed-off-by: Manan Gupta <manan@planetscale.com>
@deepthi
Copy link
Member

deepthi commented Dec 7, 2021

@sonne5 right now a whole bunch of things fail if I remove CREATE from vt_app, so it's clear that we are relying on that. Also Manan and I went through the code and vt_allprivs is being used in only one place.
There is already a tracking issue for this. We just need to do what that issue is saying.

Signed-off-by: Manan Gupta <manan@planetscale.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Nice work!

@deepthi deepthi merged commit b74fc30 into vitessio:main Dec 8, 2021
@deepthi deepthi deleted the heartbeat-writer-2-pools branch December 8, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Cluster management Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VTTablet heartbeat write should use DBA connection
5 participants