...

So you're in this context

1 innobase_commit_by_xid(

...

18 thd_binlog_pos(thd, &trx->mysql_log_file_name, 19 &trx->mysql_log_offset); 20 if (trx->mysql_log_offset > 0) 21 trx->flush_log_later = true;

and actually considering `trx->mysql_log_offset` as the return value?

...

Could you please expand on?

My point was just, that what we want to do here is for the (internal) transaction coordinator to tell the storage engine that the engine does not need to fsync() for the commit to be durable, another fsync was already done to ensure this. This is what we in the normal commit path do by calling commit_ordered(), as documented sql/handler.h.

The thd_binlog_pos() is completely unrelated, it's a way for the storage...

I see your point. My patch's block of if (trx_t* trx = trx_get_trx_by_xid(xid)) { + THD *thd = current_thd; + if (thd) + thd_binlog_pos(thd, &trx->mysql_log_file_name, + &trx->mysql_log_offset); + if (trx->mysql_log_offset > 0) + trx->flush_log_later = true; + /* use cases are: disconnected xa, slave xa, recovery */ innobase_commit_low(trx); + trx->mysql_log_file_name = NULL; should not indeed rely on anything that thd_binlog_pos() computes. I think I wanted to spare --skip-log-bin setups from the flush_log_later policy. I see now that alternatively thd_binlog_format(thd) could be used which will always return BINLOG_FORMAT_UNSPEC when that's the case (or @@skip_log_bin=1). You think it's better to covert the block to use that?