>> 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?