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

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

On Sun, Aug 15, 2021 at 04:26:03PM +0300, Mergen Imeev wrote:
> On Fri, Aug 13, 2021 at 06:12:29PM +0300, Vladimir Davydov wrote:
> > 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(...);
> > 
> Fixed. There is still some places that cannot be fixed for now, since
> sql_space_info may be modified during execution of some code.

Please also rename sql_space_info_from_id_list to
sql_space_info_new_from_id_list.

> > struct space *
> > sql_ephemeral_space_new(const struct sql_space_info *info);
> > 
> I would like to do this, but we already have a function with such name. More
> than that, this functions actually have no connection to ephemeral spaces,
> the "spaces" it creates are only needed for creation of normal spaces, i.e.
> it actually creates analogues of space_def. Not sure why space_def cannot be
> used there.

Then you should give those old functions adequate names.
Please rename them in a separate patch and rebase.

> > > 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?
> > 
> The problem here is that OpenTEphemeral opcode can be replaced by SorterOpen
> opcode. Also, its key_info could be moved to OP_Compare opcode. Actually, I
> made a patch which also replaces sql_key_info by space_info for these two
> opcodes, but space_info become too complicated. I decided to leave it for now.
> I actually think that we should drop sorter and OP_Compare, but not sure that
> we should do it now, before release. I will think of it a bit later, if I will
> be given some time to rework this.

I think you should convert space_info to key_info when changing
the opcode then.