diff --git a/CHANGELOG.md b/CHANGELOG.md index 198c08ba..e135dc82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,39 @@ true until the next version shipped. ### Fixed +- `native_reclaim_cycles` could not reach the defect it guards (#1138). + + It is the declared regression guard for #84. Deleting the #84 fix left it reporting + `12 passed + 0 failed`, arm for arm, including the arm named after the defect. + +`pgcolumnar.reclaim_coalesce` defaults on, and it does two things: it merges + adjacent freed ranges, and it carries its own `CommandCounterIncrement` on the free + path. That second one does the visibility work the #84 fix would otherwise do, so + with coalescing on the defect is masked. + + Isolated by freeing alternate whole groups, which fragments by non-adjacency and + leaves the option at its default. On the same mutated build: + + | fixture | coalesce | free list | rewrote | result | + | --- | --- | ---: | ---: | --- | + | alternate groups | on | 15 | 15 | clean | + | contiguous block | off | 18 | 12 | `tuple already updated by self` | + + Fifteen fragments with coalescing on does not reach it. The option is necessary and + sufficient, and the free-list count is a property of the fixture. The suite runs with coalescing + off and **asserts that, read back from the server**. The value is set in the cluster + config. A conf line that stops taking effect returns the suite to the state this + issue is about. With the #84 fix removed it reddens five + arms with `ERROR: tuple already updated by self`. + + The pytest twin says the same thing under the same name. It asserts the option read + back from the server, rather than assumed from the `SET` that asked for it. The parity + grader is what caught the divergence. It graded the pair `1` while every other pair + was `0`, on a difference of meaning rather than of counts. + + Found by @OffgridwithJD, whose pytest twin already had the fixture and who then + separated the two factors. + - A pytest run killed with `SIGTERM` leaked its throwaway cluster (#1170). Python does not run `finally` blocks when the default `SIGTERM` disposition diff --git a/test/native_reclaim_cycles.sh b/test/native_reclaim_cycles.sh index 05e9c09e..affb7865 100755 --- a/test/native_reclaim_cycles.sh +++ b/test/native_reclaim_cycles.sh @@ -21,15 +21,51 @@ # Written fresh for pgColumnar. set -uo pipefail + +# COALESCING OFF, OR THIS SUITE CANNOT REACH THE DEFECT IT GUARDS (#1138). +# Delete the #84 fix, rebuild, and the old fixture reported 12 passed + 0 failed, +# arm for arm -- including `compact_rewrite cycle N returns a count (no +# self-conflict)`, the arm named after the defect. Found by @OffgridwithJD. +# +# THE OPERATIVE PROPERTY IS THE GUC, NOT A FRAGMENTED FREE LIST, and the first +# version of this comment said the opposite. `reclaim_coalesce` does two things: +# it merges adjacent freed ranges, AND it carries its own CommandCounterIncrement +# on the free path (columnar_metadata.c:792, `if (pgcolumnar_reclaim_coalesce)`). +# That second one does the visibility work the #84 fix would otherwise do, so +# with coalescing on the defect is masked however fragmented the list is. +# +# Isolated by freeing ALTERNATE whole groups, which fragments by non-adjacency +# and leaves the GUC at its default. On the same mutated .so: +# +# alternate groups, coalesce=on free list 15, rewrote 15 CLEAN +# contiguous block, coalesce=off free list 18, rewrote 12 tuple already +# updated by self +# +# Fifteen fragments with coalescing on does not reach it. So coalesce=off is +# necessary and sufficient, and the free-list count is a property of the fixture +# rather than the thing that arms the suite. @OffgridwithJD separated the two +# factors; the first version of this comment would have told the next maintainer +# to preserve the wrong one. +# +# IN THE CLUSTER CONFIG, NOT A `SET`. Every psql_run here is its own session, so a +# SET would last exactly one statement and the writing session would not have it. +PGC_EXTRA_CONF="${PGC_EXTRA_CONF:-} +pgcolumnar.reclaim_coalesce=off" +export PGC_EXTRA_CONF . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" -# 8000 rows in 8 groups of 1000, so each compaction rewrites several groups and -# each command performs several free-space allocations. -GEN="SELECT g AS id, (g % 100) AS v, md5(g::text) AS payload FROM generate_series(1, 8000) g" +# 30,000 rows in groups of 1,000, then a CONTIGUOUS block of whole groups freed +# at once. That is what puts many separate reusable ranges on the free list; a +# rotating slice frees a little from every group and coalesces back to one range. +ROWS=30000 +GROUP=1000 +DEL_LO=6001 +DEL_HI=24000 +GEN="SELECT g AS id, (g % 100) AS v, md5(g::text) AS payload FROM generate_series(1, $ROWS) g" psql_run "CREATE TABLE h (id int, v int, payload text);" psql_run "CREATE TABLE n (id int, v int, payload text) USING pgcolumnar;" -psql_run "SELECT pgcolumnar.set_options('n', stripe_row_limit => 1000, chunk_group_row_limit => 1000);" +psql_run "SELECT pgcolumnar.set_options('n', stripe_row_limit => $GROUP, chunk_group_row_limit => $GROUP);" psql_run "INSERT INTO h $GEN;" psql_run "INSERT INTO n $GEN;" @@ -37,6 +73,37 @@ fsize() { q "SELECT pg_relation_size('n');"; } hash_n() { pgc_set_hash 'SELECT id, v, payload FROM n'; } hash_h() { pgc_set_hash 'SELECT id, v, payload FROM h'; } +free_rows() { q "SELECT count(*) FROM pgcolumnar.free_space + WHERE storage_id = pgcolumnar.get_storage_id('n');"; } + +# SEVERAL GROUPS, OR ONE COMMAND CANNOT ALLOCATE TWICE. Read back from the +# catalog rather than assumed from the option that asked for it. +_groups="$(q "SELECT count(*) FROM pgcolumnar.storage s + JOIN pgcolumnar.row_group rg USING (storage_id) + WHERE s.relation_oid = 'n'::regclass;")" +check "premise: the table has several row groups to rewrite" \ + "$([ "${_groups:-0}" -ge 2 ] && echo "many ($_groups)" || echo "TOO FEW ($_groups)")" \ + "many ($_groups)" + +# Free a large CONTIGUOUS block of whole groups and compact, which is what puts +# many separate reusable ranges on the free list. +psql_run "DELETE FROM h WHERE id BETWEEN $DEL_LO AND $DEL_HI;" +psql_run "DELETE FROM n WHERE id BETWEEN $DEL_LO AND $DEL_HI;" +psql_run "SELECT pgcolumnar.compact('n');" +_free="$(free_rows)" + +# THE PRECONDITION FOR #84, ASSERTED RATHER THAN HOPED FOR, AND IT IS THE GUC. +# Read back from the SERVER, because the value is set in the cluster config: a +# conf line that stops taking effect leaves every arm below passing on a build +# with the fix removed, which is the state this suite was in before #1138. +check "premise: coalescing is off, which is what lets this suite reach #84" \ + "$(q "SHOW pgcolumnar.reclaim_coalesce;")" "off" + +# The free list's shape, PRINTED rather than asserted. It is a property of the +# fixture and not what arms the suite: 15 fragments with coalescing ON do not +# reach the defect. +echo " (free_space rows after the block delete: $_free)" + check "initial parity" "$(hash_n)" "$(hash_h)" # Repeated {delete a rotating slice, compact_rewrite}. No inserts, so the only diff --git a/test/pytest/test_native_reclaim_cycles.py b/test/pytest/test_native_reclaim_cycles.py index 73f6e4e5..265c2994 100644 --- a/test/pytest/test_native_reclaim_cycles.py +++ b/test/pytest/test_native_reclaim_cycles.py @@ -16,27 +16,31 @@ That keeps the two harnesses independent by construction, and a failure prints the rows that differ instead of two unequal hashes. -THE FIXTURE IS FRAGMENTED ON PURPOSE, AND THAT IS THE WHOLE DIFFERENCE. The shell -suite's fixture cannot reach the defect it is the regression guard for. Measured: -delete the #84 fix, and the shell suite reports 12 passed / 0 failed, unchanged. - -The reason is `pgcolumnar.reclaim_coalesce`, which defaults ON. Compaction then merges -adjacent freed ranges, so the free list holds one or two rows however much is freed, -one command allocates from it at most once, and the just-consumed row is never -re-selected. Measured on the shell suite's own fixture, per cycle: - - free_space rows before compact_rewrite: 0, 1, 2, 2, 2 - -With coalescing OFF the same workload keeps the ranges separate and the precondition -holds. Two cells, each built from its own source and printing its own `.so` hash: - - fix present (.so e95880e45673) 3 cycles, no error, free list steady at 18 - fix removed (.so 42bb17933a55) first compact_rewrite raises - "tuple already updated by self" - -So this file runs its cycles with coalescing off, and asserts the free list is -actually fragmented before relying on it. That premise is what keeps the suite from -going quietly vacuous again if the allocator's shape changes. +COALESCING OFF IS THE WHOLE DIFFERENCE, AND IT IS NOT THE FRAGMENTATION. Delete the +#84 fix and the old shell fixture reported 12 passed / 0 failed, unchanged. An earlier +version of this docstring said a one-row free list was the reason, and that account is +wrong. + +`pgcolumnar.reclaim_coalesce` does TWO things. It merges adjacent freed ranges, and it +carries its own `CommandCounterIncrement` on the free path -- `columnar_metadata.c:792`, +guarded by `if (pgcolumnar_reclaim_coalesce)`. That second one does the visibility work +the #84 fix would otherwise do, so with coalescing on the defect is masked however +fragmented the list is. + +Isolated by freeing ALTERNATE whole groups, which fragments by non-adjacency and leaves +the option at its default. On the same mutated build: + + alternate groups, coalesce=on free list 15, rewrote 15 CLEAN + contiguous block, coalesce=off free list 18, rewrote 12 tuple already + updated by self + +Fifteen fragments with coalescing on does not reach it. So the option is necessary and +sufficient, and the free-list count is a property of the fixture rather than the thing +that arms the suite. + +So this file runs its cycles with coalescing off and asserts THAT, read back from the +server rather than assumed from the `SET` that asked for it. The free-list count is +printed. The shell twin asserts the same property under the same name. """ import psycopg @@ -106,10 +110,14 @@ def test_native_reclaim_cycles(pgc_conn, expect): # THE PRECONDITION FOR #84, ASSERTED RATHER THAN HOPED FOR. With a free list of # one row -- which is what coalescing produces -- no command allocates from it # twice and every arm below passes on a build with the fix removed. - expect.at_least( - free, 5, - "premise: the free list is fragmented, so one command allocates from it " - "more than once", + # THE PRECONDITION FOR #84, AND IT IS THE OPTION. Read back from the server, + # because a `SET` that silently stopped applying leaves every arm below passing + # on a build with the fix removed -- which is the state this suite was in before + # #1138. The free-list count above is printed rather than asserted: 15 fragments + # with coalescing ON do not reach the defect. + expect.text( + _one(pgc_conn, "SHOW pgcolumnar.reclaim_coalesce"), "off", + "premise: coalescing is off, which is what lets this suite reach #84", ) expect.rows(_rows(pgc_conn, "n"), _rows(pgc_conn, "h"), "initial parity")