MDEV-27837: Disallow SET @@session.server_id within transaction

[shabbann]

Setting server_id within a transaction is dangerous for replication as it
can cause events to be filtered incorrectly on the replica, especially in
parallel replication mode.

This patch adds a check to prevent SET @@session.server_id from being
executed while a transaction is active

[gkodinov]

Thank you for your contribution. This is a preliminary review. Once this is done I will solicit a final review on this.

[gkodinov]

LGTM. Thanks for your quick fixes! Stand by for the final review please.

[grooverdan]

I'm fairly sure we can't add new error messages to old versions. The numbering will conflict/renumber the newer versions.

If there's an unused error message that is still unused in the main branch that can be used. Otherwise the option is to reuse another or generic error message as best as possible.

[shabbann]

I searched for unused error messages in both 10.6 and main and found none.

My idea is either to return to ER_CANT_DO_THIS_DURING_AN_TRANSACTION or to edit only the description (not the code itself) of ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_GTID_DOMAIN_ID_SEQ_NO and add session.server to it

[bnestere]

We shouldn't add this patch to 10.6 anyway. I'd suggest just adding it to the main branch and then we don't have to worry about making error messages consistent across versions.

BTW, @grooverdan , we can add error messages to old versions, but before doing so, we have to backport the latest errmsg-utf8.txt from main to whatever version the error is being added to so things are consistent. At least, that is what @vuvova has told me in the past.

[shabbann]

the test binlog_in_engine.rpl_mysqlbinlog_slave_consistency_oob fails when I try to patch it in main

[bnestere]

Please fix the test appropriately. I'd imagine it tries to set session.server_id in a transaction somewhere. I quickly looked through the file but didn't see where immediately. You should be able to use the failure output to find where this happens. Then, likely, it could probably be set outside of the transaction.

[shabbann]

working on it

[shabbann]

@bnestere

[vuvova]

I'd prefer to avoid a per-variable error messages, better to have one generic one that takes a variable name in %s.

We already have six (!) such error messages per variable (one could've understood where it's going after the second one was added 😄)

[bnestere]

I agree with @vuvova it would indeed be better to make the message generic. Though I'm not sure whether or not we can switch the other error messages to use the new generic one, as it would break consistency when upgrading. @vuvova thoughts?

[vuvova]

I agree with @vuvova it would indeed be better to make the message generic. Though I'm not sure whether or not we can switch the other error messages to use the new generic one, as it would break consistency when upgrading. @vuvova thoughts?

Right, valid concern. There are two approaches here. Only use a generic message from now on, keep old variables on their individual messages. And an approach of ER_DUP_ENTRY vs. ER_DUP_ENTRY_WITH_KEYNAME. Originally it was

ER_DUP_ENTRY
        eng "Duplicate entry '%-.192s' for key %d"

and the usage was (simplified)

  my_error(ER_DUP_ENTRY, MYF(0), val->c_str(), key_num);

And it became

ER_DUP_ENTRY
        eng "Duplicate entry '%-.192T' for key %d"
ER_DUP_ENTRY_WITH_KEY_NAME
        eng "Duplicate entry '%-.64s' for key '%-.192s'"

used as (simplified)

  my_printf_error(ER_DUP_ENTRY, ER(ER_DUP_ENTRY_WITH_KEY_NAME), MYF(0),
    val->c_str(), table->key_info[key_num].name);

That is, it's the same error code, but a different error message.

[gkodinov]

There has been no reply on Serg's comment for 3 weeks now. Converting to Draft. Please re-open when/if you intend to resume working on this.

[shabbann]

hey @gkodinov I was waiting for the reviewers to pick an approach so I could go with it. I didn't think that the decision is up to me :)

[vuvova]

ok,let's use a new common error message from now on:

ER_CANT_SET_IN_TRANSACTION
        eng "Cannot set %s within a transaction"
ER_CANT_SET_IN_SUBSTATEMENT
        eng "Cannot set %s within a trigger or stored program"

And use it for all variables, old too. But preserve old error numbers for old variables. Like

static bool
error_if_in_trans_or_substatement(THD *thd, const char *varname, int in_substatement_error,
                                  int in_transaction_error)
{
  if (thd->in_sub_stmt)
  {
    my_printf_error(in_substatement_error, ER_THD(thd, ER_CANT_SET_IN_SUBSTATEMENT),
                              MYF(0), varname);
    return true;
  }

[gkodinov]

Buildbot fails, please fix that.

Also, if you are set on keeping the two separate commits, please prefix them both with the MDEV- number.

[gkodinov]

this fails as follows in buildbot:

sys_vars.set_server_id_inside_transaction w6 [ fail ]
        Test ended at 2026-07-14 00:52:24
CURRENT_TEST: sys_vars.set_server_id_inside_transaction
mysqltest: At line 14: Unknown SQL error name 'ER_CANT_SET_SERVER_ID_IN_TRANSACTION'