feature/padding-waste - #2
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new analysis helper to quantify avoidable alignment padding so users can quickly assess whether a table rewrite is worth it, addressing Issue #1.
Changes:
- Introduces
column_tetris.padding_wasted(table, mode)returning avoidable padding bytes per row (default) or total ('total'). - Adds SQL-based regression tests for per-row, total, and invalid-mode behavior.
- Updates README with usage examples and a query to find wasteful tables.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
pg_column_tetris--0.1.0.sql |
Adds the padding_wasted() function implementation. |
test/sql/06_padding_wasted.sql |
Adds coverage for per-row/total waste and error handling. |
README.md |
Documents the new function and provides examples/queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RETURN (v_current_waste - v_optimal_waste)::bigint * v_row_count; | ||
| END IF; | ||
|
|
||
| RETURN (v_current_waste - v_optimal_waste)::bigint; |
There was a problem hiding this comment.
padding_wasted can return a negative number when v_current_waste <= v_optimal_waste (the same situation validate() treats as non-suboptimal). Since this function is documented as returning wasted/avoidable bytes, consider clamping the result to 0 when current is not worse than optimal (and do the same for total mode).
| RETURN (v_current_waste - v_optimal_waste)::bigint * v_row_count; | |
| END IF; | |
| RETURN (v_current_waste - v_optimal_waste)::bigint; | |
| RETURN GREATEST((v_current_waste - v_optimal_waste)::bigint, 0::bigint) * v_row_count; | |
| END IF; | |
| RETURN GREATEST((v_current_waste - v_optimal_waste)::bigint, 0::bigint); |
| END IF; | ||
| END LOOP; | ||
|
|
||
| IF report_mode = 'total' THEN |
There was a problem hiding this comment.
report_mode = 'total' computes row count via count(*), which will force a full scan (or at least a potentially expensive count) on large tables. Consider either (a) documenting that total may be slow, or (b) adding an estimated mode based on pg_class.reltuples/pg_stat_all_tables.n_live_tup to avoid heavy scans when users just want an order-of-magnitude estimate.
| IF report_mode = 'total' THEN | |
| IF report_mode = 'estimated' THEN | |
| SELECT GREATEST(COALESCE(c.reltuples, 0), 0)::bigint | |
| INTO v_row_count | |
| FROM pg_class c | |
| WHERE c.oid = v_rel_oid; | |
| RETURN (v_current_waste - v_optimal_waste)::bigint * COALESCE(v_row_count, 0); | |
| ELSIF report_mode = 'total' THEN | |
| -- Exact row count; may be expensive on large tables. |
| CREATE FUNCTION column_tetris.padding_wasted(relation_name text, report_mode text DEFAULT 'row') | ||
| RETURNS bigint | ||
| LANGUAGE plpgsql AS $$ | ||
| DECLARE | ||
| v_rel_oid oid; | ||
| v_current_waste int := 0; | ||
| v_optimal_waste int := 0; | ||
| v_offset int; | ||
| v_align int; | ||
| v_padding int; | ||
| v_num_cols int; | ||
| v_any_nullable bool; | ||
| v_row_count bigint; | ||
| col record; | ||
| BEGIN | ||
| IF report_mode NOT IN ('row', 'total') THEN | ||
| RAISE EXCEPTION 'invalid report_mode: %. Must be row or total', report_mode; | ||
| END IF; | ||
|
|
||
| v_rel_oid := relation_name::regclass::oid; | ||
|
|
||
| SELECT count(*)::int, bool_or(NOT a.attnotnull) | ||
| INTO v_num_cols, v_any_nullable | ||
| FROM pg_attribute a | ||
| WHERE a.attrelid = v_rel_oid | ||
| AND a.attnum > 0 | ||
| AND NOT a.attisdropped; | ||
|
|
There was a problem hiding this comment.
This function queries pg_attribute/pg_type without fixing search_path. Other catalog-reading functions in this extension set search_path to pg_catalog, pg_temp; doing the same here would prevent user objects shadowing catalog names and keep behavior consistent.
| -- --------------------------------------------------------------------------- | ||
| -- padding_wasted(text) — Returns bytes of avoidable padding per row | ||
| -- --------------------------------------------------------------------------- | ||
|
|
||
| CREATE FUNCTION column_tetris.padding_wasted(relation_name text, report_mode text DEFAULT 'row') | ||
| RETURNS bigint |
There was a problem hiding this comment.
This adds a new public function to the extension but the extension version remains 0.1.0 (and there are no upgrade scripts). If users already have 0.1.0 installed, they won't get this function via ALTER EXTENSION ... UPDATE. Consider bumping the extension version and adding an upgrade script that creates padding_wasted.
| Use `padding_wasted()` to quickly check how many bytes a table wastes per row: | ||
|
|
||
| ```sql | ||
| SELECT column_tetris.padding_wasted('orders'); | ||
| -- Returns: 7 (bytes of avoidable padding per row) | ||
| ``` | ||
|
|
||
| Pass `'total'` to see the total waste across all rows in the table. Wrap with `pg_size_pretty()` for human-readable output: | ||
|
|
||
| ```sql | ||
| SELECT pg_size_pretty(column_tetris.padding_wasted('orders', 'total')); | ||
| -- Returns: '458 MB' | ||
| ``` |
There was a problem hiding this comment.
The README describes padding_wasted() as "bytes a table wastes per row" and shows an example return value, but the implementation is based on the extension’s fixed-width alignment heuristic (current vs suggested order). Consider clarifying that it reports avoidable fixed-width alignment padding (not total row overhead), and note that 'total' will compute row count and may be slow on large tables.
| DECLARE | ||
| v_waste int; | ||
| BEGIN | ||
| SELECT column_tetris.padding_wasted('test_pw_suboptimal') INTO v_waste; | ||
| IF v_waste <= 0 THEN | ||
| RAISE EXCEPTION 'TEST FAILED: padding_wasted should be > 0 for suboptimal table, got %', v_waste; | ||
| END IF; | ||
| RAISE NOTICE 'TEST PASSED: suboptimal table has % bytes waste per row', v_waste; | ||
| END; | ||
| $$; | ||
|
|
||
| -- Test 2: Optimal table should have 0 waste | ||
| CREATE TABLE test_pw_optimal ( | ||
| big_id bigint, | ||
| count integer, | ||
| flag boolean | ||
| ); | ||
|
|
||
| DO $$ | ||
| DECLARE | ||
| v_waste int; | ||
| BEGIN | ||
| SELECT column_tetris.padding_wasted('test_pw_optimal') INTO v_waste; | ||
| IF v_waste <> 0 THEN |
There was a problem hiding this comment.
The test stores padding_wasted() (which returns bigint) into int variables (v_waste). Using bigint here would match the function signature and avoid truncation if a future test uses larger totals.
| DECLARE | ||
| v_waste int; | ||
| BEGIN | ||
| SELECT column_tetris.padding_wasted('test_pw_zero_waste') INTO v_waste; | ||
| IF v_waste <> 0 THEN | ||
| RAISE EXCEPTION 'TEST FAILED: int4+int4+float8+text should have 0 waste, got %', v_waste; |
There was a problem hiding this comment.
Same as earlier blocks: padding_wasted() returns bigint, but this block declares v_waste int. Consider using bigint consistently in the tests to avoid truncation in future cases.
New padding_wasted(table, mode) function returns avoidable padding bytes per row (default) or total waste across all rows ('total' mode).
Closes #1