Bug #53696 Performance schema engine violates the PSEA API by calling my_error()

Description:
The storage engine must not inject errors into the connection diagnostics stack unless so is requested by calling ha_print_error(). Performance schema, however, does it in a number of cases.
This leads to ugliness of special cases for performance schema.

How to repeat:
Inspect the code:
kostja@ibbur:~/work/trunk-runtime/storage/perfschema$ grep 'my_error' *
ha_perfschema.cc:    my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
ha_perfschema.cc:    my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
ha_perfschema.cc:  my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
ha_perfschema.cc:  my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
Binary file libperfschema.a matches
Binary file libperfschema_embedded.a matches
pfs_engine_table.cc:    my_error(ER_WRONG_NATIVE_TABLE_STRUCTURE, MYF(0),
pfs_engine_table.cc:    my_error(ER_WRONG_NATIVE_TABLE_STRUCTURE, MYF(0),
pfs_engine_table.cc:  my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
table_setup_consumers.cc:        my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
table_setup_instruments.cc:        my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
table_setup_objects.cc:        my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));
table_setup_timers.cc:        my_error(ER_WRONG_PERFSCHEMA_USAGE, MYF(0));

Suggested fix:
Do not call my_error(), return a return code and inject the error
in ha_print_error() only.

Thank you for the report.

Verified as described.

Really ha_federated, myisam, innobase and ibmdb2i have such calls too.

Add csv and myisammrg to the list:

csv/ha_tina.cc:      my_error(ER_CHECK_NOT_IMPLEMENTED, MYF(0), "nullable columns");
federated/ha_federated.cc:  my_error(error_num, MYF(0), buf);
federated/ha_federated.cc:  my_error(error_num, MYF(0), error_buffer);
federated/ha_federated.cc:  my_error(retval, MYF(0), error_buffer);
federated/ha_federated.cc:    my_error(error_code, MYF(0), error_buffer);
federated/ha_federated.cc:    my_error(error_code, MYF(0), ER(error_code));
federated/ha_federated.cc:    my_error(ER_CONNECT_TO_FOREIGN_DATA_SOURCE, MYF(0), remote_error_buf);
federated/ha_federated.cc:    my_error(ER_FOREIGN_DATA_SOURCE_DOESNT_EXIST, MYF(0), sql_query.ptr());
ibmdb2i/db2i_charsetSupport.cc:    my_error(ER_OUTOFMEMORY, MYF(0), sizeof(IconvMap));
ibmdb2i/ha_ibmdb2i.cc:    my_error(ER_BLOB_USED_AS_KEY, MYF(0), "*unknown*");
innobase/handler/ha_innodb.cc:		my_error(ER_QUERY_INTERRUPTED, MYF(0));
innobase/handler/ha_innodb.cc:		my_error(ER_TOO_BIG_ROWSIZE, MYF(0),
innobase/handler/ha_innodb.cc:			my_error(EE_OUT_OF_FILERESOURCES,
innobase/handler/ha_innodb.cc:	my_error(ER_CHECK_NOT_IMPLEMENTED, MYF(0), "this functionality");
innobase/handler/ha_innodb.cc:			my_error(ER_WRONG_COLUMN_NAME, MYF(0),
innobase/handler/ha_innodb.cc:		my_error(ER_TABLE_EXISTS_ERROR, MYF(0), buf);
innobase/handler/ha_innodb.cc:		my_error(ER_TABLE_EXISTS_ERROR, MYF(0), to);
innobase/handler/ha_innodb.cc:		my_error(ER_QUERY_INTERRUPTED, MYF(0));
innobase/handler/ha_innodb.cc:				my_error(ER_BINLOG_STMT_MODE_AND_ROW_ENGINE, MYF(0),
innobase/handler/ha_innodb.cc:			my_error(ER_WRONG_NAME_FOR_INDEX, MYF(0),
innobase/handler/handler0alter.cc:				my_error(HA_ERR_TABLE_EXIST, MYF(0),
innobase/handler/handler0alter.cc:				my_error(HA_ERR_TABLE_EXIST, MYF(0), tmp_name);
innobase/handler/handler0alter.cc:		my_error(HA_ERR_TO_BIG_ROW, MYF(0));
innobase/handler/handler0alter.cc:		my_error(ER_PRIMARY_CANT_HAVE_NULL, MYF(0));
innobase/handler/handler0alter.cc:			my_error(ER_REQUIRES_PRIMARY_KEY, MYF(0));
myisam/mi_info.c:  my_error(errcode, MYF(ME_NOREFRESH), file_name);
myisam/myisampack.c:    my_error(EE_OUTOFMEMORY,MYF(ME_BELL),sizeof(uint)*length*2);
myisammrg/ha_myisammrg.cc:    my_error(ER_ADMIN_WRONG_MRG_TABLE, MYF(0), parent_l->alias);

Based on the existing calls to my_error() that originates from other storage engines, I fail to see that a "special case" exists in the performance schema.

There is no functional bug either, the product behaves as expected.

Closing as not a bug.

Guys, (and girls) can you please read the code?

Innobase and MyISAM do not push the error from, say, delete_all_rows(), performance schema does. In fact, both these engines push error in rare special cases.

CSV and others are written by copy-paste programming and do not qualify as good examples.

So, please let me refine this:
The server assumes that no error comes from the engine in cursor calls:
 - delete_all_rows() (see the sql_truncate.cc code to verify)
 - rnd_first, rnd_next(), index_first(), index_next() (see records.cc code, that calls rr_print_error() explicitly after an error, and thus assumes there is no error on the stack to verify).
There may be others. The case in point is delete_all_rows(), which currently demands a special case in the fix for Bug#42643 (please see the latest patch attached to the bug report).

Konstantin,

thank you for the update. Set back to "Verified"

I don't see how errors can be ignored. The effect is that error may appear twice on the stack (i.e. the error list may contain duplicates).

Analysis:

Just found out today that there is a handler::get_error_message() method,
which can be used to customize error messages from a storage engine.

The fix should investigate how to use:
- return HA_ERR_XXX from the performance schema code,
  preferably for existing HA_ERR error codes.
- ha_perfschema::get_error_message()

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/125646

3158 Marc Alff	2010-12-01
      Bug#53696 Performance schema engine violates the PSEA API by calling my_error()
      
      This is a code cleanup.
      
      The implementation of a storage engine (subclasses of handler) is not supposed
      to call my_error() directly inside the engine implementation, 
      but only return error codes, and report errors later at the demand
      of the sql layer only (if needed), using handler::print_error().
      
      This fix removes misplaced calls to my_error(),
      and provide an implementation of print_error() instead.
      
      Given that the sql layer implementation of create table, ha_create_table(),
      does not use print_error() but returns ER_CANT_CREATE_TABLE directly,
      the return code for create table statements using the performance schema
      has changed to ER_CANT_CREATE_TABLE.
      
      Adjusted the test suite accordingly.

Patch approved.

Pushed into:
- mysql-5.5-bugteam
- mysql-trunk-bugfixing

Pushed into mysql-trunk 5.6.1 (revid:alexander.nozdrin@oracle.com-20101205122447-6x94l4fmslpbttxj) (version source revid:alexander.nozdrin@oracle.com-20101205122447-6x94l4fmslpbttxj) (merge vers: 5.6.1) (pib:23)

Code cleanup. No changelog entry needed.

Pushed into mysql-5.5 5.5.9 (revid:georgi.kodinov@oracle.com-20101217124733-p1ivu6higouawv8l) (version source revid:marc.alff@oracle.com-20101201180757-db5woxysu9kenfau) (merge vers: 5.5.8) (pib:24)