Analysis: Refactor and Unify Expression Construction Functions in makefuncs.c

Core Problem

The patch author identified what they perceived as redundant code in makefuncs.c — specifically, several expression-construction helper functions that are thin wrappers around more general underlying functions. The observation was triggered by reviewing commit cf6723 ("Make transformAExprIn() return a flattened bool expression directly"), which touched boolean expression construction paths.

The specific "duplication" identified:

1. make_andclause() / make_orclause() / make_notclause() — These construct BoolExpr nodes for AND/OR/NOT. The author proposed replacing them with direct calls to makeBoolExpr(), noting similarity with makeAndExpr() / makeOrExpr() / makeNotExpr() in gram.y.

2. makeSimpleA_Expr() — A convenience wrapper around makeA_Expr() that constructs an A_Expr with a single-element operator name list. The author proposed eliminating it in favor of calling makeA_Expr() directly.

3. makeDefElemExtended() — A wrapper around makeDefElem() that handles namespace-qualified definition elements. The author proposed unifying these.

Why This Was Rejected

Tom Lane's terse rejection ("TBH, I don't think this is worth the trouble") reflects several well-understood principles in PostgreSQL development:

1. Semantic Clarity Over Code Minimization

Functions like make_andclause(), make_orclause(), and make_notclause() exist not because they save significant code at their definition site, but because they provide semantic clarity at call sites. When a developer reads make_andclause(args), they immediately understand intent. Replacing this with makeBoolExpr(AND_EXPR, args, -1) forces readers to parse the enum constant and understand the general-purpose interface — a net loss in readability across the hundreds of call sites.

2. Abstraction Layer Value

These "trivial wrappers" serve as abstraction barriers. If the internal representation of boolean expressions ever changes (e.g., adding a field to BoolExpr), having centralized wrappers means only one place needs updating rather than every call site. The wrappers are intentional API design, not accidental duplication.

3. Churn-to-Value Ratio

Removing well-established utility functions touches many files across the codebase (optimizer, parser, rewriter, planner all use these). The resulting diff would be large, creating merge conflicts with in-flight patches, polluting git blame, and making backporting harder — all for zero functional improvement.

4. makeSimpleA_Expr() Specifically

This function exists because the vast majority of expression constructions in the parser use a simple single-element operator name. Having makeSimpleA_Expr() avoids requiring every call site to manually construct a list_make1(makeString(...)) — which would be more error-prone and verbose.

5. Test Cases Are Irrelevant

The proposed "test cases" (e.g., SELECT * FROM t1 WHERE id = 1 AND id = 3) test basic SQL functionality that is already exhaustively covered by the regression test suite. They do not specifically exercise the refactored code paths in any meaningful way that existing tests don't already cover.

Architectural Context

In PostgreSQL's codebase, makefuncs.c and makefuncs.h intentionally provide a layered API for node construction:

• Low-level: makeNode() macro + manual field assignment
• Mid-level: makeBoolExpr(), makeA_Expr(), makeDefElem() — general-purpose constructors
• High-level: make_andclause(), makeSimpleA_Expr(), makeDefElemExtended() — convenience wrappers for common patterns

This layering is a deliberate design choice that balances code reuse with call-site ergonomics. Collapsing the layers conflates "less code in makefuncs.c" with "better code" — a false equivalence.

Outcome

The patch was immediately rejected by Tom Lane (a senior committer with decades of experience maintaining this exact code) and the author accepted the rejection gracefully. This is a textbook example of a well-intentioned cleanup that misidentifies intentional API design as accidental duplication.