Closed onderkalaci closed 9 years ago
Overall looks good. I think this is a :shipit: after a few changes that I'll add up top.
If you're wondering "why the heck does he add the checklists to the top-level comment rather than down below?" it's because adding them up top gives a little progress bar on the Pull Requests page:
@jasonmp85 I didn't merged this branch and let you do so. I applied check boxes, only not sure about "Short-circuit without caching if GetOperatorByType returns InvalidOid" item.
You suggested me to see analogous code in distribution_metadata.c for a pattern. However, we cannot apply it directly as far as I see. Could you check [my implementation][2], is it OK for you?
[2]: https://github.com/citusdata/pg_shard/pull/57/files#diff-777ad11485b48996843ea21a64147014R281
I said analogous, not identical! You did exactly what I was looking for.
I'm going to close this out myself but need to make one change before I do so: our code style is to always initialize variables. The one place it might be OK to do it on another line is when declaring a local struct
variable, but even then we make sure to memset
them with zeros as soon as possible (i.e. after all other declarations are out of the way). So when you declare operatorId
it should be set to InvalidOid
and oldContext
should initially be set to NULL
.
Looks good. Pushing up the small change I mentioned and merging.
This branch implements a cache in front of GetOperatorByType function, so that it prevents unnecessary calls to GetDefaultOpClass function.
With this implementation, we get the same performance improvement that we observed with @ozgune. I may send the details of the results via an email.
fixes #50
Review tasks:
OperatorType
withOperatorId
(also check comments)GetOperatorByType
returnsInvalidOid
if
statement with the three boolean expressions