* Marc Alff <marc.alff@stripped> [07/11/24 03:43]:
> > 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 guess I'm not making my point clear.
A situation when the server starts producing an error
where it would before execute the statement successfully is a
regression.
It may have no widespread impact -- this is not a
question to me, but to you -- please investigate it with support
engineers.
If you ask for my opinion, I believe that to some degree
regression is acceptable: e.g. when a table that is *used* in the
prepared statement was changed.
But currently, because of the choice of the version id, the
regression is severe, so it's fairly obvious that it's unacceptable:
* when FLUSH TABLE or FLUSH TABLES is called, version id mutates
* anything that calls remove_table_from_cache, which includes
OPTIMIZE and REPAIR, changes version id.
* table definition cache is just a cache, so a table definition
can be purged on LRU principle, which also leads to a new
version id being assigned.
"Automatic re-prepare" is a feature request, indeed, but
when applied to the cases when the server would previously
crash or produce bad data -- and, with a stretch -- to the cases
when it may potentially crash or produce bad data in future.
Not when applied to the cases when the server used to work
alright.
> > <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.
Not introducing it along with your patch will make
even gradual re-engineering impossible.
Please add it ignoring the redundancy.
> >> +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 ...
Don't understand the argument, object identifier indeed can mutate
when an object is changed, e.g in ALTER TABLE RENAME.
The main property of the object identifier is its uniqueness.
But let us put the terminology dispute aside for now.
> > 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.
The best candidate for views is TABLE_SHARE.
Unfortunately it's currently not part of the table cache, see in
open_unireg_entry:
/* TODO: Don't free this */
release_table_share(share, RELEASE_NORMAL);
I shall discuss with Dmitri how it can be done.
> > You should not attempt to access view definition outside of
> > locking.
> >
> Which locking ?
> Please point me to the code locking a view definition.
Now it's done under LOCK_open. There is one exception which
represents a lurking bug wherever it's used: mysql_frm_type().
#runtime did not add this exception and adding new race codnitions
to the system should be below our quality standard.
More comments to follow.
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
