Technical Analysis: [PATCH] Little refactoring of portalcmds.c

Core Problem

The patch addresses code duplication in src/backend/commands/portalcmds.c, specifically around cursor name validation logic. In PostgreSQL, cursors are implemented via portals, and the three main entry points for cursor operations — PerformCursorOpen(), PerformPortalFetch(), and PerformPortalClose() — each contain similar checks to reject empty cursor names. The proposal was to extract this repeated validation into a helper function called check_cursor_name().

Architectural Context

portalcmds.c is the command-level interface for portal/cursor operations in PostgreSQL. It sits in the executor layer and handles:

• DECLARE CURSOR → PerformCursorOpen()
• FETCH/MOVE → PerformPortalFetch()
• CLOSE → PerformPortalClose()

Each of these functions validates its cursor name argument before proceeding. The validation is a simple check — rejecting empty strings — but it's repeated across all three functions.

Proposed Solution

The patch introduced a check_cursor_name() helper function that encapsulates the empty-string rejection logic, replacing the inline if statements in each of the three functions.

Key Technical Disagreement: Semantic Asymmetry in PerformPortalClose()

The most substantive technical point raised in review concerns a semantic mismatch in PerformPortalClose(). Unlike the other two functions, PerformPortalClose() has a special case where name == NULL is valid — it corresponds to the SQL CLOSE ALL command. The control flow in this function is:

1. Check if name == NULL → if so, close all portals (valid path)
2. Check if name is empty string → error
3. Proceed with closing the named portal

Inserting check_cursor_name() after step 1 would technically preserve behavior (since the NULL case returns early), but it creates a conceptual oddity: the helper function implies uniform validation semantics across all three call sites, when in reality PerformPortalClose() has fundamentally different NULL-handling semantics than the other two.

This is a valid concern about abstraction leakage — the helper function would paper over a semantic difference rather than truly unifying the logic.

Assessment of Refactoring Value

The change is minimal in scope:

• Only 2 of the 3 call sites (PerformCursorOpen and PerformPortalFetch) are clean fits for the helper
• The extracted logic is a single if statement
• The reduction in line count and complexity is negligible

The consensus ultimately leaned toward "not worth it" because:

1. The abstraction doesn't cleanly cover all cases
2. Extracting a single conditional into a function can reduce locality of reading
3. The maintenance burden of the helper (needing to understand its contract at each call site) may exceed the cost of the duplication

Resolution

The patch was withdrawn by the author after receiving a thorough review that articulated why the refactoring, while well-intentioned, introduced more conceptual complexity than it removed. This is a textbook example of where DRY (Don't Repeat Yourself) conflicts with code clarity — sometimes a small amount of duplication is preferable to a leaky abstraction.