public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Little refactoring of portalcmds.c
6+ messages / 5 participants
[nested] [flat]
* [PATCH] Little refactoring of portalcmds.c
@ 2025-10-08 14:02 Aleksander Alekseev <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Aleksander Alekseev @ 2025-10-08 14:02 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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
Attachments:
[text/x-patch] v1-0001-Refactor-portalcmds.c.patch (2.5K, 2-v1-0001-Refactor-portalcmds.c.patch)
download | inline diff:
From af051406d09dedf87bde8802d7f7dea0ab4458ec Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 8 Oct 2025 16:54:17 +0300
Subject: [PATCH v1] Refactor portalcmds.c
Place some repetitive code in a helper function.
Author: Aleksander Alekseev <[email protected]>
Reviewed-by: TODO FIXME
Discussion: TODO FIXME
---
src/backend/commands/portalcmds.c | 40 +++++++++++++------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index ec96c2efcd3..d74a01c01a3 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -36,6 +36,19 @@
#include "utils/memutils.h"
#include "utils/snapmgr.h"
+/*
+ * Check that cursor name is not empty, which would conflict with protocol-level
+ * unnamed portal.
+ */
+static void
+check_cursor_name(const char *name)
+{
+ if (!name || name[0] == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_CURSOR_NAME),
+ errmsg("invalid cursor name: must not be empty")));
+}
+
/*
* PerformCursorOpen
@@ -53,14 +66,7 @@ PerformCursorOpen(ParseState *pstate, DeclareCursorStmt *cstmt, ParamListInfo pa
MemoryContext oldContext;
char *queryString;
- /*
- * Disallow empty-string cursor name (conflicts with protocol-level
- * unnamed portal).
- */
- if (!cstmt->portalname || cstmt->portalname[0] == '\0')
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_CURSOR_NAME),
- errmsg("invalid cursor name: must not be empty")));
+ check_cursor_name(cstmt->portalname);
/*
* If this is a non-holdable cursor, we require that this statement has
@@ -182,14 +188,7 @@ PerformPortalFetch(FetchStmt *stmt,
Portal portal;
uint64 nprocessed;
- /*
- * Disallow empty-string cursor name (conflicts with protocol-level
- * unnamed portal).
- */
- if (!stmt->portalname || stmt->portalname[0] == '\0')
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_CURSOR_NAME),
- errmsg("invalid cursor name: must not be empty")));
+ check_cursor_name(stmt->portalname);
/* get the portal from the portal name */
portal = GetPortalByName(stmt->portalname);
@@ -233,14 +232,7 @@ PerformPortalClose(const char *name)
return;
}
- /*
- * Disallow empty-string cursor name (conflicts with protocol-level
- * unnamed portal).
- */
- if (name[0] == '\0')
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_CURSOR_NAME),
- errmsg("invalid cursor name: must not be empty")));
+ check_cursor_name(name);
/*
* get the portal from the portal name
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Little refactoring of portalcmds.c
@ 2025-10-27 06:02 Quan Zongliang <[email protected]>
parent: Aleksander Alekseev <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Quan Zongliang @ 2025-10-27 06:02 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Little refactoring of portalcmds.c
@ 2025-10-27 09:50 wenhui qiu <[email protected]>
parent: Quan Zongliang <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: wenhui qiu @ 2025-10-27 09:50 UTC (permalink / raw)
To: ; +Cc: Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
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 <[email protected]>
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
>
>
>
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Little refactoring of portalcmds.c
@ 2026-05-26 03:36 Zizhuan Liu <[email protected]>
parent: Aleksander Alekseev <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Zizhuan Liu @ 2026-05-26 03:36 UTC (permalink / raw)
To: [email protected]; +Cc: Aleksander Alekseev <[email protected]>
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Little refactoring of portalcmds.c
@ 2026-05-26 03:42 Zizhuan Liu <[email protected]>
parent: Zizhuan Liu <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Zizhuan Liu @ 2026-05-26 03:42 UTC (permalink / raw)
To: [email protected]; +Cc: Aleksander Alekseev <[email protected]>
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re:[PATCH] Little refactoring of portalcmds.c
@ 2026-05-26 07:05 =?utf-8?B?Wml6aHVhbiBMaXU=?= <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: =?utf-8?B?Wml6aHVhbiBMaXU=?= @ 2026-05-26 07:05 UTC (permalink / raw)
To: =?utf-8?B?YWxla3NhbmRlcg==?= <[email protected]>; =?utf-8?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>; +Cc: =?utf-8?B?cXVhbnpvbmdsaWFuZw==?= <[email protected]>; =?utf-8?B?cWl1d2VuaHVpZng=?= <[email protected]>
Hi all,
I have reviewed patch 6113 on CommitFest.
CommitFest page: https://commitfest.postgresql.org/patch/6113/
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.
(Sorry, I tried but still cannot post this to the original thread.)
Thanks,
--
X-MAN (Zizhuan Liu)
[email protected]
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-05-26 07:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-10-08 14:02 [PATCH] Little refactoring of portalcmds.c Aleksander Alekseev <[email protected]>
2025-10-27 06:02 ` Quan Zongliang <[email protected]>
2025-10-27 09:50 ` wenhui qiu <[email protected]>
2026-05-26 03:36 ` Zizhuan Liu <[email protected]>
2026-05-26 03:42 ` Zizhuan Liu <[email protected]>
2026-05-26 07:05 Re:[PATCH] Little refactoring of portalcmds.c =?utf-8?B?Wml6aHVhbiBMaXU=?= <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox