Thread

1. [PATCH] Little refactoring of portalcmds.c

   Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-08T14:02:00Z

   Hi,
   
   The proposed patch places some repetitive code in a helper function.
   The value of this change is arguably not that high but it makes the
   code a bit neater IMO.
   
   -- 
   Best regards,
   Aleksander Alekseev
   

2. Re: [PATCH] Little refactoring of portalcmds.c

   Quan Zongliang <quanzongliang@yeah.net> — 2025-10-27T06:02:41Z

   
   On 10/8/25 10:02 PM, Aleksander Alekseev wrote:
   > Hi,
   > 
   > The proposed patch places some repetitive code in a helper function.
   > The value of this change is arguably not that high but it makes the
   > code a bit neater IMO.
   > 
   
   It also reduces the ease of reading the code.
   Just add a function for a single if statement. I don't think it's necessary.
   
   Regards,
   Quan Zongliang
   
   
   
   
   

3. Re: [PATCH] Little refactoring of portalcmds.c

   wenhui qiu <qiuwenhuifx@gmail.com> — 2025-10-27T09:50:21Z

   Hi Aleksander Alekseev
   > The proposed patch places some repetitive code in a helper function.
   > The value of this change is arguably not that high but it makes the
   > code a bit neater IMO.
   I  agree with your suggestion to refactor the duplicated code into a
   function.
   
   Thanks
   
   On Mon, Oct 27, 2025 at 2:03 PM Quan Zongliang <quanzongliang@yeah.net>
   wrote:
   
   >
   >
   > On 10/8/25 10:02 PM, Aleksander Alekseev wrote:
   > > Hi,
   > >
   > > The proposed patch places some repetitive code in a helper function.
   > > The value of this change is arguably not that high but it makes the
   > > code a bit neater IMO.
   > >
   >
   > It also reduces the ease of reading the code.
   > Just add a function for a single if statement. I don't think it's
   > necessary.
   >
   > Regards,
   > Quan Zongliang
   >
   >
   >
   >
   

4. Re: [PATCH] Little refactoring of portalcmds.c

   ZizhuanLiu X-MAN <44973863@qq.com> — 2026-05-26T03:36:06Z

   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(PerformPortalClose), 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.

5. Re: [PATCH] Little refactoring of portalcmds.c

   ZizhuanLiu X-MAN <44973863@qq.com> — 2026-05-26T03:42:46Z

   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.