[MCC-1138928] Ensure no mergejoin for operations_for_operation_sets #199
Conversation
|
Looks good, needs version bump + changelog 👍 |
| @@ -70,7 +70,10 @@ def self.operations_for_operation_sets(operation_set_ids, operation_names = nil) | |||
| # { 'operation_set_id' => 789, 'unique_identifier' => 'operation2' }, | |||
| # { 'operation_set_id' => 789, 'unique_identifier' => 'operation3' }, | |||
| # ] | |||
| connection.execute(sanitized_query) | |||
| connection.transaction do | |||
There was a problem hiding this comment.
Have you been able to test this out? I recall the behavior of AR transactions with these SET LOCAL commands being kind of hellish. I think it would be safer to just put the SET LOCAL enable_mergejoin TO FALSE at the top of the query string instead?
There was a problem hiding this comment.
Wrapping database calls inside an AR transaction block ensures those statements will be inside a database transaction. If a transaction is already open, it will use that one unless savepoints are created. SET LOCAL must be called within a transaction for it to be applied.
What is important is that SET LOCAL enable_mergejoin TO OFF and the recursive query are both called (in that order) inside the same transaction. I do like the idea of moving it into one though to save one round trip db call.
On a somewhat related note. I was considering running a check to see if enable_mergejoin was on before disabling and reverting back afterwards. I was have concerns about breaking later queries that are not explicitly disabling but rely on it being disabled. This is what dalton should be doing but isn't so I don't want to open that can or worms right now.
There was a problem hiding this comment.
I think this should be okay. Since you're in a transaction, changing LOCAL mergejoin to off here will only apply within the scope of this query, and it should reset afterwards as expected, I think.
There was a problem hiding this comment.
Changing local will apply to the scope of the transaction
There was a problem hiding this comment.
Thanks for doing this; I do vaguely remember discussing this as something I could replace with simpler ActiveRecord queries in Dalton, but I don't remember why that fell through the cracks (maybe I was still figuring out whether I would enable pg_hint_plan?).
I should still look into that after enabling pg_hint_plan everywhere, but thanks for fixing this in the meantime. 😄
ejinotti-mdsol
deleted the
feature/operations-opset-ensure-no-mergejoin
branch
Notes are linked in the ticket
@mdsol/platform-scalability