Re: [PATCH] Little refactoring of portalcmds.c

From: Zizhuan Liu <44973863(at)qq(dot)com>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Aleksander Alekseev <afiskon(at)gmail(dot)com>
Subject: Re: [PATCH] Little refactoring of portalcmds.c
Date: 2026-05-26 03:42:46
Message-ID: 177976696610.313758.9369660411072671375.pgcf@coridan.postgresql.org
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

The newly introduced check_cursor_name() implements the logic to reject empty cursor names, which matches the existing checks in PerformCursorOpen() and PerformPortalFetch().

However, this helper function is not a good fit for PerformPortalClose(). At the start of this function, there is a check for name == NULL — a NULL name corresponds to the CLOSE ALL command — and this check runs prior to validating whether the name is an empty string.

While calling check_cursor_name() inside PerformPortalClose() would keep the current behavior intact, it could confuse future readers. This approach also fails to achieve truly consistent cursor name validation across all three functions, and may add extra maintenance burdens in the long term.

On the other hand, if we only refactor PerformCursorOpen() and PerformPortalFetch(), the change would end up being rather minimal.

Considering all these factors, I'd suggest leaving the code as it is and skipping this refactoring.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2026-05-26 03:51:11 Fix bug of CHECK constraint enforceability recursion
Previous Message Zizhuan Liu 2026-05-26 03:36:06 Re: [PATCH] Little refactoring of portalcmds.c