Run ANALYZE after setting up benchmark tables #136
Conversation
mattgara
left a comment
mattgara
left a comment
There was a problem hiding this comment.
LGTM, just had a suggestion to make this behaviour optional with a sensible default.
|
|
||
| ./setup_benchmark_tables.sh -b $BENCHMARK_TYPE -s $SCHEMA_NAME -d $DATA_DIR_NAME | ||
|
|
||
| echo "Running ANALYZE TABLES for schema '$SCHEMA_NAME'..." |
There was a problem hiding this comment.
It probably makes sense to make this optional. Otherwise, for the foreseeable future this would place a CPU worker requirement on this script (due to the requirement on the analyze tables script.)
That is add a command line option to this script that (may be enabled by default) will toggle this behaviour on/off.
There was a problem hiding this comment.
I agree, and I'm in favor of having the option "on" by default.
I think we should also add a check when the option is "on" to make sure we are running on CPU workers (as ANALYZE is currently not supported by GPU workers, as Matt alluded to). If the option is set and we are running on GPU, then I think we should abort the script with an error suggesting we run analyze_tables.sh with CPU workers
|
|
||
| ./setup_benchmark_tables.sh -b $BENCHMARK_TYPE -s $SCHEMA_NAME -d $DATA_DIR_NAME | ||
|
|
||
| echo "Running ANALYZE TABLES for schema '$SCHEMA_NAME'..." |
There was a problem hiding this comment.
I agree, and I'm in favor of having the option "on" by default.
I think we should also add a check when the option is "on" to make sure we are running on CPU workers (as ANALYZE is currently not supported by GPU workers, as Matt alluded to). If the option is set and we are running on GPU, then I think we should abort the script with an error suggesting we run analyze_tables.sh with CPU workers
| --schemas-dir-path $TEMP_SCHEMA_DIR \ | ||
| --data-dir-name $DATA_DIR_NAME | ||
|
|
||
| echo "Running ANALYZE TABLES for schema '$SCHEMA_NAME'..." |
There was a problem hiding this comment.
I think this leads to analyze_tables.sh being called twice, does it not? As setup_benchmark_data_and_tables.sh calls setup_benchmark_tables.sh on line 63?
I think we can just have the one copy here.
|
|
||
| echo "Running ANALYZE TABLES for schema '$SCHEMA_NAME'..." | ||
| ./analyze_tables.sh --schema-name "$SCHEMA_NAME" | ||
| echo "Column statistics refreshed for schema '$SCHEMA_NAME'." |
There was a problem hiding this comment.
Nit: This message does not match the previous message. Consider something along the lines of "ANALYZE TABLES successfully executed for schema '$SCHEMA_NAME'".
|
Updated. Both setup scripts now accept --analyze-tables/--skip-analyze (default on), guard against GPU-only clusters before running ANALYZE, and log “ANALYZE TABLES successfully executed…” when it runs. setup_benchmark_data_and_tables.sh passes the flag through and no longer runs ANALYZE twice. |
4 participants
I’ve reverted the previous stats-checking logic and repurposed the changes so the PR now ensures we automatically run ANALYZE right after tables are created by the setup scripts. This targets your earlier suggestion directly:
setup_benchmark_tables.sh now runs ./analyze_tables.sh --schema-name ... after creating the tables, so column stats are always produced.
setup_benchmark_data_and_tables.sh already calls the tables script, so it inherits the same behavior (no duplicate logic needed).
Both scripts print clear messaging before and after ANALYZE to show progress.