Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Making your patch more committable

December 21, 2023

Making your patch more committable

What to do and not to do to make your Postgres patch more committable.


December 21, 2023

More Decks by Melanie

Other Decks in Programming


  1. Terminology ▪ patch ó commit ▪ patch set ó commits

    (branch) ▪ mailing list (pgsql- hackers)
  2. Structure of an initial email ▪ High level summary ▪

    Why do we want this? ▪ Why this way? ▪ Call to action
  3. Why do we want this? ▪ Problem fixed ▪ Benefit

    added ▪ Users impacted ▪ Why now https://www.postgresql.org/message-id/flat/cf85f46f-b02f-05b2-5248-5000b894ebab%40enterprisedb.com
  4. Why this way? ▪ How is this different from previous

    attempts? ▪ Explain the design ▪ Why not any obvious alternative designs? ▪ Why isn’t a part implemented? https://www.postgresql.org/message-id/flat/cf85f46f-b02f-05b2-5248-5000b894ebab%40enterprisedb.com
  5. Call to action ▪ TODOs awaiting validation – No documentation

    yet – Haven’t implemented it for this scenario yet ▪ Open questions – Is this part of the design good? ▪ What else needs to be done? – Do I need to implement it for partition tables? ▪ Issues – Why is this test failing? https://www.postgresql.org/message-id/flat/cf85f46f-b02f-05b2-5248-5000b894ebab%40enterprisedb.com https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com
  6. Other tips for initial email ▪ Don't make everything equally

    important in commit message and email ▪ Try to loop in invested users if user facing or devs if API – CC people who have worked on this problem in the past ▪ Chime in on/review threads in the same subject area
  7. How to respond ▪ Answer questions that reviewers ask you

    ▪ Address review inline point-by-point when sending a new version ▪ Be flexible with stylistic changes ▪ Address all instances of a given type of feedback ▪ Don’t be afraid to fork new mailing list discussions
  8. Opening Arguments With my patch applied, vacuum is 10% faster

    on average. A trivial example: CREATE TABLE foo(a INT) WITH (autovacuum_enabled=false); INSERT INTO foo SELECT i FROM generate_series(1, 1000) AS i; \timing on VACUUM (VERBOSE) foo; [Paste results here]
  9. Less is More in Benchmarks ▪ \timing on is okay!

    ▪ Use pgbench – Other supporting benchmarks are okay ▪ Avoid bash scripts and attachments ▪ Minimize distractions ▪ Eliminate extraneous conditions – Use development GUCs, table options when helpful ▪ Minimize runtime
  10. Benchmark the right thing ▪ Use pgbench “custom scripts” pgbench

    –c 1 –M prepared –P 1 –t 50 \ –f <(echo "COPY table(…) FROM 'copysource'") ▪ Ensure reproducibility
  11. Explain How to Run Your Benchmark ▪ Postgres prep steps:

    prewarm, restart, initdb ▪ Postgres settings (GUCs, table options, etc) ▪ Schema and data ▪ Machine specs or settings (e.g. readahead)
  12. - DDL and data generation - steps required - command

    to run https://www.postgresql.org/message-id/flat/CA%2BTgmoa8nddwpMA4Emn7sVoNrQ883mn8ZJBiqXa8dm3puKn%3DqQ%40mail.gmail.com#4251b35792be16116af1c6cf87c2eb5d
  13. Provide the realistic use case ▪ If example is contrived,

    explain realistic scenario – e.g. not tpcb-like pgbench with random data access distribution ▪ Describe expected impact and how much of user base it would affect ▪ Outline worst and best cases ▪ Run additional benchmarks with longer runtimes that prove lack of negative interaction with subsystems like checkpointer
  14. Presenting Performance Test Results ▪ Don't provide more benchmark results

    than is useful ▪ TPS (Transactions per second) isn’t the only metric – Plan diffs – perf output summary – P99 latency – Memory utilization – Per process IO consumption – …
  15. Regression Tests ▪ Minimal ▪ Fast ▪ Reliable ▪ Core

    suite (SQL) ▪ “Isolation, recovery, subscription” suite (Perl) ▪ …
  16. ▪ When adding a test is difficult ▪ Instills confidence

    in correctness of work ▪ Protects future developers Debug-only Validation
  17. in July… ▪ reduces vacuum WAL volume by > 50%!

    ▪ passes tests! ▪ and the code looks good too!
  18. in July… ▪ 21 files changed, 140 insertions, 1348 deletions

    L ▪ unreviewable L ▪ no commit message L
  19. the journey to committability 1. Every commit works 2. No

    omnibus commits 3. Each line of a commit is purposeful and clearly outlined in commit message 4. Refactoring should be motivated 5. Functional changes should be obvious 6. Comments and commit messages must clearly communicate risk and action required
  20. - turn the refactor into a feature - independently valuable

    commit justifying churn add error detail for messages originating in on-access pruning