List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:November 22 2010 7:18am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229)
WL#5594
View as plain text  

On 11/18/2010 10:44 PM, Guilhem Bichot wrote:
> Hello Jorgen,
>
> I deleted all "ok" pieces where I had nothing to add.
>
> Jorgen Loland a écrit, Le 17.11.2010 13:24:
>> On 11/10/2010 11:05 PM, Guilhem Bichot wrote:
>> > Jorgen Loland a écrit, Le 22.10.2010 10:31:
>> > GB The view "v1" is said to have database "". It's not very intuitive;
>> > I assume it's because the optimizer is rather using a temp table
>> > materializing the view, and such internal temp tables don't have a
>> > database.
>> > So, probably nothing to worry about.
>>
>> Indeed. The trace starts with
>>
>> {
>> "view": {
>> "db": "test",
>> "name": "v1",
>> "in_select#": 1,
>> "materialized": true
>> } /* view */
>> },
>>
>> Maybe we should do like we do for fake select? Print something more
>> understandable? I'm not sure if this is possible; didn't check. I
>> notice that database and tablename is empty for fake selects as well,
>> by the way. Whatever we choose to do about the empty name (if
>> anything) should be done the same way in both cases.
>
> GB I'd say: this is a very small problem, let's ignore it, let's
> continue displaying "" for materialized views and fake selects; I doubt
> people will complain loudly about this.

I agree: we'll ignore this until somebody finds it problematic.

>> If you have suggestions for how to make this more self explanatory,
>> I'm all ears
>
> GB How about replacing ":" with "AND", like this:
> ranges [
> col1 < 5 AND col2 = 5,
> 7 <= col1 AND col2 = 5
> ]
> ?
> "AND" is clear and exists in SQL.
> If we really want a symbol instead of a word, there are:
> - "&&" (at least C-like)
> - or math symbol meaning "and" (unicode 0x2227) or "intersection"
> (unicode 0x2229)
>
> (http://en.wikipedia.org/wiki/Mathematical_operators_and_symbols_in_Unicode#Mathematical_Operators)
>
> but I find those too obscure for many readers, compared to "AND".

"AND" it is.

>> > > + for (st_ror_scan_info **cur_scan= first_scan;
>> > > + cur_scan != last_scan;
>> > > + cur_scan++)
>> > > + {
>> > > + const KEY cur_key=
> param->table->key_info[(*cur_scan)->keynr];
>> > > + const KEY_PART_INFO *key_part= cur_key.key_part;
>> > > +
>> > > + Opt_trace_object trace_isect_idx(trace_ctx);
>> > > + trace_isect_idx.add_str("type", "range_scan").
>> > > + add_str("index", cur_key.name, strlen(cur_key.name)).
>> >
>> > GB there are few add_str() calls in the patch, they may need escaping
>> > or not. The current API is that
>> > - add_str() (with or without a "length" parameter) expects UTF8
>> > without special characters (like, no \n), so does no escaping.
>> > - add_str_escaped() expects UTF8 and does escaping (of
>> > \n and other special characters)
>> > So add_str_escaped() is when data may contain "whatever" (a range
>> > would fall in this category: it can contain a varchar with \n in the
>> > middle...).
>> > add_str_escaped() is slower (tests each byte for escaping), which is
>> > why add_str() exists.
>> > add_str() is ok for hard-coded strings made of A-Za-z0-9_-
>> > (non-special ASCII characters) like
>> > trace->add_str("type", "index_roworder_intersect")
>> > or
>> > trace_summary.add_str("cause", cause);
>> > When the string is variable and user-provided, like the cur_key.name
>> > above, we should use add_str_escaped(). Because it's possible to
>> > create an index named index\n1, as Bar showed to me:
>> > create table... index `index
>> > 1`));
>> > Don't bother with fixing this now. I can, after your push, make a pass
>> > over all add_str() calls and fix what needs to be (there are problems
>> > in my code too).
>> > There are also less performant solutions (but less a burden on the
>> > developer): I can remove add_str_escaped() and make add_str() always
>> > do escaping. Feels stupid to spend CPU time on escaping "cost" which
>> > doesn't need to be escaped, though.
>> > What do you think?
>>
>> The risk is high that we'll call add_str in some places where an
>> add_str_esc is really needed, but on the other hand the consequences
>> are almost negligible as I see it (we'll get some strange characters
>> from a few trace locations, but then we notice it and fix it
>> immediately).
>
> GB yes.
>
>> However, my impression is that the majority (or even
>> all?) strings we get from the server (typically names) will need to be
>> escaped anyway.
>
> GB yes.
>
>> If it is correct that almost all add_str() except the ones with
>> "static labels" ("cause", "cost") need escaping
>
> GB I think this is correct. Table/database names, conditions, ranges,
> need escaping.
>
>> , we also have these
>> possibilities (ignore if assumption is not true):
>>
>> 1)
>> Merge the two add_str and add_str_escaped functions into one, add a
>> default bool that we use only in "labeling" cases:
>>
>> Opt_trace_struct& add_str(const char *key,
>> const char *value, size_t val_length, bool needs_escape= true)
>>
>> or 2)
>> change to add_str() for strings that need escaping and
>> add_str_noescape() for others
>>
>> Both makes it more explicit in the code that the implementor really
>> intended to use the unescaped version of add.
>
> GB Both are good. I'd like to have a short form for both escaped and not
> escaped, it's not great to have "add_long_name("cause", "left join") all
> around". Also, I have the other problem that I'm not happy to be running
> strlen() on fixed strings (as add_str() does), I would like to find a
> way to pass the length determined at compilation time (sizeof()). Need
> to think about it.
>
>> > > @@ -4728,9 +5062,22 @@ TRP_ROR_INTERSECT *get_best_ror_intersec
>> > > if (ror_intersect_add(intersect, cpk_scan, TRUE) &&
>> > > (intersect->total_cost < min_cost))
>> > > {
>> > > + Opt_trace_object (param->thd->opt_trace, "clustered_pk").
>> > > + add("cpk_scan_added_to_intersect", true).
>> > > + add("cumulated_cost", intersect->total_cost);
>> > > cpk_scan_used= TRUE;
>> > > intersect_best= intersect; //just set pointer here
>> > > }
>> > > + else
>> > > + Opt_trace_object (param->thd->opt_trace, "clustered_pk").
>> > > + add("cpk_added_to_intersect", false).add_str("cause", "cost");
>> > > + }
>> > > + else
>> > > + {
>> > > + Opt_trace_object trace_cpk(param->thd->opt_trace,
> "clustered_pk");
>> > > + trace_cpk.add("cpk_added_to_intersect", false);
>> > > + cpk_scan ? trace_cpk.add_str("cause", "ror_is_covering")
>> > > + : trace_cpk.add_str("cause", "no_clustered_pk_index");
>> >
>> > GB The two last lines could be
>> > trace_cpk.add_str("cause", cpk_scan ? "ror_is_covering" :
>> > "no_clustered_pk_index");
>> > (I guess it generates less function calls).
>>
>> I don't see how. My code checks cpk_scan variable and calls add_str()
>> once. Your code checks cpk_scan variable and calls add_str() once.
>> What am I missing?
>
> GB My sentence was imprecise.
> cpk_scan ? trace_cpk.add_str("cause", "ror_is_covering")
> : trace_cpk.add_str("cause", "no_clustered_pk_index");
> translates to
> if (cpk_scan)
> trace_cpk.add_str("cause", "ror_is_covering");
> else
> trace_cpk.add_str("cause", "no_clustered_pk_index");
> which, if the compiler is not optimizing, could put in the final
> assembler code a test and two function calls (even though only one
> function call will be executed, depending on the test's result).
> With
> trace_cpk.add_str("cause", cpk_scan ? "ror_is_covering" :
> "no_clustered_pk_index");
> there will be one test and one function call in the final code.
> I checked that in the generated assembler, at -O2.

This is of course correct, it was just way below my radar of micro-optimization. 
Changed as suggested.

>> > > @@ -6121,6 +6492,11 @@ get_mm_leaf(RANGE_OPT_PARAM *param, Item
>> > > {
>> > > if (type == Item_func::LT_FUNC || type == Item_func::LE_FUNC)
>> > > {
>> > > + Opt_trace_object (param->thd->opt_trace,
> "impossible_condition",
>> > > + Opt_trace_context::RANGE_OPTIMIZER).
>> > > + add_str("table", *field->table_name,
> strlen(*field->table_name)).
>> > > + add_str("field", field->field_name,
> strlen(field->field_name)).
>> > > + add_str("cause", "unsigned_int_cannot_be_negative");
>> >
>> > GB We have three such blocks which follow the same pattern: they
>> > create "impossible_condition", fill "table" and "field" and then
>> > "cause", they differ only by "cause", and then go to label
>> > "end". I propose to reduce code duplication by setting a "const char
>> > *cause" to the cause, and doing the duplicated trace code at label
>> > "end".
>> > I worry a bit about growing the code's size too much: unlike DBUG, the
>> > optimizer trace code will be put into release binaries where speed
>> > matters. My nightmare is that the QA team runs a benchmark and finds a
>> > notable difference between when opt trace is not compiled in and when
>> > it is compiled in.
>>
>> Done, but to which extent does program size degrade performance when
>> there is no difference in the functions that are called?
>
> GB Hard to know. What is easy to imagine is that instructions are too
> spread apart, the CPU's instruction cache will be more often exhausted
> and the system will more frequently have to read instructions from
> memory to fill the cache again; and memory is a bottleneck for the CPU.
> What is more definitely known is that branches (like if()) are a problem
> (heard it from colleagues, read this in a low-level optimization book
> from Intel guys). Branches which the CPU cannot easily predict may make
> the CPU waste time executing the wrong branch and then notice that it
> was wrong, have to discard effects of that wrong branch, and jump to the
> right branch.
> I think we should generally go with code which sounds natural and easy
> to read, except in speed-critical code. When we had the above block
> (with add_str(*field->table_name) etc) repeated in several places, it
> was harder to read to me. In the new patch, those kind of "error cases"
> are pushed to final "gotos", I read this faster. Also, not having code
> duplication is good practice as we know (less to read for the reviewer,
> future reader, maintainer).
> I think the speed wins should more often be gained by new or corrected
> high-level SQL optimizer techniques than by a low-level trick here or
> there. But having no competence in low-level performance tuning, I feel
> vastly incompetent to have a definite position. And performance-critical
> is not always where we expect: there is a pathological case (bug#50595)
> where a 20-table join is intensive on best_access_path() and any change
> to this function has a notable impact on the elapsed time.
>
>> I thought
>> this was negligible - I may be wrong.
>
> GB I do not know, alas.

Ok, I haven't paid attention to micro-optimizations like this. I may have to 
reconsider. Thanks for the heads-up.

>> > > @@ -7434,6 +7509,7 @@ best_access_path(JOIN *join,
>> > > }
>> > > else
>> > > {
>> > > + trace_access_scan.add_str("access_type", "scan");
>> >
>> > GB do we, further down in the trace, tell the user whether
>> > we decided to use _table_ scan or _index_ scan?
>>
>> This decision is made in make_join_readinfo(). I added a tracepoint to
>> this method ("refine_plan") that currently does nothing more than
>> output whether table or index will be scanned. We probably want to add
>> info to this trace object later (but maybe not 1st priority)
>
> GB Good.
> Something I don't understand: in a previous mail, I _think_ you wrote
> that in best_access_path(), "scan" can be index scan or table scan, here:

I did :)

> if (!(records >= s->found_records || best > s->read_time))
> // (1)
> {
> trace_access_scan.add_str("access_type", s->quick ? "range" : "scan");
> trace_access_scan.add("cost", s->read_time).
> add("records", s->found_records).
> add_str("cause", "cost");
>
> goto skip_table_scan;
> }
> But: s->read_time (used above in the comparison) is set at only two
> places (well I grepped for "read_time", so there may be a memcpy() or
> entire JOIN_TAB assignment which I would have missed):
>
> - first place:
> s->read_time= (ha_rows) s->table->file->scan_time();
> and scan_time() is the time of a table scan (see how it's defined in
> handler.h), whereas read_time() and index_only_read_time() seem to be
> about an index scan.
>
> - second place:
> s->read_time= (ha_rows) (s->quick ? s->quick->read_time : 0.0);
> which suggests a range scan (if s->quick is not NULL).
>
> So I interpret that the "scan" which is mentioned in best_access_path(),
> because it depends on s->read_time, can be range scan or table scan but
> not index scan.
> If that is true (?) I would change
> trace_access_scan.add_str("access_type", s->quick ? "range" : "scan");
> to
> trace_access_scan.add_str("access_type", s->quick ? "range" :
> "table_scan");
>
> On the other hand, it is surprising that best_access_path() would not
> consider index scan (i.e. would do all cost calculations assuming the
> choice is between "ref", "range scan" and "table scan"), would decide on
> "table scan" (for example) and would later change its mind, in
> make_join_readinfo(), deciding to convert a table scan to an index scan
> using a rule-based decision (see the piece of code which sets tab->type
> to JT_NEXT: no cost there apparently).
> But maybe that's how it really is.
> I'm quite confused. If you understand anything, let me know maybe then
> we can sort out "scan" vs "table scan"; if not, let's leave "scan" in
> place.

I'm afraid I'm not entirely sure. My initial understanding of this may have been 
incorrect. I'll have to spend some hours investigating to get to the bottom of 
this. Do you think it's worth it at this point? I think it will be 100% clear 
once we get going on the cost-based optimizer project.

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229) WL#5594Jorgen Loland22 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229)WL#5594Guilhem Bichot10 Nov
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229)WL#5594Jorgen Loland17 Nov
      • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229)WL#5594Guilhem Bichot18 Nov
        • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3229)WL#5594Jorgen Loland22 Nov