Subject: [postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Mon, 20 Apr 2026 04:09:16 +0000

Adds test/src/bracket-parse-test.c (20 test cases covering all branches of the OPENING_BRACKET switch in dconn_get_attributes) and a Chinese analysis document at test/docs/bracket-parse-analysis.md describing the parser's control flow, risk points, and how to run the tests (including ASan/UBSan setups for stability hardening).

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 23 Apr 2026 10:55:57 +0000

1. Incomplete PR — missing integration into the test suite                                         
    - No test/tests modification (the test isn't added to TESTBINS)                                  
    - No test/expected/bracket-parse.out file                                                        
    - Without these, make installcheck won't run this test. The analysis doc describes how to add    
  them but the PR doesn't actually do it. This should be included in the PR itself.                  
                                                                                                     
  2. Documentation is entirely in Chinese                                                            
    - The psqlodbc project is English-language. Both markdown files should be in English, or at      
  minimum have English translations. This is a barrier for other contributors and maintainers.       
                                                                                                     
  3. `test/docs/` is a new directory with no precedent                                               
    - The project has no existing test/docs/ directory. Analysis documents and coverage reports don't
  belong in the source tree — they're ephemeral artifacts. The coverage report in particular is      
  specific to one run on one machine and will go stale immediately.                                  
    - Recommendation: remove both markdown files from the PR. The analysis is valuable but belongs in
  the PR description or a wiki, not committed to the repo. The coverage report is a point-in-time    
  artifact.                                                                                          
                                                                                                     
  4. 3 branches still uncovered (B2a, B3a, `!equals`)                                                
    - The author identifies these gaps but doesn't close them. B2a is the most interesting — it's the
  error path when delp has been cleared to NULL by a prior }} recovery and then no closing bracket is
  found. This is exactly the kind of edge case that could hide a real bug.                           
    - The suggested TC21/TC22 in the coverage report should be implemented before merge.             
                                                                                                     
  5. Test doesn't validate parsed values                                                             
    - Every test case only checks "didn't crash." None verify that the parsed attribute/value pairs  
  are correct. For example, TC02 PWD={pass;word;123} should verify the password was parsed as        
  {pass;word;123} (with braces, since the function preserves them). A crash-only test is useful for  
  fuzzing but weak as a regression test.                                                             
                                                                                                     
  6. TC07 behavior is suspicious                                                                     
    - The coverage report shows TC07 PWD={no_close_at_all hits "B2-recover × N" and returns with     
  authentication failure (not a parse error). This means the parser didn't detect the missing closing
  bracket — it fell through to the connection attempt. That's arguably a bug in the parser itself,   
  and the test should flag it rather than marking it PASS.                                           
                                                                                                     
  7. Minor code issues in the test                                                                   
    - try_connect doesn't check return values of SQLAllocHandle/SQLSetEnvAttr — if these fail, the   
  subsequent SQLDriverConnect will crash on a null handle                                            
    - TC13 and TC17 are nearly identical (PWD={value}; vs PWD={val};) — they test the same branch    
  (B4-eof)                                                                                           
                                                                                                     
  Verdict                                                                                            
                                                                                                     
  The test cases themselves are solid work and the branch analysis is excellent. But the PR isn't    
  merge-ready:                                                                                       
                                                                                                     
  - Add the test to test/tests and provide test/expected/bracket-parse.out                           
  - Remove the Chinese markdown docs from the tree (put the analysis in the PR description)          
  - Add the 3 missing test cases for B2a/B3a/!equals                                                 
  - Add at least basic value validation for the happy-path cases                                     
  - Investigate TC07's behavior — a missing closing bracket silently succeeding is worth a follow-up 
  issue