Skip to content

PostgreSQL: Add load step and run_multiple_times.sh for dirty-folio regression repro - #6

Open
salvatoredipietro wants to merge 1 commit into
mainfrom
PG
Open

salvatoredipietro wants to merge 1 commit into
mainfrom
PG

Conversation

@salvatoredipietro

Copy link
Copy Markdown
Contributor

Add load step and run_multiple_times.sh for dirty-folio regression repro

Split pgbench init (load step) from benchmark (run step) so an external
controller can restart PostgreSQL between them to build up dirty-page /
WBT state iteration-over-iteration. Adds PG_LDG_VERSION and
PG_HUGEPAGE_PAD_PERCENT knobs, fixes LDG package install for Debian vs
RPM, bumps pg_isready timeout to 180s, and adds ulimit -n guards.

run_multiple_times.sh drives the full repro: provisions once, then loops
N iterations (PG restart → load → PG restart → run) on a persistent
cluster so the regression accumulates predictably.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@salvatoredipietro salvatoredipietro changed the title Add load step and run_multiple_times.sh for dirty-folio regression repro PostgreSQL: Add load step and run_multiple_times.sh for dirty-folio regression repro Jul 31, 2026
# iteration-over-iteration. That accumulation is the regression.
#
# WHY THIS DIFFERS FROM A NAIVE `run.sh ... ` PER ITERATION:
# The stock default step list is `install configure run results cleanup`. Running

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The premise here is wrong. This script passes an explicit op list (./run.sh postgresql SUT load). Since repro:run takes ops positionally, "provision once, then loop a subset" is a first-class framework capability, not something needing an external driver. This entire script is trying to work around a limitation that isn't there, and rewriting framework functionality it doesn't need to.

A main.sh needs to exist in each repro, otherwise the canonical repro.sh repro-postgresql-dirty-folios SUT does not work. The PR adds an unrunnable entry to the scenario list.

Suggest dropping this script and adding a main.sh which uses the existing framework capabilities. Something like this:

# repros/repro-postgresql-dirty-folios/main.sh
: ${SCENARIO_ITERATIONS:=5}
: ${SCENARIO_RESTART_PG:=true}   # restart PG per phase to flush shared_buffers
# regression config (was SUT_ENV/LDG_ENV in the script)
: ${PG_HUGE_PAGES:=on}
: ${PGBENCH_SCALE:=8470}
# ...

function scenario:workloads() { echo postgresql; }

function scenario:restart_pg() {
    $SCENARIO_RESTART_PG && repro:cmd sudo systemctl restart postgresql
}

# provision once (install/configure), then N x (restart -> load -> restart -> run)
function scenario:run:sut() {
    {
        local i
        for ((i = 1; i <= SCENARIO_ITERATIONS; i++)); do
            echo "repro:info Iteration $i/$SCENARIO_ITERATIONS"
            echo "scenario:restart_pg; repro:run postgresql SUT load"
            echo "scenario:restart_pg; repro:run postgresql SUT run"
        done
    } | repro:persistent_steps "${SCENARIO_NAME}"
}

function scenario:run:loadgen() {
    local i
    for ((i = 1; i <= SCENARIO_ITERATIONS; i++)); do
        WORKLOAD_RESULTS_FILE="${SCENARIO_RESULTS_PATH}/results-${i}.json" \
            repro:run postgresql LDG load run results
    done
}

Note 1: the ssh fan-out and log collection here would be framework capabilities, not part of a workload's driving script. As it happens, they are part of an upcoming feature which will sit in util/controller.sh.

Note 2: If this scenario requires anything more than repro.sh repro-postgresql-dirty-folios <SUT|DRV>, suggest adding a README.md to the scenario as well, to document to users unfamiliar with the code.

Note 3: Remember to have the results step in the flow. The script runs load, run, load, run, ... then cleanup; postgresql:results:loadgen is never invoked, so WORKLOAD_RESULTS_FILE is
never written, which breaks the framework's convention.

# can restart PG between load and benchmark.
# TODO: Consider to make 'load' step default across benchmarks.
function postgresql:default_steps() {
local steps="install configure load run results"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the SUT because default_steps is currently mode-blind. results only exists as postgresql:results:loadgen so with this override, the SUT gets results in its op list and the validation loop in repro:run will hit repro:fatal "Repro 'postgresql' does not support operation 'results'".
The bug is covered by the script above because it always passes explicit ops.

The correct fix first needs a separate PR for improving repro:default_steps to filter a workload's default steps through actually defined functions, and also handle REPROCFG_CLEANUP. Then this function simply becomes echo "install configure load run results cleanup".


# Add a 'load' step (DB reload + pgbench -i) BEFORE 'run', so the external reproduce.sh controller
# can restart PG between load and benchmark.
# TODO: Consider to make 'load' step default across benchmarks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unlikely a cross-benchmark step, especially for non-database workloads. Changes to the framework need to benefit the present + intended corpus as a whole.

repro:cmd "pgbench -c ${PGBENCH_CLIENTS} -j ${PGBENCH_THREADS} -T ${PGBENCH_DURATION} -b ${PGBENCH_BUILTIN} -M ${PGBENCH_PROTOCOL} ${report_flag} --progress=10 ${PGBENCH_RUN_EXTRA_ARGS} \"${connstr}\" 2>&1 | tee /tmp/pgbench_run.log"

# signal SUT that we're done
repro:cmd "ulimit -n 65535; pgbench -c ${PGBENCH_CLIENTS} -j ${PGBENCH_THREADS} -T ${PGBENCH_DURATION} -b ${PGBENCH_BUILTIN} -M ${PGBENCH_PROTOCOL} ${report_flag} --progress=10 ${PGBENCH_RUN_EXTRA_ARGS} \"${connstr}\" 2>&1 | tee /tmp/pgbench_run.log"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're changing this line, let's use REPROCFG_TMP (for throwaway files) or SCENARIO_RESULTS_PATH (for files you want to keep after the test) instead of hardcoding the path. This keeps it forward-compatible with upcoming changes that automate the "keep after the test" part.


# initialize pgbench tables
# # fresh DB each iteration
repro:cmd "psql \"${admstr}\" -c 'DROP DATABASE IF EXISTS ${PG_DBNAME} WITH (FORCE)'"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: FORCE requires PG 13+, but PG_VERSION is user-settable and could cause failure at this step if old enough.

repro:package:install "postgresql${PG_LDG_VERSION:+-${PG_LDG_VERSION}}"
else
repro:package:install postgresql${PG_LDG_VERSION} postgresql${PG_LDG_VERSION}-contrib
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing issue but good time to fix: installing this package starts a local PostgreSQL service on the load generator; which competes for resources with pgbench and makes the LDG more likely to become a bottleneck. Suggest stop+disable of the service after installing the package.

: ${PG_HUGE_PAGES:=try} # off, try, on
: ${SYSTEM_TRANSPARENT_HUGE_PAGES:=off} # off = don't change THP; always|never|inherit|madvise = set THP to that value
: ${PG_SYSTEM_NR_HUGEPAGES:=off} # off = don't touch; on = allocate 2MB hugepages based on shared_buffers
: ${PG_HUGEPAGE_PAD_PERCENT:=15} # extra % of hugepages above shared_buffers calculation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an override in the repro scenario, not in the default benchmark - unless you intend this to be the default value for all future repros using postgresql.


# --- PostgreSQL ---
: ${PG_VERSION:=17}
: ${PG_LDG_VERSION:=}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest : ${PG_LDG_VERSION=${PG_VERSION}} here (no colon assignment). This will match the server version when unset, and fall back to distro default when empty. Otherwise it will produce bare postgresql /
postgresql-contrib, which don't exist on AL2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants