test: native_reclaim_cycles could not reach the defect it guards (#1138)

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

test/native_reclaim_cycles.sh

@@ -21,22 +21,89 @@
 # 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;"
 
 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

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")