Hi Diwas, Nice analysis. Please find my comments below. On Mon, Jun 29, 2015 at 10:01:56AM +0530, Diwas Joshi wrote:
hello sergey, so i was reading further on MDEV-8356 last night. There are two main things here: type of sf and return type of sf. Right now the create function... is giving errors at
store_failed= store_failed || table->field[MYSQL_PROC_MYSQL_TYPE]-> store((longlong)type, TRUE);
in sf_create_routine(), which is right since we have been been using TYPE_ENUM_TABLE for m_type so far and mysql.proc takes enum('FUNCTION','PROCEDURE') for type. This can be solved by either changing this enum in mysql.proc definition and including TABLE, or storing table_valued functions as FUNCTION as well. I think first option is better, including TABLE in the enum, since this will help us in distinguishing these functions while executing later if needed.
It is not clear for me which option is better. On one hand, there is your argument that adding 'TABLE' into the enum will make it easier to tell table functions from regular function. On the other hand, try grepping for 'TYPE_ENUM_FUNCTION' in sql/*.{h,cc}. You find a lot of code that checks if m_type == TYPE_ENUM_FUNCTION. You will need to review each check to see what should be done for TYPE_ENUM_FUNCTION. If we can't decide towards one option or another, let's use whatever you prefer right now. If we discover that that works poorly, we will be able to reverse the decision later on.
The second thing is about return type of function. Right now sp_returns_type() method is being used to store the return type of functions. But in our case we have a list of create_fields: List<Create_field> m_cols_list keeping list of attribute names and their types for the table. We need to store all these, for this we can either alter mysql.proc definition to make a field appropriate for storing this list. The type of the field storing return type right now is longblob which as i read further on it is a binary large object that can hold a variable amount of data and are treated as binary strings (byte strings). Maybe we can try to find a way to store all the attribute information in this field only, maybe putting in some delimiters between different field types or some other way since after all its a string. I am still not sure about this problem.
I tried creating this function: CREATE FUNCTION f2(a INT, b VARCHAR(11)) RETURNS VARCHAR(32) DETERMINISTIC BEGIN set @a=3; RETURN @a; END| and mysql.proc.returns has: returns: varchar(32) CHARSET latin1 So, it is a text defition of the return type. It seems natural if for table functions we Is it a problem to do similar thing for Table functions? e.g for a function CREATE FUNCTION f1(a INT, b VARCHAR(11)) RETURNS TABLE t1(id INT, name VARCHAR(11)) BEGIN set @a=3; END| store 'TABLE t1(id INT, name VARCHAR(11)' ? I guess it depends on where the value of mysql.proc.returns is used. Please research this.
Also, another thing is to store the table alias m_table_alias, because queries inside the function will be using it. There is no field present specifically for this in mysql.proc. So either we can create one. There is a field specific_name in it, as i saw in the code is right now storing the same thing as name field. So maybe we can use this field to store the table alias.
I would prefer that m_table_alias was stored as part of the expression in mysql.proc.returns field. Can we try that option first? If we figure that can't work, we can think of other options. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog