From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 87FCD23F2D for ; Mon, 14 May 2018 07:52:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4cEmUO3eS8rg for ; Mon, 14 May 2018 07:52:10 -0400 (EDT) Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 44E0A23E00 for ; Mon, 14 May 2018 07:52:10 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 4/4] sql: move SQL statistics to server From: "n.pettik" In-Reply-To: <1f4a08f6-366d-8d0c-ec5c-9e8e33a766ee@tarantool.org> Date: Mon, 14 May 2018 14:52:07 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <81C6752B-7CC6-4AED-9199-5471204C18CF@tarantool.org> References: <291aa5961a366c81ffa7ea465636125617a78dad.1524515002.git.korablev@tarantool.org> <1dd6cfc3-25f1-8184-df64-2e1638ad55a0@tarantool.org> <8E69DCFF-0088-4FCD-A4CA-E3C98DEF7274@tarantool.org> <1f4a08f6-366d-8d0c-ec5c-9e8e33a766ee@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >> + dup->samples[i].dlt =3D (uint32_t *)((char*)dup + >> + current_sample_offset = + >> + 2 * size); >=20 > 1. Typo? Maybe array_size instead of size? I slightly refactored it. = Take a look. Looks > more compact, and tests are passed. Yep, it was definitely typo. Anyway, lets use your variant since it = seems to be easier to understand. > I wish it was my task, huge malloc formatting is very interesting. I wish I could understand whether this is sarcasm or not... >=20 > diff --git a/src/box/index_def.c b/src/box/index_def.c > index 1702430b3..d9035e3c5 100644 > --- a/src/box/index_def.c > +++ b/src/box/index_def.c > @@ -205,31 +205,25 @@ index_stat_dup(const struct index_stat *src) > memcpy(dup, src, size); > uint32_t array_size =3D src->sample_field_count * = sizeof(uint32_t); > uint32_t stat1_offset =3D sizeof(struct index_stat); > - dup->tuple_stat1 =3D (uint32_t *)((char*)dup + stat1_offset); > - dup->tuple_log_est =3D (log_est_t *)((char*)dup + stat1_offset + > - array_size + = sizeof(uint32_t)); > - dup->avg_eq =3D (uint32_t *)((char*)dup + stat1_offset + > - 2 * (array_size + sizeof(uint32_t))); > - uint32_t samples_offset =3D stat1_offset + 3 * array_size + > - 2 * sizeof(uint32_t); > - dup->samples =3D (struct index_sample *)((char*)dup + = samples_offset); > - uint32_t current_sample_offset =3D samples_offset + > - src->sample_count * > - sizeof(struct index_sample); > + char *pos =3D (char *) dup + stat1_offset; > + dup->tuple_stat1 =3D (uint32_t *) pos; > + pos +=3D array_size + sizeof(uint32_t); > + dup->tuple_log_est =3D (log_est_t *) pos; > + pos +=3D array_size + sizeof(uint32_t); > + dup->avg_eq =3D (uint32_t *) pos; > + pos +=3D array_size; > + dup->samples =3D (struct index_sample *) pos; > + pos +=3D src->sample_count * sizeof(struct index_sample); > + > for (uint32_t i =3D 0; i < src->sample_count; ++i) { > - dup->samples[i].eq =3D (uint32_t *)((char*)dup + > - = current_sample_offset); > - dup->samples[i].lt =3D (uint32_t *)((char*)dup + > - current_sample_offset = + > - array_size); > - dup->samples[i].dlt =3D (uint32_t *)((char*)dup + > - current_sample_offset = + > - 2 * size); > - dup->samples[i].sample_key =3D > - (char *)((char*)dup + current_sample_offset + 3 = * size); > - current_sample_offset +=3D current_sample_offset + > - 3 * array_size + > - dup->samples[i].key_size + 2; > + dup->samples[i].eq =3D (uint32_t *) pos; > + pos +=3D array_size; > + dup->samples[i].lt =3D (uint32_t *) pos; > + pos +=3D array_size; > + dup->samples[i].dlt =3D (uint32_t *) pos; > + pos +=3D array_size; > + dup->samples[i].sample_key =3D pos; > + pos +=3D dup->samples[i].key_size + 2; > } > return dup; > } Ok, your way rather better. >> +/** >> + * Duplicate index_stat object. >> + * To understand memory layout see index_stat_sizeof() function. >> + * >> + * @param src Stat to duplicate. >> + * @retval Copy of the @src. >=20 > 3. Or NULL. Ok, added clarification. >> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c >> index 57cac437c..4f21cd25f 100644 >> --- a/src/box/sql/analyze.c >> +++ b/src/box/sql/analyze.c >> +decode_stat_string(const char *stat_string, int stat_size, tRowcnt = *stat_exact, >> + LogEst *stat_log) { >> + char *z =3D (char *) stat_string; >=20 > 4. Why do you need this cast? Since implicit cast discards const modifier and leads to compilation = error. >=20 >> + /* >> + * Take a copy of the sample. Add two 0x00 bytes >> + * the end of the buffer. This is in case the >> + * sample record is corrupted. In that case, the >> + * sqlite3VdbeRecordCompare() may read up to two >> + * varints past the end of the allocated buffer >> + * before it realizes it is dealing with a corrupt >> + * record. Adding the two 0x00 bytes prevents this >> + * from causing a buffer overread. >=20 > 5. Are you sure that this 0x0000 is still needed? Tarantool does not = stores > any "corrupted" things. Maybe it is Btree artifacts? >=20 Well, no corruptions - no zero bytes. I have removed mentions about these bytes both from comments and code. Everything seems to be OK. Also, functions which are supposed to rely on this condition (i.e. 0x00 = bytes) are unused. Thus, I will remove them in separate commit. >=20 > 6. How about this diff? Pretty nice, I have updated my branch with your changes.