[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! I understand, that Vova said
that it should not be pushed, but Kirill asked me, on
the contrary, to review it. So I do.

On 29/10/2018 22:02, Nikita Pettik wrote:
> Ephemeral space are extensively used in SQL to store intermediate
> results of query processing. To keep things simple, they feature only
> one unique index (primary) which covers all fields. However, ephemeral
> space can be used to store non-unique entries. In this case, one
> additional field added to the end if stored data:
> 
> [field1, ... fieldn, rowid]
> 
> Note that it can't be added to the beginning of tuple since data in
> ephemeral space may be kept as sorted. Previously, to generate proper
> rowid index_max() was used. However, it is obviously wrong way to do it.
> Hence, lets add simple integer counter to memtx space (ephemeral spaces
> are valid only for memtx engine) and introduce method in vtab to fetch
> next rowid value.
> 
> Needed for #3297
> ---
>   src/box/blackhole.c    |  1 +
>   src/box/errcode.h      |  2 ++
>   src/box/memtx_space.c  | 17 +++++++++++++++++
>   src/box/memtx_space.h  |  7 +++++++
>   src/box/space.c        |  9 +++++++++
>   src/box/space.h        |  3 +++
>   src/box/sysview.c      |  1 +
>   src/box/vinyl.c        |  1 +
>   src/errinj.h           |  1 +
>   test/box/errinj.result |  2 ++
>   test/box/misc.result   |  1 +
>   11 files changed, 45 insertions(+)
> 
> index 04f4f34ee..fab8b6617 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -223,6 +223,8 @@ struct errcode_record {
>   	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
>   	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
>   	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
> +	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
> +

This error message as well as check on uint64_max are
not necessary, IMHO. I can not imagine how many hundreds of
years a one should insert into one ephemeral table to
reach this limit.

Also, one extra empty line.

 From this patch I removed errinj and moved rowid_overflow to
the next patch since it stores ids in 32 bits and here
the overflow is much more feasible.

Review fixes here and on the branch:

=============================================================

diff --git a/src/box/errcode.h b/src/box/errcode.h
index fab8b6617..04f4f34ee 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -223,8 +223,6 @@ struct errcode_record {
  	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
  	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
  	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
-	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
-
  
  /*
   * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 075b86f9d..e810011c8 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -606,12 +606,6 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
  {
  	assert(rowid != NULL);
  	struct memtx_space *memtx_space = (struct memtx_space *)space;
-	ERROR_INJECT(ERRINJ_ROWID_OVERFLOW,
-		     { memtx_space->rowid = UINT64_MAX; });
-	if (unlikely(memtx_space->rowid == UINT64_MAX)) {
-		diag_set(ClientError, ER_ROWID_OVERFLOW);
-		return -1;
-	}
  	*rowid = memtx_space->rowid++;
  	return 0;
  }
diff --git a/src/errinj.h b/src/errinj.h
index 58aa5864a..84a1fbb5e 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -121,7 +121,6 @@ struct errinj {
  	_(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
  	_(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
  	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
-	_(ERRINJ_ROWID_OVERFLOW, ERRINJ_BOOL, {.bparam = false}) \
  
  ENUM0(errinj_id, ERRINJ_LIST);
  extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 191e16911..c4a1326cd 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -54,8 +54,6 @@ errinj.info()
      state: false
    ERRINJ_RELAY_REPORT_INTERVAL:
      state: 0
-  ERRINJ_ROWID_OVERFLOW:
-    state: false
    ERRINJ_VY_READ_PAGE_TIMEOUT:
      state: 0
    ERRINJ_XLOG_META: