[Zeyadzahran]

Jira: MDEV-40294

--start-datetime and --stop-datetime silently treated values outside of the TIMESTAMP range as 1970, so an over-max --start-datetime printed every event and an over-max --stop-datetime printed none.

my_system_gmt_sec() returns 0 and sets ER_WARN_DATA_OUT_OF_RANGE for such values, but convert_str_to_timestamp() never checked it. Now it prints a "Date and time argument out of range" warning and clamps over-max values to the max timestamp.

Tests: new cases in main.mysqlbinlog, which fail without the fix. main.mysqlbinlog and the related binlog tests pass on a debug build.

I am contributing my new code of the whole pull request, including one or several files that are either new files or modified ones, under the BSD-new license.

[CLAassistant]

All committers have signed the CLA.

[gkodinov]

Thank you for your contribution! This is a preliminary review.

Please:

1. address the test failures in buildbot
2. add a commit message to your commit

Otherwise the change seems logical.

I'd do an error since this is changing what's explicitly been specified by the user. But I can live with a warning too. It's something for the final reviewer to decide I guess.

[gkodinov]

LGTM. Please stand by for the final review.

[Zeyadzahran]

Thanks @gkodinov for the review! I addressed both points.

About error vs warning, my first approach was an error, but Jimmy suggested a warning with clamping in Jira, since values like these were accepted before and some scripts might not sanitize their inputs, so an error could break them.

[ParadoxV5]

Thanks for helping out.

[ParadoxV5]

Well that’s a lot of data in the .result file, but only 2 warnings out of 3 test cases.
Did you mean?

Suggested change

	--exec $MYSQL_BINLOG --short-form "--stop-datetime=2106-02-08 00:00:00" $MYSQL_TEST_DIR/std_data/binlog_transaction.000001 2> /dev/null
	--exec $MYSQL_BINLOG --short-form "--stop-datetime=2106-02-08 00:00:00" $MYSQL_TEST_DIR/std_data/binlog_transaction.000001 > /dev/null

If the test focuses on warnings, then it makes sense to squelch the stdout of all of these cases.
But I suppose this test doubles as a max-boundary output test; in this case, it’s better to keep them, with a smaller file to reduce output size.

(FYI, /dev/null might not work on Windoze (MDEV-41085), so error message tests like this use --result-file=path/to/throwaway_file.)

[ParadoxV5]

P.S.
Back-to-back binlog output can easily become unreadable without some --echo labels, though these warnings are sufficient dividers because they’re before the cases’ actual output.

[Zeyadzahran]

The 2> was on purpose because of the Windows issue (your comment on the .result). Now all 4 cases show their warnings, I switched to binlog_savepoint.000001 which is about half the size, and there's no /dev/null anymore. Also added an --echo before each case, it reads better.

[ParadoxV5]

Choosing binlog_savepoint.000001 “for consistency with earlier in this test¹” is okay.

Though, not far above your addition, there’s a test that generates $MYSQLD_DATADIR/master-bin.000001 (and doesn’t purge it in the name of corruption).
Looks like it’ll give an even smaller output.

(Size doesn’t matter too much; it’s about reducing the noise.
The option with the smallest yet sufficient output is most likely to generate a binlog file with only one entry.)

Footnotes

1. apparently the only thing that uses binlog_savepoint.000001 ↩

[Zeyadzahran]

I tried a generated one, but it prints sql_mode and collation_server, which differ between builders, so it needs the same replace_regex as the other cases here. Felt like more noise in the .test than it saves in the .result, so I kept binlog_savepoint.000001.