Konstantin,
Konstantin Osipov wrote:
> * marc.alff@stripped <marc.alff@stripped> [07/11/16 10:38]:
>
>> ChangeSet@stripped, 2007-11-16 00:10:19-07:00, malff@stripped.
> +36 -0
>> Bug#12093 (SP not found on second PS execution if another thread drops other
>> SP in between)
>>
>
> Hi Marc,
>
> Please see my comments inline.
>
> A general comment about prepared statements is that, if for
> backward compatibility purposes only, they should not be
> invalidated, but automatically reprepared.
>
> If the new prepared statement produces a different metadata, then
> and only then we should produce 'statement was invalidated' error.
>
> But since we have discussed this in detail on the phone, I shall
> not explore further.
>
>
This is exactly the opposite of what we discussed 2 days ago on the phone.
Having prepared statements "automatically re-prepare" is a feature request,
and was explicitly out of the scope of this fix.
> I deliberately do not make comments about the coding style --
> there are numerous deviations. I sincerely hope that they all will
> be fixed without me having to point them out.
>
>
>> +/**
>> + This helper ensures that a given Stored Function or Stored Procedure
>> + is present in the corresponding per session cache,
>> + and loads the Stored Function/Procedure from storage into the cache,
>> + if necessary.
>> + @param [in] thd Current thread
>> + @param [in] type Type of stored object to lookup/load
>> + @param [in] name Name of the stored object
>> + @param [out] sphp Stored object refreshed in the cache, if found
>> + @return An error status
>> + @retval 0 Success, sphp will contain the stored object fetched, if any
>> +*/
>> +
>> +static int
>> +sp_cache_load(THD *thd, int type, sp_name *name, sp_head **sphp)
>> +{
>>
>
> <cut>
>
>
>> + if (! sp)
>> + {
>> + switch ((ret= db_find_routine(thd, type, name, &sp)))
>> + {
>> + case SP_OK:
>> + sp_cache_insert(cp, sp);
>> +
>> + if (thd->m_object_observer)
>> + {
>> + sp->make_object_version(& sp_version);
>> +
>> + if (thd->m_object_observer->notify(thd, sp_version))
>>
>
> Let us align the terminology here:
>
> - a version of the object reflects its change over time.
> I.e. for stored procedures we chose md5 to represent a version.
>
Version_id
> - a fully qualified name of the object reflects its place in the
> universe. I.e. for stored procedures a fully qualified name is
> db_name.sp_name
>
There is currently no structure/class for that currently.
For stored procedures only, the fully qualified name is represented by
sp_name,
but other object types don't define an equivalent.
> - object type defines object type
>
Object_type
> - an object identifier is something that identifies the object
> fully, in time, in space and in type.
> In other words, my expectation is that an object identifier is a
> triage <type, fully qualiifed name, version>
>
No, an object identifier identifies an object, and is <type, fully
qualified name>.
Given that an object can have multiple versions, if you want to identify
a specific version,
use Object_version, which is <type, fully qualified name, version>
>
> If I understand the intent correctly, object observer should
> have <object identifier> as the parameter of 'notify'.
>
>
No, Object_version ... notify get the full <type, qualified name, version>,
where the version might be null (using objects that don't exist).
> <object identifier> should be member of the object, in this case,
> please make it a member of class sp_head.
>
>
In principle I agree, and it's actually part of my long term plan
refactoring for this area.
The problem with doing this now is that this new member of sp_head will
then be redundant
with the current key (used for the sp_cache hash), the database and
table name members,
so that a *significant* code cleanup would be needed to avoid such
duplication.
For the short term however, having this version computed on a per-need
basis (the patch)
seems better that bloating the structure with duplicate data.
The trade-off here is more CPU cycles on a very unfrequent case
(prepared statements),
as opposed to a bigger memory footprint all the time.
The same goes for TABLE_SHARE and the table definition cache.
>
>> + DBUG_RETURN(1);
>> + }
>> +
>> + *sphp= sp;
>> + ret= 0;
>> + break;
>> + case SP_KEY_NOT_FOUND:
>> + if (thd->m_object_observer)
>> + {
>> + sp_head::make_object_null_version(& sp_version,
>> + type,
>> + name->m_db,
>> + name->m_name);
>> + if (thd->m_object_observer->notify(thd, sp_version))
>> + DBUG_RETURN(1);
>> + }
>>
>
> Good, OK to create an object identifier for NULL version of
> the object on stack.
>
>
>> +
>> +
>> +/**
>> + Refresh all the routines used by a statement in the session cache.
>> + Note that routines that can be found are reloaded,
>> + while routines that can not be found are ignored.
>> + @param [in] thd Current thread
>> + @param [in] lex Statement semantic tree
>> + @return An error status
>> + @retval 0 Success
>> +*/
>> +int
>> +sp_refresh_routines(THD *thd, LEX *lex)
>> +{
>> + Sroutine_hash_entry *start;
>> + int ret= 0;
>> +
>> + DBUG_ENTER("sp_recache_routines");
>> + start= (Sroutine_hash_entry *)lex->sroutines_list.first;
>> +
>> + for (Sroutine_hash_entry *rt= start; rt; rt= rt->next)
>> + {
>> + sp_name name(thd, rt->key.str, rt->key.length);
>> + int type= rt->key.str[0];
>> + sp_head *sp;
>> +
>> + ret= sp_cache_load(thd, type, &name, &sp);
>> +
>> + if (ret)
>> + break;
>> + }
>> + DBUG_RETURN(ret);
>> +}
>>
>
> OK, but please see below about when this function should be called
> from.
>
> A comment on preloading approach: at first I was opposed to
> pre-loading since we can always load a stored routine on demand,
> e.g. in Item_func_sp::fix_fields().
>
> But then I realized that a statement abort due to a version
> mismatch is not friendly towards non-transactional engines -- we
> should try to compute as much as possible up-front.
>
>
>> +int sp_head::make_object_version(Object_version * version)
>> +{
>> + int rc;
>> +
>> + switch(m_type)
>> + {
>> + case TYPE_ENUM_FUNCTION:
>> + rc= version->m_oid.set_function(m_db, m_name);
>> + break;
>> + case TYPE_ENUM_PROCEDURE:
>> + rc= version->m_oid.set_procedure(m_db, m_name);
>> + break;
>> + case TYPE_ENUM_TRIGGER:
>> + rc= version->m_oid.set_trigger(m_db, m_name);
>> + break;
>> + default:
>> + DBUG_ASSERT(false);
>> + rc= 1;
>> + }
>> +
>> + version->m_vid.set_long(m_modified);
>> +
>> + return rc;
>> +}
>> +
>> +int sp_head::make_object_null_version(
>> + Object_version * version,
>> + int type,
>> + const LEX_STRING& db,
>> + const LEX_STRING& name)
>> +{
>> + int rc;
>> +
>> + switch(type)
>> + {
>> + case TYPE_ENUM_FUNCTION:
>> + rc= version->m_oid.set_function(db, name);
>> + break;
>> + case TYPE_ENUM_PROCEDURE:
>> + rc= version->m_oid.set_procedure(db, name);
>> + break;
>> + case TYPE_ENUM_TRIGGER:
>> + rc= version->m_oid.set_trigger(db, name);
>> + break;
>> + default:
>> + DBUG_ASSERT(false);
>> + rc= 1;
>> + }
>> +
>> + version->m_vid.set_null();
>> +
>> + return rc;
>> +}
>>
>
> OK, no additional comments except the suppressed ones about
> egregious violations of the coding style.
>
>
>> +TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
>> + bool *refresh, uint flags)
>> +{
>> + TABLE *table;
>> +
>> + table= open_table_impl(thd, table_list, mem_root, refresh, flags);
>> +
>> + if (thd->m_object_observer)
>> + {
>> + Object_version table_version;
>> +
>> + if (table)
>> + {
>> + table->s->make_object_version(& table_version);
>> + if (thd->m_object_observer->notify(thd, table_version))
>> + {
>> + *refresh= FALSE;
>> + return(NULL);
>> + }
>> + }
>> + else
>> + {
>> + TABLE_SHARE::make_object_null_version(& table_version,
>> + table_list->db,
>> + table_list->table_name);
>> + if (thd->m_object_observer->notify(thd, table_version))
>> + return(NULL);
>> + }
>> + }
>> + return table;
>>
>
> No comments, since it's on your plan to clean up.
> Table identifier should be a member of TABLE_SHARE.
>
>
See previous comments.
>>
>> +enum Object_type
>> +{
>> + NO_OBJECT_TYPE= 0,
>> + OBJECT_TYPE_TABLE= 1,
>> + OBJECT_TYPE_VIEW= 2,
>> + OBJECT_TYPE_UDF= 3,
>> + OBJECT_TYPE_PROCEDURE= 4,
>> + OBJECT_TYPE_FUNCTION= 5,
>> + OBJECT_TYPE_TRIGGER= 6,
>> + OBJECT_TYPE_EVENT= 7
>> +};
>>
>
> If your task is to produce an exhausitve list, it's more than that:
>
> USER
> TABLESPACE
> PARTITION
>
> And with help of PeterG:
>
> ASSERTION
> CHARACTER SET
> COLLATION
> LOG FILE GROUP
> DOMAIN
> ROLE
> SEQUENCE
> SERVER
> TYPE
>
> I think you should either remove all unused values or add
> everything to the enumeration and mark unused members as such.
>
>
>> +class Object_id
>> +{
>> +public:
>> + Object_id();
>> + Object_id(const Object_id& oid);
>> + ~Object_id() {}
>> +
>> + bool is_equal(const Object_id& oid) const;
>> +
>> + Object_type get_type() const
>> + { return m_type; }
>> +
>> + void reset_type(Object_type type);
>> +
>> + int set(const Object_id& oid);
>> +
>> + int set_table(const char* db, const char* name);
>> +
>> + int set_table(const LEX_STRING& db, const LEX_STRING& name)
>> + {
>> + m_type= OBJECT_TYPE_TABLE;
>> + return set_key('T', db, name);
>> + }
>> +
>> + int set_view(const char* db, const char* name);
>> +
>> + int set_view(const LEX_STRING& db, const LEX_STRING& name)
>> + {
>> + m_type= OBJECT_TYPE_VIEW;
>> + return set_key('V', db, name);
>> + }
>> +
>> + int set_procedure(const LEX_STRING& db, const LEX_STRING& name)
>> + {
>> + m_type= OBJECT_TYPE_PROCEDURE;
>> + return set_key('P', db, name);
>> + }
>> +
>> + int set_function(const LEX_STRING& db, const LEX_STRING& name)
>> + {
>> + m_type= OBJECT_TYPE_FUNCTION;
>> + return set_key('F', db, name);
>> + }
>> +
>> + int set_trigger(const LEX_STRING& db, const LEX_STRING& name)
>> + {
>> + m_type= OBJECT_TYPE_TRIGGER;
>> + return set_key('G', db, name);
>> + }
>> +
>> + uchar* get_hash_key(size_t *length) const
>> + {
>> + *length= m_key_length;
>> + return (uchar*) & m_key[0];
>> + }
>> +
>> + const char* get_printable_type() const;
>> +
>> + const char* get_printable_name() const;
>> +
>> +private:
>> + int set_key(char kind, const LEX_STRING& db, const LEX_STRING& name);
>> +
>> +private:
>> + Object_type m_type;
>> + char m_key[MAX_OBJ_ID_KEY_LEN];
>> + int m_key_length;
>> +
>> +private: // Not implemented, use set()
>> + Object_id& operator = (const Object_id&);
>> +};
>>
>
> Why do you need a separate class for this?
>
>
Because there is the need for a hash code table based keyed by object id
(in the sense: <type, name>),
and that the HASH implementation is so restricted that the only way to
use HASH is to provide something like
Object_id::get_hash_key()
Of course, Object_id could have been completely inlined inside
Object_version and Object_version renamed Object_id,
but doing so perverts the semantic of things IMO, and prevents code
later to use the concept <type, name> when needed,
since there would be no structure to represent that.
>> +
>> +/**
>> + A Version_id identifies a version of a given object.
>> + Different objects (TABLE, VIEW, FUNCTION, PROCEDURE, ...)
>> + can use version ids of different types (MD5, timestampt, counter, ...),
>> + so that a Version_id is an heterogeneous type.
>> + Versions can be compared for equality, but don't define any order.
>> +*/
>> +
>> +class Version_id
>>
>
> Good, Object_version in the terminology above.
>
>
No.
If versions were so simple that it's a single attribute,
the class Object_version would just have the following attributes:
- Object_id
- int version
and this class would not exist.
Now, since versions are implemented differently for different object types,
a class is necessary to hide this complexity -- hence Version_id.
No matter how you name these classes, there should be 3 of them:
- Object_id = <type, name>
- Version_id = <version number / md5 / ??? >
- Object_version = Object_id + Version_id, or <type, name, version>
Anticipating that a Version_id will eventually be stored in a column in
a table
when we have metadata, having a class/struct for it seems very
legitimate to me.
>> +{
>> +public:
>> + Version_id();
>> + Version_id(const Version_id& vid);
>> + ~Version_id();
>> +
>> + bool is_null() const;
>> + bool is_equal(const Version_id& v) const;
>> + bool is_null_or_equal(const Version_id& v) const;
>> +
>> + void set_null();
>> + void set_long(longlong value);
>> + void set_md5(const LEX_STRING& md5);
>> +
>> +private:
>> + enum kind {
>> + NULL_VERSION,
>> + LONG_VERSION,
>> + MD5_VERSION
>> + };
>> +
>> + kind m_kind;
>> + longlong m_long_value;
>> + char m_md5_value[32];
>> +
>> +private: // Non implemented
>> + Version_id& operator = (const Version_id&);
>> +};
>> +
>> +
>> +class Object_version
>>
>
> Good, Object_identifier in the terminology above.
>
No.
Under your definition of object_identifier = <type, name, version>,
object identifier mutates when the object implementation changes ...
>> +{
>> +public:
>> + static void* operator new(size_t size, MEM_ROOT* mem_root) throw();
>> + static void operator delete(void *ptr, MEM_ROOT* mem_root) throw();
>> +
>> + Object_version()
>> + {}
>> +
>> + Object_version(const Object_id& oid, const Version_id& vid)
>> + : m_oid(oid), m_vid(vid)
>> + {}
>> +
>> + ~Object_version() {}
>> +
>> + Object_id m_oid;
>> + Version_id m_vid;
>> +
>> +private: // Non implemented
>> + Object_version(const Object_version&);
>> + Object_version& operator = (const Object_version&);
>> +};
>> +
>> +
>> +class Object_version_collection
>> +{
>> +public:
>> + Object_version_collection()
>> + : m_mem_root(NULL)
>> + {}
>> +
>> + ~Object_version_collection()
>> + {}
>> +
>> + int init(MEM_ROOT *mem_root);
>> + int add(const Object_id& oid, const Version_id & vid);
>> + int find(const Object_id& oid, const Object_version ** over) const;
>> +
>> +private:
>> + HASH m_hash;
>> + MEM_ROOT *m_mem_root;
>> +
>> +private: // Non implemented
>> + Object_version_collection(const Object_version_collection&);
>> + Object_version_collection& operator=(const
> Object_version_collection&);
>> +};
>>
>
> Object_identifier_collection
>
>
No, the collection does track versions.
>> +
>> +/**
>> + Interface class for observers, used to inspect objects referenced
>> + by a statement.
>> + An Object_observer can inspect existing objects,
>> + as well as get notified that some objects used in a statement are missing.
>> + Observers can cause the current operation to fail, by returning an error
>> + from the notification methods.
>> +*/
>> +
>> +class Object_observer
>> +{
>> +protected:
>> + Object_observer() {}
>> + virtual ~Object_observer() {}
>> +
>> +public:
>> + /**
>> + Notify that an object is used in a statement.
>> + @param [in] thd Current thread
>> + @param [in] over object version
>> + @return An error status
>> + @retval 0 Success
>> + */
>> + virtual int notify(THD *thd, const Object_version& over) = 0;
>> +
>> +private: // Non implemented
>> + Object_observer(const Object_observer&);
>> + Object_observer& operator = (const Object_observer&);
>> +};
>>
>
> OK.
>
>
>> +int recheck_all_views(THD *thd, TABLE_LIST *start)
>> +{
>> + Object_observer *observer;
>> + TABLE_LIST *tables;
>> + TABLE_LIST dummy;
>> + int rc;
>> + Object_version view_version;
>> +
>> + DBUG_ASSERT(thd->m_object_observer);
>> +
>> + observer= thd->m_object_observer;
>> +
>> + /*
>> + FIXME: There is no equivalent of sroutines_list for views,
>> + so we have to iterate in TABLE_LIST.
>> + This is poor, since the same view might be expanded
>> + in multiple places, and will cause multiple parsing
>> + */
>> + for (tables= start; tables; tables= tables->next_global)
>> + {
>> + if (tables->view)
>> + {
>> +#ifdef LATER
>> + dummy.db= tables->db;
>> + dummy.table_name= tables->table_name;
>> +#endif
>> +
>> + /*
>> + WARNING: [1] and [2] are due to the implementation of views,
>> + where the view logical name is found:
>> + - sometime in (db, table_name) if the view has not be processed,
>> + - sometime in (view_db, view_name) if the view has been processed.
>> + See the comments in mysql_make_view()
>> + */
>> +
>> + /* [1] mysql_load_view_md5 uses (db, table_name) */
>> + dummy.db= tables->view_db.str;
>> + dummy.table_name= tables->view_name.str;
>> + rc= mysql_load_view_md5(thd, &dummy);
>>
>
> Object identifier should be a member of the object.
>
Which object ?
We don't have any for views, only TABLE_LIST.
Adding a new member in TABLE_LIST just for views would increase the
memory footprint
of the TABLE_LIST array used in a statement, and seems too expensive to me.
> You should not attempt to access view definition outside of
> locking.
>
Which locking ?
Please point me to the code locking a view definition.
> If notify() for views is done in open_table/open_tables,
>
>
>
>> +
>> + if (rc == 0)
>> + {
>> + /* [2] TABLE_LIST::make_object_version uses (view_db, view_name) */
>> + LEX fake_lex;
>> + dummy.view_db= tables->view_db;
>> + dummy.view_name= tables->view_name;
>> + dummy.view= & fake_lex;
>> + rc= dummy.make_object_version(& view_version);
>> + }
>> + else
>> + rc= TABLE_LIST::make_object_null_version(& view_version,
>> + tables->db,
>> + tables->table_name);
>> +
>> + if (rc != 0)
>> + return 1;
>> +
>> + if (observer->notify(thd, view_version))
>> + return 1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>>
>
> I believe you should not need this, but rather call notify() at the
> right place in open_table or open_tables.
>
>
Will investigate
> Speaking of which, I wonder how your patch will work with
> INFORMATION_SCHEMA tables. Temporary tables for these are created
> on the fly, so have a different version each time.
> But table metadata didn't change, so prepared statement should
> continue to function.
>
> Please add a test for that.
>
>
Tests for this case and more have already been implemented, pass, and
will be posted soon.
>> +
>> +int
>> +Prepare_observer::notify(THD *thd, const Object_version& over)
>> +{
>> + switch(over.m_oid.get_type())
>> + {
>> + case OBJECT_TYPE_TABLE:
>> + case OBJECT_TYPE_VIEW:
>> + case OBJECT_TYPE_PROCEDURE:
>> + case OBJECT_TYPE_FUNCTION:
>> + return m_coll->add(over.m_oid, over.m_vid);
>> + default:
>> + return 0;
>> + }
>> +}
>>
>
> OK.
>
>
>> +
>> +bool
>> +Execute_observer::is_safe_table_operation(LEX *lex) const
>> +{
>> + switch(lex->sql_command)
>> + {
>> + case SQLCOM_ALTER_TABLE:
>> + case SQLCOM_REPAIR:
>> + case SQLCOM_ANALYZE:
>> + case SQLCOM_OPTIMIZE:
>> + return TRUE;
>> +
>> + default:
>> + return FALSE;
>> + }
>> +}
>>
>
> I don't understand this, please explain.
>
>
A prepared statement which is "repair table t1" won't complain that the
table has changed,
and can be re-executed even when some DDL affected the table.
Already covered by the extended tests, soon to be posted.
>> +
>> +int
>> +Execute_observer::notify(THD *thd, const Object_version& over)
>> +{
>> + const Object_version *collected;
>> + int rc;
>> +
>> + DBUG_ENTER("Execute_observer::notify()");
>> +
>> + switch(over.m_oid.get_type())
>> + {
>> + case OBJECT_TYPE_TABLE:
>> + case OBJECT_TYPE_VIEW:
>> + rc= m_coll->find(over.m_oid, & collected);
>> + if (rc != 0)
>> + goto err;
>> +
>> + if (collected)
>> + {
>> + if (! collected->m_vid.is_equal(over.m_vid))
>> + {
>> + if (! m_expected_table_change.is_equal(over.m_oid) &&
>> + ! is_safe_table_operation(thd->lex))
>> + {
>> + my_error(ER_PS_INVALIDATED, MYF(0),
>> + over.m_oid.get_printable_type(),
>> + over.m_oid.get_printable_name());
>> + rc= 1;
>> + }
>> + }
>> + }
>> + else
>> + {
>> + Object_id shadowing_oid(over.m_oid);
>> + if (over.m_oid.get_type() == OBJECT_TYPE_TABLE)
>> + shadowing_oid.reset_type(OBJECT_TYPE_VIEW);
>> + else
>> + shadowing_oid.reset_type(OBJECT_TYPE_TABLE);
>> +
>> + rc= m_coll->find(shadowing_oid, & collected);
>> + if (rc != 0)
>> + goto err;
>> +
>> + if (collected)
>> + {
>> + my_error(ER_PS_INVALIDATED, MYF(0),
>> + shadowing_oid.get_printable_type(),
>> + shadowing_oid.get_printable_name());
>> + rc= 1;
>> + }
>> + else
>> + {
>> + /*
>> + - SHOW COLUMNS FROM <table>
>> + --> open the table <table>
>> + - INSTALL PLUGIN ... SONAME ...
>> + --> open the table mysql.plugin
>> + - SHOW CREATE TRIGGER t1_bi
>> + --> open the table t1 (that the trigger relates to)
>> + */
>> + /*
>> + - SHOW COLUMNS FROM <view>
>> + --> open the view <view>
>> + */
>> + if (! over.m_vid.is_null())
>> + rc= m_coll->add(over.m_oid, over.m_vid);
>> + }
>> + }
>> + break;
>> + case OBJECT_TYPE_PROCEDURE:
>> + case OBJECT_TYPE_FUNCTION:
>> + rc= m_coll->find(over.m_oid, & collected);
>> + if (rc != 0)
>> + goto err;
>> +
>> + if (! collected || ! collected->m_vid.is_equal(over.m_vid))
>> + {
>> + if (! is_safe_sp_operation(thd->lex))
>> + {
>> + /*
>> + For these objects, we do require an exact match:
>> + - collected != NULL means the object name must have been seen
>> + during prepare, and not be discovered during execute.
>> + - Version_id::is_equal() means that:
>> + - either the object did not exist, and still does not exist
>> + - or the object existed and is unchanged.
>> + */
>> + my_error(ER_PS_INVALIDATED, MYF(0),
>> + over.m_oid.get_printable_type(),
>> + over.m_oid.get_printable_name());
>> + rc= 1;
>> + }
>> + }
>> + break;
>> + case OBJECT_TYPE_TRIGGER:
>> + case OBJECT_TYPE_UDF:
>> + case OBJECT_TYPE_EVENT:
>> + case NO_OBJECT_TYPE:
>> + rc= 0;
>> + break;
>> + }
>> +
>> +err:
>> + DBUG_RETURN(rc);
>>
>
> Would be nice to have an algorithm description
> in a comment, not inline.
>
> Not following all the details yet.
>
>>
>> + flags((uint) IS_IN_USE),
>> + m_versions(),
>> + m_expected_table_change(),
>> + m_expected_view_change()
>> {
>> init_alloc_root(&main_mem_root,
> thd_arg->variables.query_alloc_block_size,
>> thd_arg->variables.query_prealloc_size);
>> *last_error= '\0';
>> +
>> + if (m_versions.init(& main_mem_root))
>> + {
>> + state= Query_arena::ERROR;
>> + last_errno= ER_OUTOFMEMORY;
>> + sprintf(last_error, ER(ER_OUTOFMEMORY), 0);
>> + }
>>
>
> Why do you need to allocate memory here?
>
The HASH inside m_version needs to be initialized, which allocates memory.
Since we don't use C++ exceptions, and since the return code of
hash_init should not be ignored,
this code can not be implemented in the Object_version_collection
constructor.
> Not necessary to call my_error if out of memory.
>
>
This code is not calling my_error.
This initialization can probably be delayed until
Prepared_statement::prepare(),
so that the code can return a failure on PREPARE instead.
>> /*
>> The only case where we should have items in the thd->free_list is
>> after stmt->set_params_from_vars(), which may in some cases create
>> @@ -2907,6 +3221,14 @@ bool Prepared_statement::prepare(const c
>> if (error == 0)
>> error= check_prepared_statement(this, name.str != 0);
>>
>> + if (error == 0)
>> + error= sp_refresh_routines(thd, thd->lex);
>>
>
> Please move this to open_tables, the branch which is executed when
> the prelocking list is pre-built, after all tables are opened.
>
> This functionality is not specific to prepared statements -- it's
> a coincidence that we invalidate all stored procedures and
> functions at once, when it is not the case, one will have to
> pre-load routines in a similar fashion individually for each stored
> procedure statement.
>
Will investigate
>>
>> +void Prepared_statement::expect_table_change(const Object_id& oid)
>> +{
>> + DBUG_ASSERT(m_expected_table_change.get_type() == NO_OBJECT_TYPE);
>> + m_expected_table_change.set(oid);
>> +}
>> +
>> +void Prepared_statement::expect_view_change(const Object_id& oid)
>> +{
>> + DBUG_ASSERT(m_expected_view_change.get_type() == NO_OBJECT_TYPE);
>> + m_expected_view_change.set(oid);
>>
>
> Hmm, why do you need a separate method for tables and views...
>
expect_view_changes was used during prototyping but is now dead code, it
should have been removed
>
>> +int mysql_load_view_md5(THD *thd, TABLE_LIST *view);
>> +
>>
>
> This should be removed. View definition should never be accessed
> outside of locking. If notification of views is done inside
> open_table/open_tables, this is unnecessary.
>
>
I fail to see how opening a view.frm file within open_table will change
anything regarding locking,
since we simply don't have any locking on views.
The whole point with views is that the view file is opened once, and the
result is inlined in the statement TABLE_LIST,
so that successive calls to open_table do not open the view .frm again.
Implementing what you suggest effectively would disable the validation
of views, and cause the crashes reported.
Please detail what you mean and what you would like to have implemented
here.
> These are all comments on the substance that I have.
> Thank you for working on this.
>
>
