Add the option to record history or not#31
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new configuration option to control whether gp_relsizes_stats writes collected size data into the table_sizes_history table, enabling users to disable history persistence while still collecting current stats.
Changes:
- Added a new boolean GUC
gp_relsizes_stats.want_to_save_history(defaultfalse) and used it to gate history-table refresh. - Updated regression SQL/expected outputs to cover enabling/disabling history writes.
- Updated README configuration table to document the new parameter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/gp_relsizes_stats.c |
Adds want_to_save_history GUC and conditionally calls update_table_sizes_history(). |
test/sql/gp_relsizes_stats.sql |
Extends regression test script to toggle and validate history writing behavior. |
test/expected/gp_relsizes_stats.out |
Updates expected regression output for the new SQL statements and assertions. |
README.md |
Documents the new gp_relsizes_stats.want_to_save_history configuration option. |
Comments suppressed due to low confidence (1)
test/sql/gp_relsizes_stats.sql:109
- Grammar in the test comment: "history don't write" is ungrammatical and a bit unclear. Consider changing it to something like "history is not written" / "history is not saved" to make the intent unambiguous.
-- Default is want_to_save_history = off — history don't write
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (want_to_save_history) { | ||
| retcode = update_table_sizes_history(); | ||
| if (retcode < 0) { | ||
| error = "relsizes_database_stats_job: updating tables sizes history table failed"; | ||
| goto finish_spi; | ||
| } |
| DefineCustomBoolVariable("gp_relsizes_stats.enabled", "Enable main background worker flag", NULL, &enabled, false, | ||
| PGC_SIGHUP, GUC_NOT_IN_SAMPLE, NULL, NULL, NULL); | ||
| DefineCustomBoolVariable("gp_relsizes_stats.want_to_save_history", "Enable saving table sizes to history table", NULL, &want_to_save_history, false, | ||
| PGC_SIGHUP, GUC_NOT_IN_SAMPLE, NULL, NULL, NULL); |
| -- Default is want_to_save_history = off — history don't write | ||
| SELECT relsizes_stats_schema.relsizes_collect_stats_once(); | ||
| SELECT count(*) FROM relsizes_stats_schema.table_sizes_history | ||
| WHERE relname = 't_history_test'; | ||
|
|
||
| -- Enable option - history should write | ||
| SET gp_relsizes_stats.want_to_save_history = on; |
| @@ -22,12 +41,13 @@ make && make install | |||
|
|
|||
| ### Confguration | |||
Vlasdislav
changed the title
…history a user-facing option
| DefineCustomBoolVariable("gp_relsizes_stats.save_history", | ||
| "Enable saving table sizes to history table.", | ||
| NULL, &save_history, false, | ||
| PGC_USERSET, 0, NULL, NULL, NULL); |
There was a problem hiding this comment.
USERSET is to low for this option. Probably should be SIGHUP
Smyatkin-Maxim
left a comment
Smyatkin-Maxim
left a comment
There was a problem hiding this comment.
There are a few critical problems here to be fixed.
Also while reviewing this PR I've noticed that there might be another problem. My understanding is that the main BG worker doesn't respond to SIGHUP. We need to write a small handler for that and reload parameters in a single point (I think right before checking for gp_relsizes_stats.enabled in main worker is the best spot).
Let's create a separate PR with this, and then get back to this one. Cuz it makes no sense writing save_history option w/o working sighup
| */ | ||
| static void get_stats_for_databases(Oid *databases_oids, int databases_cnt, bool fast) { | ||
| static void get_stats_for_databases(Oid *databases_oids, int databases_cnt, bool fast, bool save_history) { | ||
| for (int i = 0; i < databases_cnt; ++i) { |
There was a problem hiding this comment.
Only here I'd load save_history GUC and pass it to the worker, so that all databases are dumped consistently with or without history. This would eliminate the case when one database is dumped with history and another without.
I think that's the only place where we read from global GUC variable. And run_database_stats_worker and relsizes_database_stats_job are the only places where we have to look for the local copy of this GUC and never look for the global variable.
| BackgroundWorkerUnblockSignals(); | ||
|
|
||
| BackgroundWorkerInitializeConnectionByOid(wa.s.db, InvalidOid); | ||
| ProcessConfigFile(PGC_SIGHUP); |
There was a problem hiding this comment.
That's bad. At the very least it's expensive and should always be guarded by something like if (got_sighup) or if (ConfigReloadPending) or something similar. But in this specific case it breaks the whole logic behind the local variable - it simply ignores it and reads from config.
This code should be removed and local variable shold be used (via DbWorkerArg struct) as proposed in my another comment, and maybe you need the ProcessConfigFile in another place (in case the config reload is pending)
| PGC_SIGHUP, GUC_NOT_IN_SAMPLE, NULL, NULL, NULL); | ||
| DefineCustomBoolVariable("gp_relsizes_stats.save_history", | ||
| "Enable saving table sizes to history table.", | ||
| NULL, &save_history, false, |
There was a problem hiding this comment.
Can't be false by default. It was true before and the expected behavior should not change this way. Let it be true by default.
No description provided.