Rework struct MEM

[ImeevMA]

I propose to refactor and rework struct MEM. What we have at the moment:

/*
 * Internally, the vdbe manipulates nearly all SQL values as Mem
 * structures. Each Mem struct may cache multiple representations (string,
 * integer etc.) of the same value.
 */
struct Mem {
	union MemValue {
		double r;	/* Real value used when MEM_Real is set in flags */
		i64 i;		/* Integer value used when MEM_Int is set in flags */
		uint64_t u;	/* Unsigned integer used when MEM_UInt is set. */
		bool b;         /* Boolean value used when MEM_Bool is set in flags */
		int nZero;	/* Used when bit MEM_Zero is set in flags */
		void *p;	/* Generic pointer */
		/**
		 * A pointer to function implementation.
		 * Used only when flags==MEM_Agg.
		 */
		struct func *func;
		struct VdbeFrame *pFrame;	/* Used when flags==MEM_Frame */
	} u;
	u32 flags;		/* Some combination of MEM_Null, MEM_Str, MEM_Dyn, etc. */
	/** Subtype for this value. */
	enum sql_subtype subtype;
	/**
	 * If value is fetched from tuple, then this property
	 * contains type of corresponding space's field. If it's
	 * value field_type_MAX then we can rely on on format
	 * (msgpack) type which is represented by 'flags'.
	 */
	enum field_type field_type;
	int n;			/* size (in bytes) of string value, excluding trailing '\0' */
	char *z;		/* String or BLOB value */
	/* ShallowCopy only needs to copy the information above */
	char *zMalloc;		/* Space to hold MEM_Str or MEM_Blob if szMalloc>0 */
	int szMalloc;		/* Size of the zMalloc allocation */
	u32 uTemp;		/* Transient storage for serial_type in OP_MakeRecord */
	sql *db;		/* The associated database connection */
	void (*xDel) (void *);	/* Destructor for Mem.z - only valid if MEM_Dyn */
#ifdef SQL_DEBUG
	Mem *pScopyFrom;	/* This Mem is a shallow copy of pScopyFrom */
	void *pFiller;		/* So that sizeof(Mem) is a multiple of 8 */
#endif
};

Refactoring:

1. Rewrite comments to match the style of our code.
2. Remove u32 uTemp field, as it is not used anywhere.
3. Remove void * pFiller field as it is not used anywhere.
4. Remove sql * db field as it is not needed.
5. Replace i64 i; with int64_t i; (i64 == sql_int64 == long long int).
6. Change field names according to our code style.
7. Rename the structure from struct Mem to struct mem.

Rework:

1. Use field enum mem_type type instead of setting type using flags (Replace MEM_* types by MP_* types and field_types #4906).
2. Move all value and all information about the value to union, except for type and field type.
3. Remove enum field_type field_type field (Remove enum field_type from struct Mem #5957).
4. Make all STRING values not NULL-terminated.
5. Remove void (*xDel) (void *) since we always use sql_free if MEM_Dyn is set.

For STRING, VARBINARY, MAP and ARRAY values, I suggest creating a structure like this:

/** Description of STRING, VARBINARY, MAP or ARRAY values. */
struct {
	/** STRING, VARBINARY, MAP or ARRAY value. */
	char *z;
	/** Allocation type of the value. */
	enum alloc_type alloc_type;
	/** Length of the value. */
	uint32_t size;
	/** Number of zeroes at the end of VARBINARY. */
	uint32_t n_zeroes;
};

If we don't remove MEM_Term, i.e. we can have NULL-terminated STRING values, we can use n_zeroes to determine if there is '\0' at the end. However, in this case it will work differently than in the case of n_zeroes > 0 for VARBINARY: in the case of VARBINARY we have to add an extra '\0' at the end, but in the case of STRING we already have at least one '\0' at the end of the value .

For AGGREGATE mem, I suggest creating a structure like this:

/** Value, used in aggregate functions. */
struct {
	/** Pointer to accumulation structure. */
	void *acc;
	/** Pointer to function. */
	struct func *func;
}

In this case, void *acc; could be replaced with a union containing a pointers to all possible accumulation structures, but I don't think that's necessary.

So, if we make all the suggested changes, the structure should look something like this:

/** Internally, the vdbe manipulates nearly all SQL values as Mem structures. */
struct mem {
	union value {
		/** Double value when MEM type is MEM_TYPE_DOUBLE. */
		double r;
		/** Negative integer value when MEM type is MEM_TYPE_INT. */
		int64_t i;
		/** Unsigned integer value when MEM type is MEM_TYPE_UINT. */
		uint64_t u;
		/** Boolean value when MEM type is MEM_TYPE_BOOL. */
		bool b;
		/** Generic pointer when MEM type is MEM_TYPE_PTR. */
		void *p;
		/** Pointer to frame when MEM type is MEM_TYPE_FRAME. */
		struct VdbeFrame *frame;
		/** True if mem was cleared by OP_Null, not from data. */
		bool is_cleared;
		/** Description of STRING, VARBINARY, MAP or ARRAY values. */
		struct {
			/** STRING, VARBINARY, MAP or ARRAY value. */
			char *z;
			/** Allocation type of the value. */
			enum alloc_type alloc_type;
			/** Length of the value. */
			uint32_t size;
			/** Number of zeroes at the end of VARBINARY. */
			uint32_t n_zeroes;
		};
		/** Value, used in aggregate functions. */
		struct {
			/** Pointer to accumulation structure. */
			void *acc;
			/** Pointer to function. */
			struct func *func;
		}
	} u;
	/** Type of the value this MEM contains. */
	enum mem_type type;
	/** Memory, allocated by MEM. */
	char *buf;
	/** Size of memory allocated by MEM. */
	uint32_t buf_size;
#ifdef SQL_DEBUG
	/** This Mem is a shallow copy of pScopyFrom. */
	struct mem *copy_from;
#endif
};

[Gerold103]

All looks good so far. Except maybe rename is_cleared to is_null_clear? Another question is why aren't buf and buf_size inside of the enum? For instance, how is it possible, that mem.u is an integer, but buf and buf_size are not zeros?

[ImeevMA]

All looks good so far. Except maybe rename is_cleared to is_null_clear

Thanks, I will fix this.

Another question is why aren't buf and buf_size inside of the enum? For instance, how is it possible, that mem.u is an integer, but buf and buf_size are not zeros?

The buf field is a kind of temporary buffer that can contain STRING, VARBINARY, MAP, ARRAY, an accumulation structure for AGGREGATE MEM, or some other temporary value. It is usually freed when the MEM is destroyed or when buf is reused. Also, when MEM_Dyn is set, but that is also a question we should solve and I will describe it a bit later. Answering your last question - yes, it is possible that buf is not NULL and buf_size is not 0 if MEM is int.

[Totktonada]

(I'm not familiar with tarantool's SQL code, so my questions are based only on the proposal above. Just because of curiosity.)

On the first glance it seems logical to use enum field_type, where enum mem_type is proposed now. I see the following differences between them from the proposal above:

• enum mem_type may mean a frame.
• enum mem_type may mean an aggregate function.
• enum mem_type may mean a generic pointer.

So I would ask to show all possible enum mem_type values. Otherwise it is not obvious how it is different from enum field_type.

And, from my point of view, those frame/aggregate function/pointer are not values in the common understanding of this term. Should they actually be part of the struct mem? Or I wrongly understand struct mem as 'an SQL value'?

[ImeevMA]

Description of enum mem_type:

enum mem_type {
	MEM_TYPE_NULL,
	MEM_TYPE_UINT,
	MEM_TYPE_INT,
	MEM_TYPE_STR,
	MEM_TYPE_BIN,
	MEM_TYPE_ARRAY,
	MEM_TYPE_MAP,
	MEM_TYPE_BOOL,
	MEM_TYPE_FLOAT,
	MEM_TYPE_DOUBLE,
	MEM_TYPE_INVALID,
	MEM_TYPE_FRAME,
	MEM_TYPE_PTR,
	MEM_TYPE_AGG,
};

The main problem with enum field_type values is that sometimes we cannot determine which field of the mem->u union we should take. For example, for FIELD_TYPE_INTEGER, we could use mem->u.i or mem->u.u. There will be mem->u.r for FIELD_TYPE_NUMBER in addition to the two mentioned earlier. For FIELD_TYPE_SCALAR, you can take almost any field from the union. I believe this is the reason why we use enum mp_type in serialization instead of enum field_type.

[Gerold103]

Looks good to me.

[tsafin]

I general it all looks good to me, but here is (nob-blocking) question I would like to ask while we are in business of renaming types:

• what about to rename struct mem to something less generic, making more visible the fact that it's SQL specific value type?