Re: [Tarantool-patches] [PATCH v1 1/1] sql: define ephemeral space fields

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Aug 13, 2021 at 06:09:19AM +0300, imeevma@tarantool.org wrote:
> diff --git a/src/box/sql.c b/src/box/sql.c
> index c805a1e5c..2b95aed8d 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -243,114 +243,163 @@ tarantoolsqlCount(struct BtCursor *pCur)
>  	return index_count(pCur->index, pCur->iter_type, NULL, 0);
>  }
>  
> -struct space *
> -sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> +struct space_info *
> +sql_space_info_new(uint32_t field_count, uint32_t part_count)
>  {
> -	struct key_def *def = NULL;
> -	uint32_t part_count = field_count;
> -	if (key_info != NULL) {
> -		def = sql_key_info_to_key_def(key_info);
> -		if (def == NULL)
> -			return NULL;
> -		/*
> -		 * In case is_pk_rowid is true we can use rowid
> -		 * as the only part of the key.
> -		 */
> -		if (key_info->is_pk_rowid)
> -			part_count = 1;
> +	assert(field_count > 0);
> +	uint32_t info_size = sizeof(struct space_info);
> +	uint32_t field_size = field_count * sizeof(enum field_type);
> +	uint32_t colls_size = field_count * sizeof(uint32_t);
> +	uint32_t parts_size = part_count * sizeof(uint32_t);
> +	uint32_t size = info_size + field_size + colls_size + parts_size;
> +
> +	struct space_info *info = sqlDbMallocRawNN(sql_get(), size);
> +	if (info == NULL) {
> +		diag_set(OutOfMemory, size, "sqlDbMallocRawNN", "info");
> +		return NULL;
>  	}
> +	info->types = (enum field_type *)((char *)info + info_size);
> +	info->coll_ids = (uint32_t *)((char *)info->types + field_size);
> +	info->parts = part_count == 0 ? NULL :
> +		      (uint32_t *)((char *)info->coll_ids + colls_size);
> +	info->field_count = field_count;
> +	info->part_count = part_count;
> +	info->sort_order = SORT_ORDER_ASC;
>  
> -	struct region *region = &fiber()->gc;
> +	for (uint32_t i = 0; i < field_count; ++i) {
> +		info->types[i] = FIELD_TYPE_SCALAR;
> +		info->coll_ids[i] = COLL_NONE;
> +	}
> +	for (uint32_t i = 0; i < part_count; ++i)
> +		info->parts[i] = i;
> +	return info;
> +}
> +
> +void
> +sql_space_info_from_space_def(struct space_info *info,
> +			      const struct space_def *def)

Separating allocation from initialization only complicates the protocol:
 - The caller must ensure that the arguments passed to alloc and init
   are compatible.
 - There's a window between alloc and init when the object is unusable.

Please don't:

	struct sql_space_info *
	sql_space_info_new_from_space_def(...);

	struct sql_space_info *
	sql_space_info_new_from_index_def(...);

> +{
> +	assert(info->field_count == def->field_count + 1);
> +	assert(info->part_count == 0);
> +	for (uint32_t i = 0; i < def->field_count; ++i) {
> +		info->types[i] = def->fields[i].type;
> +		info->coll_ids[i] = def->fields[i].coll_id;
> +	}
> +	/* Add one more field for rowid. */
> +	info->types[def->field_count] = FIELD_TYPE_INTEGER;
> +	info->coll_ids[def->field_count] = COLL_NONE;
> +}
> +
> +void
> +sql_space_info_from_index_def(struct space_info *info,
> +			      const struct index_def *def)
> +{
> +	assert(info->field_count >= def->key_def->part_count);
> +	assert(info->part_count == 0);
> +	for (uint32_t i = 0; i < def->key_def->part_count; ++i) {
> +		info->types[i] = def->key_def->parts[i].type;
> +		info->coll_ids[i] = def->key_def->parts[i].coll_id;
> +	}
> +}
> +
> +enum {
>  	/*
> -	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
> -	 * and so on. Due to this, length of each name is no more
> -	 * than strlen("_COLUMN_") plus length of UINT32_MAX
> -	 * turned to string, which is 10 and plus 1 for \0.
> +	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2" and so on. Due to
> +	 * this, length of each name is no more than strlen("_COLUMN_") plus
> +	 * length of UINT32_MAX turned to string, which is 10 and plus 1 for \0.
>  	 */
> -	uint32_t name_len = strlen("_COLUMN_") + 11;
> -	uint32_t size = field_count * (sizeof(struct field_def) + name_len) +
> -			part_count * sizeof(struct key_part_def);
> +	NAME_LEN = 19,
> +};

Please move this definition to the function that uses it.

> +
> +struct space *
> +sql_ephemeral_space_new_from_info(const struct space_info *info)

struct space *
sql_ephemeral_space_new(const struct sql_space_info *info);

> +{
> +	uint32_t field_count = info->field_count;
> +	uint32_t part_count = info->parts == NULL ? field_count :
> +			      info->part_count;
> +	uint32_t parts_indent = field_count * sizeof(struct field_def);
> +	uint32_t names_indent = part_count * sizeof(struct key_part_def) +
> +				parts_indent;
> +	uint32_t size = names_indent + field_count * NAME_LEN;
> +
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
>  	struct field_def *fields = region_aligned_alloc(region, size,
>  							alignof(fields[0]));
>  	if (fields == NULL) {
>  		diag_set(OutOfMemory, size, "region_aligned_alloc", "fields");
>  		return NULL;
>  	}
> -	struct key_part_def *ephemer_key_parts =
> -		(void *)fields + field_count * sizeof(struct field_def);
> -	static_assert(alignof(*fields) == alignof(*ephemer_key_parts),
> -		      "allocated in one block, and should have the same "
> -		      "alignment");
> -	char *names = (char *)ephemer_key_parts +
> -		       part_count * sizeof(struct key_part_def);
> -	for (uint32_t i = 0; i < field_count; ++i) {
> -		struct field_def *field = &fields[i];
> -		field->name = names;
> -		names += name_len;
> -		sprintf(field->name, "_COLUMN_%d", i);
> -		field->is_nullable = true;
> -		field->nullable_action = ON_CONFLICT_ACTION_NONE;
> -		field->default_value = NULL;
> -		field->default_value_expr = NULL;
> -		if (def != NULL && i < def->part_count) {
> -			assert(def->parts[i].type < field_type_MAX);
> -			field->type = def->parts[i].type;
> -			field->coll_id = def->parts[i].coll_id;
> -		} else {
> -			field->coll_id = COLL_NONE;
> -			field->type = FIELD_TYPE_SCALAR;
> +	struct key_part_def *parts = (struct key_part_def *)((char *)fields +
> +							     parts_indent);
> +	static_assert(alignof(*fields) == alignof(*parts), "allocated in one "
> +		      "block, and should have the same alignment");
> +	char *names = (char *)fields + names_indent;
> +
> +	for (uint32_t i = 0; i < info->field_count; ++i) {
> +		fields[i].name = &names[i * NAME_LEN];
> +		sprintf(fields[i].name, "_COLUMN_%d", i);
> +		fields[i].is_nullable = true;
> +		fields[i].nullable_action = ON_CONFLICT_ACTION_NONE;
> +		fields[i].default_value = NULL;
> +		fields[i].default_value_expr = NULL;
> +		fields[i].type = info->types[i];
> +		fields[i].coll_id = info->coll_ids[i];
> +	}
> +	if (info->parts == NULL) {
> +		for (uint32_t i = 0; i < part_count; ++i) {
> +			parts[i].fieldno = i;
> +			parts[i].nullable_action = ON_CONFLICT_ACTION_NONE;
> +			parts[i].is_nullable = true;
> +			parts[i].exclude_null = false;
> +			parts[i].sort_order = info->sort_order;
> +			parts[i].path = NULL;
> +			parts[i].type = info->types[i];
> +			parts[i].coll_id = info->coll_ids[i];
> +		}
> +	} else {
> +		for (uint32_t i = 0; i < part_count; ++i) {
> +			uint32_t j = info->parts[i];
> +			parts[i].fieldno = j;
> +			parts[i].nullable_action = ON_CONFLICT_ACTION_NONE;
> +			parts[i].is_nullable = true;
> +			parts[i].exclude_null = false;
> +			parts[i].sort_order = info->sort_order;
> +			parts[i].path = NULL;
> +			parts[i].type = info->types[j];
> +			parts[i].coll_id = info->coll_ids[j];

	for (uint32_t i = 0; i < part_count; ++i) {
		uint32_t j = info->parts != NULL ? info->parts[i] : i;
		parts[i].fieldno = j;
		...
	}

>  		}
>  	}
>  
> -	for (uint32_t i = 0; i < part_count; ++i) {
> -		struct key_part_def *part = &ephemer_key_parts[i];
> -		/*
> -		 * In case we need to save initial order of
> -		 * inserted into ephemeral space rows we use rowid
> -		 * as the only part of PK. If ephemeral space has
> -		 * a rowid, it is always the last column.
> -		 */
> -		uint32_t j = i;
> -		if (key_info != NULL && key_info->is_pk_rowid)
> -			j = field_count - 1;
> -		part->fieldno = j;
> -		part->nullable_action = ON_CONFLICT_ACTION_NONE;
> -		part->is_nullable = true;
> -		part->exclude_null = false;
> -		part->sort_order = SORT_ORDER_ASC;
> -		part->path = NULL;
> -		part->type = fields[j].type;
> -		part->coll_id = fields[j].coll_id;
> -	}
> -	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
> -						      part_count, false);
> -	if (ephemer_key_def == NULL)
> +	struct key_def *key_def = key_def_new(parts, part_count, false);
> +	if (key_def == NULL)
>  		return NULL;
>  
> -	struct index_def *ephemer_index_def =
> -		index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE,
> -			      &index_opts_default, ephemer_key_def, NULL);
> -	key_def_delete(ephemer_key_def);
> -	if (ephemer_index_def == NULL)
> +	const char *name = "ephemer_idx";
> +	struct index_def *index_def = index_def_new(0, 0, name, strlen(name),
> +						    TREE, &index_opts_default,
> +						    key_def, NULL);
> +	key_def_delete(key_def);
> +	if (index_def == NULL)
>  		return NULL;
>  
>  	struct rlist key_list;
>  	rlist_create(&key_list);
> -	rlist_add_entry(&key_list, ephemer_index_def, link);
> +	rlist_add_entry(&key_list, index_def, link);
>  
> -	struct space_def *ephemer_space_def =
> -		space_def_new_ephemeral(field_count, fields);
> -	if (ephemer_space_def == NULL) {
> -		index_def_delete(ephemer_index_def);
> +	struct space_def *space_def = space_def_new_ephemeral(field_count,
> +							      fields);
> +	if (space_def == NULL) {
> +		index_def_delete(index_def);
>  		return NULL;
>  	}
>  
> -	struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def,
> -							      &key_list);
> -	index_def_delete(ephemer_index_def);
> -	space_def_delete(ephemer_space_def);
> +	struct space *space = space_new_ephemeral(space_def, &key_list);
> +	index_def_delete(index_def);
> +	space_def_delete(space_def);
> +	region_truncate(region, svp);
>  
> -	return ephemer_new_space;
> +	return space;
>  }
>  
>  int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index b9107fccc..a3e551017 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -5879,6 +6047,23 @@ sqlSelect(Parse * pParse,		/* The parser context */
>  				      sSort.reg_eph,
>  				      nCols,
>  				      0, (char *)key_info, P4_KEYINFO);
> +		/*
> +		 * We also should define space_info in case this opcode will not
> +		 * be converted to SorterOpen.
> +		 */
> +		uint32_t part_count = sSort.pOrderBy->nExpr + 1;
> +		struct space_info *info = sql_space_info_new(nCols, part_count);
> +		if (info == NULL) {
> +			pParse->is_aborted = true;
> +			goto select_end;
> +		}
> +		key_info->info = info;
> +		if (space_info_for_sorting(info, pParse, sSort.pOrderBy,
> +					   pEList) != 0) {
> +			pParse->is_aborted = true;
> +			goto select_end;
> +		}
> +

I don't understand why you can't pass sql_space_info instead of
sql_key_info. AFAIU, you don't need key_info here at all anymore.
Anyway, keeping a pointer to space_info in key_info looks bad.
Can you avoid that?

>  		sqlVdbeAddOp3(v, OP_IteratorOpen, sSort.iECursor, 0, sSort.reg_eph);
>  		VdbeComment((v, "Sort table"));
>  	} else {
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 1f87e6823..d854562d0 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4055,6 +4057,66 @@ sql_key_info_unref(struct sql_key_info *key_info);
>  struct key_def *
>  sql_key_info_to_key_def(struct sql_key_info *key_info);
>  
> +/**
> + * Structure that is used to store information about ephemeral space field types
> + * and fieldno of key parts.
> + */
> +struct space_info {

sql_space_info

> +	/** Field types of all fields of ephemeral space. */
> +	enum field_type *types;
> +	/** Collation ids of all fields of ephemeral space. */
> +	uint32_t *coll_ids;
> +	/**
> +	 * Fieldno key parts of the ephemeral space. If NULL, then the index
> +	 * consists of all fields in sequential order.
> +	 */
> +	uint32_t *parts;
> +	/** Number of fields of ephemetal space. */
> +	uint32_t field_count;
> +	/**
> +	 * Number of parts in primary index of ephemetal space. If 0 then parts
> +	 * is also NULL.
> +	 */
> +	uint32_t part_count;
> +	/** Sort order of index. */
> +	enum sort_order sort_order;
> +};
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index fcea9eefe..14caa6f82 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2392,10 +2390,11 @@ open_cursor_set_hints:
>  case OP_OpenTEphemeral: {
>  	assert(pOp->p1 >= 0);
>  	assert(pOp->p2 > 0);
> -	assert(pOp->p4type != P4_KEYINFO || pOp->p4.key_info != NULL);
>  
> -	struct space *space = sql_ephemeral_space_create(pOp->p2,
> -							 pOp->p4.key_info);

You don't use p2 anymore. Do you still need to pass it?

> +	struct space_info *info = pOp->p4type == P4_KEYINFO ?
> +				  pOp->p4.key_info->info : pOp->p4.space_info;
> +	assert(info != NULL);
> +	struct space *space = sql_ephemeral_space_new_from_info(info);
>  
>  	if (space == NULL)
>  		goto abort_due_to_error;