Hello Jorgen,
Jorgen Loland a écrit, Le 22.10.2010 09:04:
> Hi Guilhem
>
> Due to the sheer size of this patch, I have removed comments from you
> which I agree and would only reply "done". Places where I disagree or
> have further information can be found inline (size is now down to 1700
> lines!).
GB only 650 lines now!
> > Jorgen Loland a écrit, Le 07.10.2010 11:02:
> >> #At
>
file:///export/home/jl208045/mysql/wl4800/mysql-next-mr-opt-backporting-wl4800-patchcleanup/
> based on revid:guilhem@stripped
> >>
> >> 3219 Jorgen Loland 2010-10-07
> >> WL#5594 - Add optimizer traces to the range optimizer
> >>
> >> Add optimizer trace points to the range optimizer.
> >> @ mysql-test/include/optimizer_trace.inc
> >> Added tests to cover range optimizer
> >> @ mysql-test/r/optimizer_trace_no_prot.result
> >> Added tests to cover range optimizer. Also adds range
> information to trace of existing queries
> >> @ sql/handler.cc
> >> Add tracepoints to range optimizer
> >> @ sql/opt_range.cc
> >> Add tracepoints to range optimizer
> >> @ sql/opt_range.h
> >> Add tracepoints to range optimizer
> >> @ sql/sql_select.cc
> >> Add tracepoints to range optimizer
> >
> >GB Ah! now I see... this patch is about adding tracepoints to the range
> >optimizer!
>
> You're not in favor of tagging each individual file?
GB ;-) For a patch of this size, I am all in favour. But I find the
per-file commit comments in the new patch more informative because
some don't only repeat the global comment.
> >> === modified file 'mysql-test/r/optimizer_trace_no_prot.result'
> >> --- a/mysql-test/r/optimizer_trace_no_prot.result 2010-10-05
> 13:19:55 +0000
> >> +++ b/mysql-test/r/optimizer_trace_no_prot.result 2010-10-07
> 09:02:31 +0000
> >> @@ -459,9 +615,13 @@ QUERY_ID TRACE MISSING_BYTES_BEYOND_MAX_
> >> "best_access_path": {
> >> "considered_access_paths": [
> >> {
> >> - "access_type": "table scan",
> >> - "cost": 10,
> >> - "records": 0,
> >> + "access_type": "scan",
> >> + "using_range_access": false,
> >
> >GB I wonder how a range access can be used when we are doing a "scan"
> >(which means table scan)? I think range access is only when there is
> >an index...?
>
> Changed to print either
>
> "access_type": "scan"
> or
> "access_type": "range"
>
> but note that scan can also be index scan.
GB Looks like you are right, scan can be index scan. But in the trace,
the opposite of "access_type:scan" is "access_type:index"; maybe this
latter one is misleading: it is about an index, but not about an index
scan (it is "scan" which is about an "index scan" possibly). Then what
is "access_type:index" about? Is it "ref" access, or possibly
something else? Maybe it can only be "ref" access: I see that if we
set best_key, we then set POSITION::key, which is described in
sql_select.h as non-NULL only for "ref" methods. If that's only for
"ref" access, maybe we should say "ref" instead of "index" for
"access_type".
> >> @@ -4252,9 +4885,53 @@ QUERY_ID TRACE MISSING_BYTES_BEYOND_MAX_
> >> "records_estimation": [
> >> {
> >> "table": "D",
> >> - "more_range_optimizer_trace": {
> >> - "TODO?": "yes!"
> >> - } /* more_range_optimizer_trace */
> >> + "range_analysis": {
> >> + "table_scan": {
> >> + "records": 4,
> >> + "cost": 4.9068
> >> + } /* table_scan */,
> >> + "potential_range_indices": [
> >> + {
> >> + "index": "d",
> >> + "key_parts": [
> >> + "d"
> >> + ] /* key_parts */
> >> + }
> >> + ] /* potential_range_indices */,
> >> + "best_full_index_scan": {
> >> + "index": "d",
> >> + "cost": 1.8698,
> >> + "chosen": true
> >> + } /* best_full_index_scan */,
> >> + "group_index_range": {
> >> + "chosen": false,
> >> + "cause": "not_single_table"
> >> + } /* group_index_range */,
> >> + "analyzing_range_alternatives": {
> >> + "range_scan_alternatives": [
> >> + {
> >> + "index": "d",
> >> + "ranges": [
> >> + "3 <= d <= 3"
> >> + ] /* ranges */,
> >> + "index_only": true,
> >> + "records": 1,
> >> + "cost": 2.21,
> >> + "rowid_ordered": true,
> >> + "chosen": false,
> >> + "cause": "higher_cost"
> >
>
> >GB how can a range access with 3 <= d <= 3 (i.e. an index lookup?) be
> >more expensive (than index scan?)?
>
> A guess: there are very few records in D, and full index means that
> the values we're looking for is in the index. Thus,
> best_full_index_scan reads the index only. Further, range access type
> with d=3 is not the same as ref access which is actually chosen a bit
> further down
GB ahah... I can't see how, conceptually, a range access with d=3 is
different from a "ref" access, but this isn't related to your patch,
so you can ignore it.
> I think "best_covering_index_scan" makes this more explicit. Agree?
GB yes
> >> + }
> >> + ]
> >> + }
> >> + }
> >> + ]
> >> + }
> >> + }
> >> + ]
> >> +}
> >> +
> >> +EXPLAIN SELECT key2, MIN(key2_1) FROM t2 GROUP BY key2;
> >> +id select_type table type possible_keys key
> key_len ref rows Extra
> >> +1 SIMPLE t2 range NULL i2_1 4 NULL 103
> Using index for group-by
> >> +
> >> +SELECT TRACE FROM information_schema.OPTIMIZER_TRACE;
> >> +TRACE
> >> +{
> >> + "top_query": "EXPLAIN SELECT key2, MIN(key2_1) FROM t2 GROUP BY
> key2",
> >> + "steps": [
> >> + {
> >> + "expanded_query": "/* select#1 */ select `key2` AS
> `key2`,min(`key2_1`) AS `MIN(key2_1)` from `test`.`t2` group by `key2`"
> >> + },
> >> + {
> >> + "join_preparation": {
> >> + "select#": 1,
> >> + "steps": [
> >> + ]
> >> + }
> >> + },
> >> + {
> >> + "join_optimization": {
> >> + "select#": 1,
> >> + "steps": [
> >> + {
> >> + "constant_tables": [
> >> + ],
> >> + "records_estimation": [
> >> + {
> >> + "table": "t2",
> >> + "const_keys_added": {
> >
> >GB Here again, understanding "const_key" requires knowing code.
>
> Only problem is that I don't fully understand which indices are put
> into const_keys either. If you have a better naming suggestion I will
> change it.
GB No. Let it be. Someone who will be interested in the value of
const_keys in the trace will likely know what const_keys is, or will
find out and enlighten us :-)
> >> +EXPLAIN SELECT * FROM t1 WHERE key2=10 OR key3=3 OR key4 <=> null;
> >> +id select_type table type possible_keys key
> key_len ref rows Extra
> >> +1 SIMPLE t1 index_merge i2,i3,i4 i2,i3 4,4
> NULL 2 Using union(i2,i3); Using where
> >
> >> + "records_estimation": [
> >> + {
> >> + "table": "t2",
> >> + "range_analysis": {
> >> + "table_scan": {
> >> + "records": 1024,
> >> + "cost": 215.15
> >> + },
> >> + "potential_range_indices": [
> >> + {
> >> + "index": "i1a",
> >> + "usable": false,
> >> + "cause": "not_in_keys_to_use"
> >> + },
> >> + {
> >> + "index": "i1b",
> >> + "usable": false,
> >> + "cause": "not_in_keys_to_use"
> >> + },
> >> + {
> >> + "index": "i2_1",
> >> + "key_parts": [
> >> + "key2",
> >> + "key2_1"
> >> + ]
> >GB here we don't have "usable":true. We need to solve the problem of
> >boolean attributes which are sometimes present sometimes absent: when
> >absent, what is their implicit value? Or should we explicitely print
> >"true" for them?
> >For what it's worth, in Python the None value evaluate to false.
>
> Ok, I didn't pay much attention to always display T or F for boolean
> outputs. In this case I added usable T to potential_range_indices:
>
> "index": "i2_1",
> "usable": true,
> "key_parts": [
> "key2",
> "key2_1"
> ] /* key_parts */
GB fine.
> Are you suggesting that any Opt_trace_object that may contain
> <something>:F should in all other cases contain <something>:T? That
> will be a big job...
GB No, I'm not suggesting that, indeed it would be tough.
> >> === modified file 'sql/opt_range.cc'
> >> --- a/sql/opt_range.cc 2010-09-27 09:26:39 +0000
> >> +++ b/sql/opt_range.cc 2010-10-07 09:02:31 +0000
> >GB How about having a "RANGE_OPTIMIZER" flag, used in the constructor of
> >a few top-level objects/arrays of the range opt code, so that a user
> >can request hiding of the trace pieces generated by the range
> >optimizer?
>
> Of course. I have added flag a flag and use it so that when disabled
> we still get table_scan and the range access summary parts which I
> find interesting even if the intermediate decisions are not shown:
>
> Is this acceptable? Should we go as far as
> "range_analysis": "..."
> ?
GB No, the way you did is perfect to me.
> >> + continue;
> >> + }
> >> + key++; // Skip null byte
> >> + store_length--;
> >> + }
> >> + field->set_key_image(key, key_part->length);
> >
>
> >GB do you know what set_key_image() does? I imagine it leaves data in
> >some member of the field; so the content of that member is different
> >depending on whether trace is enabled or not; we need to be sure that
> >this difference is innocuous.
>
> I am pretty sure it sets the key value (less than value, greater than
> value etc) in field's part of record[0] so that field->val_*() (which
> is instructed to read from some offset in record[0]) can output the
> key value as string. I am also pretty sure record[0] is overwritten
> every time new data is requested, so whatever we put in here will not
> be visible when reading records.
GB ok
> Btw: note that this is SergeyP's code
GB yes. And it's us who are going to put it into a production release,
and who will fix the bug if a customer hits one ;-)
> >
> >> + if (field->type() == MYSQL_TYPE_BIT)
> >> + (void) field->val_int_as_str(&tmp, 1);
> >> + else
> >> + field->val_str(&tmp);
> >
> >GB do we need to have such special case for MYSQL_TYPE_BIT?
>
> I have no idea. SP wrote this.
GB Ok, we need to investigate/test this (if not before the push, we
should put it in WL4800_TODO.txt).
> >> + out->append("'",1);
> >
> >GB out->append('\'') should do. And, what does this quote server for? Is
> >it that for a multi-column key we want to print
> >part1'part2'part3?
>
> Yes, that's what it is for. If we go for trace_basic_info() (comment
> further down) we should be able to do without this.
GB but trace_basic_info() uses append_range() which uses print_key2().
> >If this has to be, we see that we compute key+store_length above,
> >compare it to key_end, then we go to the next iteration, where we
> >compute key+store_length again (in the incrementation in the for(...)
> >line) and compare it with key_end again (in the for(...)), it's a bit
> >inefficient.
> >Also, we don't append a quote after NULL (as we just "continue"), this
> >sounds strange. Will a multi-column key (NULL,3,4) be printed as
> >NULL3'4 ? That should be tested.
> >I suggest to test every branch of this function (the BIT one, the
> >quote one, etc) in some .test file.
>
> Ok, but can we reach a conclusion on tracing QUICK vs TRP first?
GB but even if we stick with TRP, trace_basic_info() will remain
unchanged (I think), and it uses append_range() which uses
print_key2() so we need to test every branch of print_key2()...?
> >> @@ -1996,8 +2052,54 @@ public:
> >> }
> >> DBUG_RETURN(quick);
> >> }
> >> + const char *get_name() const {return "range_scan";}
> >> + void trace_basic_info(const PARAM *param, Opt_trace_object
> *trace) const
> >> + {
> >> + trace->add("table_read_plan_type", get_name()).
> >> + add("index",
> param->table->key_info[param->real_keynr[key_idx]].name).
> >
> >GB is usage of real_keynr here inspired from existing places of code?
> >I need to ask, I don't know when real_keynr is good and when not.
> >About the latest patch: keynr_in_table could be const (maybe the
> >compiler can do more optimizations with this information).
>
> Variables storing key number can refer to the position in either table
> (table->key_info[<this>]) or in param (param->real_keynr[<this>]).
If
> the variable is the position in param, we use real_keynr to get the
> position in table.
>
> You really have to check which one you're dealing with for each class.
> TRP::key_idx is key number in param. In the followup patch, I tried to
> make this more explicit. My code was wrong in some places due to
> copy-pasting.
GB ok. I suggest to change
uint key_idx; /* key number in PARAM::key */
to
uint key_idx; /* key number in PARAM::key and PARAM::real_keynr */
> >> + {
> >> + String multikey;
> >> + for (SEL_ARG *part= current; part; part= part->next_key_part)
> >
> >GB "part" could be const. In all logic, in those functions which only do
> >tracing, all pointers to the optimizer's structures should be const,
> >to emphasize that this only updates the trace.
>
> Including TRP_ROR_INTERSECT and others with double-pointers? That
> requires a bunch of casting as well:
>
> from this:
> for (st_ror_scan_info **cur_scan= first_scan;
> to this:
> for (const st_ror_scan_info **cur_scan= (const st_ror_scan_info**)
> first_scan;
>
> That's a lot of noise as I see it, but I'll do it if you insist :)
GB I don't insist when C rules make it hard; as you noted,
const st_ror_scan_info **cur_scan= first_scan;
though logical, won't compile (I just re-read the explanation in some
book, where the author recognizes that this is annoying).
For simple pointers, I find const to be very valuable as
safeguard.
> >GB Could you also, please, add a test for this:
> >- a test of escaping, by storing \n in some range bound of your query,
> >which shows up in the trace via print_key2(), so that we can check
> >that displays well (escaped) and that it's valid JSON (you can use
> >WL4800_validate_json.py on the result file to see)
>
>
> Done.
> Problem: It breaks the line in the trace output. The problem is that
> escape=FALSE in call to do_add(..., bool escape)
GB I will fix that later (see also reply to the new commit).
> >> @@ -2246,12 +2425,10 @@ int SQL_SELECT::test_quick_select(THD *t
> >> + }
> >> +
> >> +free_mem:
> >> + Opt_trace_object trace_range_summary(thd->opt_trace,
> >> +
> "chosen_range_access_summary");
> >> + if (quick)
> >> + {
> >> + /* Guilhem question: I've worked with two alternative ways of
> >> + printing the chosen range access.
> >> +
> >> + 1) Use trace_basic_info() of the TABLE_READ_PLAN object. This
> >> + can print multi-key-part ranges separately (I prefer this)
> >> +
> >> + 2) Use dbug_dump() of the QUICK_SELECT object. Maybe
> >> + conceptually more correct since it is what will be
> >> + executed, but I cannot make it print multi-key-part ranges
> >> + in an understandable way (and I tried a lot).
> >
> >GB you mean that in a QUICK_SELECT object, we don't have all info which
> >trace_basic_info() uses?
> >Assuming we go with trace_basic_info(), do you think that what it
> >prints may be misleading compared to what is in QUICK_SELECT?
>
> The information in QUICK_SELECT is in some compressed form and I am
> unable to find individual ranges. I don't think it misleads in any
> way, it only shows the range in a human readable form.
GB Then let's go for the information source which is easier for you
(TRP I think).
Do I understand correctly that:
- check_quick_select() calls the handler (the storage engine), asking
it to give statistics for a set of ranges; this set is made accessible
to the engine via an "iterator" (sel_arg_range_seq_next())
- this iterator creates range intervals as it runs (so it's quite a
smart iterator), and prints them to the trace?
If you see anything which would be useful to add to the function
comment of sel_arg_range_seq_next(), please don't hesitate (have you
noticed this subtle way to hint that I don't fully understand how this
function works?).
> >> @@ -4683,28 +4947,42 @@ TRP_ROR_INTERSECT *get_best_ror_intersec
> >> ROR_SCAN_INFO **intersect_scans_best;
> >> cur_ror_scan= tree->ror_scans;
> >> intersect_scans_best= intersect_scans;
> >> - while (cur_ror_scan != tree->ror_scans_end &&
> !intersect->is_covering)
> >> {
> >> - /* S= S + first(R); R= R - first(R); */
> >> - if (!ror_intersect_add(intersect, *cur_ror_scan, FALSE))
> >> + Opt_trace_array ota(param->thd->opt_trace,
> "intersecting_indices");
> >> + while (cur_ror_scan != tree->ror_scans_end &&
> !intersect->is_covering)
> >> {
> >> - cur_ror_scan++;
> >> - continue;
> >> - }
> >> + Opt_trace_object trace_idx(param->thd->opt_trace);
> >> + trace_idx.add("index",
> >> +
> param->table->key_info[(*cur_ror_scan)->keynr].name);
> >> + /* S= S + first(R); R= R - first(R); */
> >> + if (!ror_intersect_add(intersect, *cur_ror_scan, FALSE))
> >> + {
> >> + trace_idx.add("used_in_intersect", false).
> >> + add("cause", "cost_not_reduced");
> >
> >GB what does cost_not_reduced mean?
>
> adding this index to the intersect does not reduce the cost of the
> plan. changed to "does_not_reduce_cost_of_intersect"
GB Ok. Maybe then "used_in_intersect" could be "usable"? Trying to
standardize...
>
> >> @@ -4805,6 +5105,12 @@ TRP_ROR_INTERSECT *get_best_covering_ror
> >> ROR_SCAN_INFO **ror_scans_end= tree->ror_scans_end;
> >> DBUG_ENTER("get_best_covering_ror_intersect");
> >>
> >> + // None of our tests enter this function
> >> + Opt_trace_object (param->thd->opt_trace).
> >> + add("get_best_covering_ror_intersect", true).
> >> + add("untested_code", true).
> >> + add("need_tracing",true);
> >
> >GB by "our tests" do you mean the entire mtr testsuite?
>
> yes
GB in another place, the patch uses "done_tracing" instead of
"need_tracing".
> >> @@ -7839,17 +8159,18 @@ walk_up_n_right:
> >> seq->param->is_ror_scan= FALSE;
> >> }
> >> }
> >> - OPT_TRACE2(seq->param->thd->opt_trace,
> >> - print_range(seq->param->thd->opt_trace,
> >> -
> seq->param->table->key_info[seq->real_keyno].key_part,
> >> - range->start_key.key,
> >> - range->start_key.length,
> >> - range->end_key.key,
> >> - range->end_key.length,
> >> - cur->min_key_flag | cur->max_key_flag));
> >>
> >> seq->param->range_count++;
> >>
>
seq->param->max_key_part=max(seq->param->max_key_part,key_tree->part);
> >> +
> >> + if (key_range_trace.length())
> >> + {
> >> + DBUG_ASSERT(seq->param->thd->opt_trace);
> >> + DBUG_ASSERT(seq->param->thd->opt_trace->is_started());
> >> + Opt_trace_array *trace_range=
> >> +
> (Opt_trace_array*)seq->param->thd->opt_trace->get_current_struct();
> >
>
> >GB Do we have a way to pass this array down from the callers, for more
> >safety? Maybe by adding it as a member of "SEL_ARG_RANGE_SEQ seq" (I'm
> >looking for a way which doesn't affect the storage engine API).
>
> Yes, that's possible, but I'm not sure I like that better than getting
> it like here.
>
> >If impossible, it would be better to not cast, and declare trace_range
> >of type Opt_trace_struct*. Indeed, with casting, if the struct happens
> >to be Opt_trace_object, a wrong cast may give a crash including in
> >release binaries. Without casting, the worse which can happen is a
> >JSON syntax error, which asserts in debug builds but is not dangerous
> >in release binaries (see Opt_trace_context::syntax_error()).
>
> Good point! Cast removed.
>
> >I have the goal to make get_current_struct() private soon if
> >possible. So, nobody could access "the current struct"; it could just
> >add objects/arrays (which implicitely adds them as children of the
> >current struct), or pass down an object/array if a function wants to
> >add to it.
>
> Ok, but can I please get this patch in first?
GB yes
> >> @@ -11927,20 +12280,17 @@ static void print_quick(QUICK_SELECT_I *
> >> DBUG_UNLOCK_FILE;
> >> DBUG_VOID_RETURN;
> >> }
> >> -#endif
> >> -
> >>
> >> void QUICK_RANGE_SELECT::dbug_dump(int indent, bool verbose)
> >> {
> >> Opt_trace_context *out= current_thd->opt_trace;
> >> /* purecov: begin inspected */
> >> -#ifndef NO_OPT_TRACE_FOR_RANGE_OPT
> >> - Opt_trace_object oto(out, "range_select");
> >> - oto.add(head->key_info[index].name, "index").
> >> - add(static_cast<ulonglong>(max_used_key_length), "key_length");
> >> + Opt_trace_object oto(out, "range_scan");
> >> + oto.add("index", head->key_info[index].name).
> >> + add("key_length", max_used_key_length).
> >> + add("used_keyparts", used_key_parts);
> >
>
> >GB This isn't covered in optimizer_trace_no_prot; do we need to print to
> >the trace in this dbug_dump()? My experience is that such dbug_dump()
> >is called from several contexts, including some where a trace would
> >not be desirable.
> >It looks like it's only print_quick() which calls dbug_dump()
> >functions, and print_quick() is called only once, around line 2757, in
> >an "else" branch, whereas the alternate branch prints to the optimizer
> >trace; I suggest not making print_quick() print to the opt trace, same
> >for dbug_dump() functions.
>
> I think we should revert print_quick() and dbug_dump() to how they are
> in mysql-next-mr-opt-backporting, i.e, not use them for tracing at all.
GB sounds good; who will do it?
> >> @@ -6970,8 +7037,8 @@ best_access_path(JOIN *join,
> >> loose_scan_opt.next_ref_key();
> >> DBUG_PRINT("info", ("Considering ref access on key %s",
> >>
keyuse->table->key_info[keyuse->key].name));
> >> - Opt_trace_object oto1(thd->opt_trace);
> >> - oto1.add("access_type", "index").add("index", keyinfo->name);
> >> + Opt_trace_object trace_access_idx(thd->opt_trace);
> >> + trace_access_idx.add("access_type", "index").add("index",
> keyinfo->name);
> >>
> >> /*
> >> True if we find some keys from the range optimizer that
> match more
> >> @@ -7027,7 +7094,7 @@ best_access_path(JOIN *join,
> >> */
> >> if (!found_part && !ft_key &&
!loose_scan_opt.have_a_case())
> >> {
> >> - oto1.add("usable", false);
> >> + trace_access_idx.add("usable", false);
> >> goto done_with_index; // Nothing usable found
> >> }
> >>
> >> @@ -7306,7 +7373,7 @@ best_access_path(JOIN *join,
> >> loose_scan_opt.check_ref_access_part2(key, start_key,
> records, tmp);
> >>
> >> } /* not ft_key */
> >> - oto1.add("cost", tmp).add("records", records);
> >> + trace_access_idx.add("records", records).add("cost", tmp);
> >> /** @todo trace quick_matches_more_parts etc? */
> >> if (tmp < best_time - records/(double) TIME_FOR_COMPARE ||
> >> (quick_matches_more_parts &&
> >> @@ -7321,13 +7388,13 @@ best_access_path(JOIN *join,
> >> best_ref_depends_map= found_ref;
> >> }
> >> done_with_index:
> >> - oto1.add("chosen", best_key == start_key);
> >> + trace_access_idx.add("chosen", best_key == start_key);
> >> } /* for each key */
> >> records= best_records;
> >> }
> >>
> >> - Opt_trace_object oto1(thd->opt_trace);
> >> - oto1.add("access_type", "table scan");
> >> + Opt_trace_object trace_access_scan(thd->opt_trace);
> >> + trace_access_scan.add("access_type", "scan");
> >> /*
> >> Don't test table scan if it can't be better.
> >> Prefer key lookup if we would use the same key for scanning.
> >> @@ -7358,27 +7425,31 @@ best_access_path(JOIN *join,
> >> */
> >> if (!(records >= s->found_records || best >
> s->read_time)) // (1)
> >> {
> >> - oto1.add("cost", s->read_time).add("records",
s->found_records);
> >> + trace_access_scan.add("cost", s->read_time).
> >> + add("records", s->found_records).
> >> + add("chosen", false);
> >GB There was a line, which this patch deletes, which made sure to output
> >'"chosen":false' at label skip_table_scan, instead of once per if()
> >block. Why change this?
>
> Because I wanted it to print "chosen:false, cause:<whatever>", and
> <whatever> is different in each if. I could have made a "String cause"
> instead, or I could ignore cost and just print chosen:F. What do you think?
GB you can print cause, but postpone printing "chosen" to a single call
right after skip_table_scan (and not right before), like this:
skip_table_scan:
trace_scan_path.add("chosen", best_key == NULL);
That would replace a few add("chosen",false) calls.
Now sending a reply to the new commit.
