MDEV-38600: Annotate the binlogging of recreating MEMORY tables#4942
MDEV-38600: Annotate the binlogging of recreating MEMORY tables#4942
Conversation
andrelkin
left a comment
andrelkin
left a comment
There was a problem hiding this comment.
Howdy Jimmy!
While this looks good, we have had a similar pattern that I suggest to follow.
| query.append(STRING_WITH_LEN("TRUNCATE TABLE ")); | ||
| static const char | ||
| QUERY_START[]= "TRUNCATE TABLE ", | ||
| QUERY_COMMENT[]= " -- auto-generated for recreating memory table"; |
There was a problem hiding this comment.
This is good, though please consider to make it more uniform with an existing pattern of such marking, see it for DROP TABLE specifically.
There was a problem hiding this comment.
This reads better. Thanks!
There was a problem hiding this comment.
The new
/* generated by recreated memory table */
still reads strange to me. I think "generated by server" (from Andrei's suggested pattern) is more clear, as it makes it clear who the actor is (and not some outside source recreating the memory table). Then you can re-use that variable too 🤷
There was a problem hiding this comment.
‘auto-recreated’? ‘restarted’? 🤔
I’m concerned that “generated by server” doesn’t hint at the purpose this TRUNCATE is about.
P.S. Nor that DROP Andrei mentioned… What is it?
The data in MEMORY tables is temporary and is lost when the server restarts. Thus, it is well-intentioned to record TRUNCATE statements to the binary log when these tables are rediscovered empty. However, as entries with no explicit user query associated (especially the lack of SHUTDOWN on crashes), those unaware of this mechanism can find them unexpected, not to mention their significance downstream in replication. This commit adds a comment to these automatically generated TRUNCATE entries to briefly self-describe their source and purpose. As a part of the generated query, this comment is visible together with the TRUNCATE itself in SHOW BINLOG EVENTS and `mariadb-binlog`, while maintaining seamlessness in replication. There are no other changes in behaviour or storage engine API. Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Thanks for the PR, @ParadoxV5 !
I left one comment on an existing thread, but Github PR pages sometimes make that hard to see, so here's the link to my comment.
Also, didn't we discuss writing a warning message to the error log if --init-rpl-role=slave? Granted, there are existing problems with --init-rpl-role, but it at least provides some additional way to get such information. And then init-rpl-role can be extended to be per-domain (or however we design it) later.
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
Also, didn't we discuss writing a warning message to the error log if
--init-rpl-role=slave?
I wonder about the value of having a warning in only the (intermediary) master’s error log, now that this SQL comment is visible directly from both binary and relay log.
--init-rpl-role=master(default):
You were concerned that this is mere log clutter for (top-level) masters, where this TRUNCATE introduces no harm.--init-rpl-role=slave:
The only current purpose this setting serves is to enable semi-sync binlog truncation recovery.
Scalability aside, I wonder whether users proactively set this as if it has a purpose beyond enabling the status variableRpl_status.
| query.append(STRING_WITH_LEN("TRUNCATE TABLE ")); | ||
| static const char | ||
| QUERY_START[]= "TRUNCATE TABLE ", | ||
| QUERY_COMMENT[]= " -- auto-generated for recreating memory table"; |
There was a problem hiding this comment.
‘auto-recreated’? ‘restarted’? 🤔
I’m concerned that “generated by server” doesn’t hint at the purpose this TRUNCATE is about.
P.S. Nor that DROP Andrei mentioned… What is it?
3 participants
The data in MEMORY tables is temporary and is lost when the server restarts.
Thus, it is well-intentioned to record TRUNCATE statements to the binary log when these tables are rediscovered empty.
However, as entries with no explicit user query associated (especially the lack of SHUTDOWN on crashes), those unaware of this mechanism can find them unexpected, not to mention their significance downstream in replication.
This commit adds a comment to these automatically generated TRUNCATE entries to briefly self-describe their source and purpose.
As a part of the generated query, this comment is visible together with the TRUNCATE itself in SHOW BINLOG EVENTS and
mariadb-binlog, while maintaining seamlessness in replication.There are no other changes in behaviour or storage engine API.