Make infer role configurable and fix double parse bug #533
Conversation
src/query_router.rs
Outdated
| @@ -372,6 +372,10 @@ impl QueryRouter { | |||
|
|
|||
| /// Try to infer which server to connect to based on the contents of the query. | |||
| pub fn infer(&mut self, ast: &Vec<sqlparser::ast::Statement>) -> Result<(), Error> { | |||
| if !self.pool_settings.infer_role_from_query { | |||
There was a problem hiding this comment.
Wouldn't it be better to check for this in the client code than here? I think it's confusing to return nothing from a function based on a setting vs. just not calling the function in the first place.
There was a problem hiding this comment.
I think there's arguments for both approaches, I kinda like abstracting away things the client has to do so that's why I went this way. Though if you feel we should put that control on the client then happy to change.
.circleci/pgcat.toml
Outdated
| @@ -74,6 +74,10 @@ default_role = "any" | |||
| # we'll direct it to the primary. | |||
| query_parser_enabled = true | |||
|
|
|||
| # If the query parser is enabled and this setting is enabled, we'll attempt to | |||
| # infer the role from the query itself. | |||
| infer_role_from_query = true | |||
There was a problem hiding this comment.
Should we call it something better like query_parser_read_write_splitting or something less "pgcat"-internals specific and more end-user oriented? I'm open to ideas.
|
Looks pretty good to me. Would be good to add some tests and address the name of the configuration parameter, and some code organization concerns. |
…o parse, and change config name
|
Ready on my end |
Would like to test latency of
QueryPraser::parsebut by default it tries to infer the role. Would like the option to disable that and have query_parser_enabled be a switch to easily turn on and off the query parser to see live latency impact without impacting traffic routing.Fixes bug where first message isn't parsed and subsequent messages are parsed twice.