Add a tool to print buffered aggregates to the CLI #298
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will be used in a forthcoming tool to inspect the database contents. https://phabricator.endlessm.com/T35331
wjt
force-pushed
the
T35331-print-aggregates
branch
from
aa60c72
to
add638b
Compare
Previously emer_aggregate_tally_new() would call g_error() in case of an error during setup, which will abort the process. This is inconvenient if one wants to do something different if construction fails, such as in a test suite. Instead, implement GInitable to allow the initialization to fail. Turn the old emer_aggregate_tally_constructed() into the GInitableIface.init implementation. Make emer_aggregate_tally_new() a wrapper around g_initable_new(). https://phabricator.endlessm.com/T35331
wjt
force-pushed
the
T35331-print-aggregates
branch
from
add638b
to
849669b
Compare
This will be useful in an standalone tool to inspect the database, which does not need to write to the database (and indeed may not have permission to). https://phabricator.endlessm.com/T35331
This dumps the contents of the aggregate tally database in a developer-readable form. Like the existing print-persistent-cache tool, this is built but not installed (though you could imagine installing both). https://phabricator.endlessm.com/T35331
wjt
force-pushed
the
T35331-print-aggregates
branch
from
849669b
to
fb59b76
Compare
wjt
commented
Apr 20, 2024
Comment on lines
+194
to
+197
| if (ret) | ||
| ret = CHECK (sqlite3_finalize (stmt)); | ||
| else | ||
| sqlite3_finalize (stmt); |
There was a problem hiding this comment.
This repeated pattern feels really ugly but I can't think of a way to make it more concise without adding yet more macros. The CHECK macro is already a little bit too magic for my tastes today – and I wrote it!
There was a problem hiding this comment.
I tried writing another macro but it actually made it worse.
| @@ -178,16 +187,19 @@ delete_tally_entries (EmerAggregateTally *self, | |||
| } | |||
| g_string_append (query, ");"); | |||
|
|
|||
| if (!CHECK (sqlite3_prepare_v2 (self->db, query->str, -1, &stmt, NULL)) || | |||
| !CHECK (sqlite3_step (stmt)) || | |||
| !CHECK (sqlite3_reset (stmt)) || | |||
There was a problem hiding this comment.
sqlite3_reset() followed by sqlite3_finalize() is redundant, which is why I have removed it.
Previously, in error paths sqlite3_finalize() would not be called on prepared statements. This is a memory leak; it also means that the implicit transaction begun by preparing a statement will remain alive, causing issues later when closing the connection. Instead, call sqlite3_finalize() on all prepared statements. sqlite3_finalize(NULL) is documented to succeed, so we do not need to avoid calling it in the case where sqlite3_prepare_v2() fails. This is all a bit awkward because in the case where we have already hit an error, we want to ignore any error returned by sqlite3_finalize(); but if we have not yet hit an error, we need to catch and propagate any error returned by sqlite3_finalize(). Suggestions for improving this flow welcome! https://phabricator.endlessm.com/T35331
Previously emer_aggregate_tally_iter() and emer_aggregate_tally_iter_before() had no return value and would simply call g_warning() in the error case. This makes it hard to test the error paths without aborting the test program. Make each function return an error and move the g_warning() out into the calling functions. https://phabricator.endlessm.com/T35331
This (effectively) reverts commit a013002. It's been more than 2 years. https://phabricator.endlessm.com/T35331
Previously, the callback passed to emer_aggregate_tally_iter() or emer_aggregate_tally_iter_before() would return a value from the EmerTallyIterResult enum, either CONTINUE or STOP. The iteration logic would stop iteration if the callback returned STOP. (Actually it performed a bitwise comparison, but since the only documented values were 0 or 1 it's the same thing.) This functionality is unused, and has been broken since commit c56d656. In the case where the loop was terminated early by the callback, 'ret' would still hold SQLITE_ROW, which would then be passed to our check_sqlite_error() helper (via the CHECK() macro). This macro considers any value other than SQLITE_OK or SQLITE_DONE to be an error. Rather than try to fix it, we can just remove it without loss of functionality. https://phabricator.endlessm.com/T35331
wjt
force-pushed
the
T35331-print-aggregates
branch
from
fb59b76
to
6dbac4b
Compare
wjt
commented
Apr 25, 2024
| emer_shared_dep, | ||
| ], | ||
| include_directories: include_directories('../daemon'), | ||
| install: false, |
There was a problem hiding this comment.
It would probably be more useful to install this (perhaps under a different name) but I'd want to somehow make it clear that it's a debugging tool rather than something that can be relied upon in scripts, to reserve the right to change it at any time.
|
I read through this and I didn't spot anything amiss. Those sqlite3 docs are … unique. I'll play with this locally, next, just to be sure. |
dylanmccall
approved these changes
Apr 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have written a half-baked version of this useful tool many times. Let's put it in the source tree.
While writing some tests, I found some minor misuses of the sqlite3 API which lead to memory leaks in the error paths of basically every function. So, draft for that reason.
https://phabricator.endlessm.com/T35331