public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
4+ messages / 3 participants
[nested] [flat]
* [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
@ 2026-04-15 08:28 =?gb18030?B?uvq0q87E?= <[email protected]>
2026-04-16 02:05 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Chao Li <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: =?gb18030?B?uvq0q87E?= @ 2026-04-15 08:28 UTC (permalink / raw)
To: =?gb18030?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>
Hi,
Found a misleading comment in JsonTablePlanJoinNextRow() while reading
the JSON_TABLE execution code.
The function returns false when both siblings are exhausted (meaning no
more rows), but the comment says "there are more rows" — the exact
opposite of what's happening. The code itself is correct.
if (!JsonTablePlanNextRow(planstate->right))
{
/* Right sibling ran out of row, so there are more rows. */ /* wrong */
return false;
}
A reader might reasonably treat this as a bug and flip the return value,
which would cause JSON_TABLE UNION plans to loop indefinitely.
Patch attached.
Regards,
Chuanwen Hu
Attachments:
[application/octet-stream] fix-jsontable-comment.patch (510B, 3-fix-jsontable-comment.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 770840a..0ec9b4d 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -4729,7 +4729,7 @@ JsonTablePlanJoinNextRow(JsonTablePlanState *planstate)
*/
if (!JsonTablePlanNextRow(planstate->right))
{
- /* Right sibling ran out of row, so there are more rows. */
+ /* Right sibling ran out of rows too, so there are no more rows. */
return false;
}
}
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
2026-04-15 08:28 [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() =?gb18030?B?uvq0q87E?= <[email protected]>
@ 2026-04-16 02:05 ` Chao Li <[email protected]>
2026-04-16 03:03 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Amit Langote <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Chao Li @ 2026-04-16 02:05 UTC (permalink / raw)
To: 胡传文 <[email protected]>; +Cc: pgsql-hackers <[email protected]>
> On Apr 15, 2026, at 16:28, 胡传文 <[email protected]> wrote:
>
> Hi,
> Found a misleading comment in JsonTablePlanJoinNextRow() while reading
> the JSON_TABLE execution code.
> The function returns false when both siblings are exhausted (meaning no
> more rows), but the comment says "there are more rows" — the exact
> opposite of what's happening. The code itself is correct.
> if (!JsonTablePlanNextRow(planstate->right))
> {
> /* Right sibling ran out of row, so there are more rows. */ /* wrong */
> return false;
> }
> A reader might reasonably treat this as a bug and flip the return value,
> which would cause JSON_TABLE UNION plans to loop indefinitely.
> Patch attached.
> Regards,
> Chuanwen Hu<fix-jsontable-comment.patch>
The fix looks correct to me. I guess “no” was just unintentionally missed.
A small comment, I just feel “too” you newly added might not be needed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
2026-04-15 08:28 [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() =?gb18030?B?uvq0q87E?= <[email protected]>
2026-04-16 02:05 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Chao Li <[email protected]>
@ 2026-04-16 03:03 ` Amit Langote <[email protected]>
2026-04-16 05:17 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Amit Langote <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Amit Langote @ 2026-04-16 03:03 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: 胡传文 <[email protected]>; pgsql-hackers <[email protected]>
Hi,
On Thu, Apr 16, 2026 at 11:05 AM Chao Li <[email protected]> wrote:
> > On Apr 15, 2026, at 16:28, 胡传文 <[email protected]> wrote:
> >
> > Hi,
> > Found a misleading comment in JsonTablePlanJoinNextRow() while reading
> > the JSON_TABLE execution code.
> > The function returns false when both siblings are exhausted (meaning no
> > more rows), but the comment says "there are more rows" — the exact
> > opposite of what's happening. The code itself is correct.
> > if (!JsonTablePlanNextRow(planstate->right))
> > {
> > /* Right sibling ran out of row, so there are more rows. */ /* wrong */
> > return false;
> > }
> > A reader might reasonably treat this as a bug and flip the return value,
> > which would cause JSON_TABLE UNION plans to loop indefinitely.
> > Patch attached.
Thanks for the report and the patch.
> The fix looks correct to me. I guess “no” was just unintentionally missed.
>
> A small comment, I just feel “too” you newly added might not be needed.
Actually, I'd keep it, because it ties the comment to the left-sibling
check just above.
Will push the attached down to v17.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v1-0001-Fix-incorrect-comment-in-JsonTablePlanJoinNextRow.patch (1.4K, 2-v1-0001-Fix-incorrect-comment-in-JsonTablePlanJoinNextRow.patch)
download | inline diff:
From b0677179e116fd91bb5d83a24b517e85c5446a91 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 16 Apr 2026 11:52:16 +0900
Subject: [PATCH v1] Fix incorrect comment in JsonTablePlanJoinNextRow()
The comment on the return-false path when both UNION siblings are
exhausted said "there are more rows," which is the opposite of what
the code does. The code itself is correct, returning false to signal
no more rows, but the misleading comment could tempt a reader into
"fixing" the return value, which would cause UNION plans to loop
indefinitely.
Back-patch to 17, where JSON_TABLE was introduced.
Author: Chuanwen Hu <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 17
---
src/backend/utils/adt/jsonpath_exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 770840a0611..0ec9b4df2ef 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -4729,7 +4729,7 @@ JsonTablePlanJoinNextRow(JsonTablePlanState *planstate)
*/
if (!JsonTablePlanNextRow(planstate->right))
{
- /* Right sibling ran out of row, so there are more rows. */
+ /* Right sibling ran out of rows too, so there are no more rows. */
return false;
}
}
--
2.47.3
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
2026-04-15 08:28 [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() =?gb18030?B?uvq0q87E?= <[email protected]>
2026-04-16 02:05 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Chao Li <[email protected]>
2026-04-16 03:03 ` Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() Amit Langote <[email protected]>
@ 2026-04-16 05:17 ` Amit Langote <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Amit Langote @ 2026-04-16 05:17 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: 胡传文 <[email protected]>; pgsql-hackers <[email protected]>
On Thu, Apr 16, 2026 at 12:03 PM Amit Langote <[email protected]> wrote:
> On Thu, Apr 16, 2026 at 11:05 AM Chao Li <[email protected]> wrote:
> > > On Apr 15, 2026, at 16:28, 胡传文 <[email protected]> wrote:
> > >
> > > Hi,
> > > Found a misleading comment in JsonTablePlanJoinNextRow() while reading
> > > the JSON_TABLE execution code.
> > > The function returns false when both siblings are exhausted (meaning no
> > > more rows), but the comment says "there are more rows" — the exact
> > > opposite of what's happening. The code itself is correct.
> > > if (!JsonTablePlanNextRow(planstate->right))
> > > {
> > > /* Right sibling ran out of row, so there are more rows. */ /* wrong */
> > > return false;
> > > }
> > > A reader might reasonably treat this as a bug and flip the return value,
> > > which would cause JSON_TABLE UNION plans to loop indefinitely.
> > > Patch attached.
>
> Thanks for the report and the patch.
>
> > The fix looks correct to me. I guess “no” was just unintentionally missed.
> >
> > A small comment, I just feel “too” you newly added might not be needed.
>
> Actually, I'd keep it, because it ties the comment to the left-sibling
> check just above.
>
> Will push the attached down to v17.
Done.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-04-16 05:17 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-15 08:28 [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow() =?gb18030?B?uvq0q87E?= <[email protected]>
2026-04-16 02:05 ` Chao Li <[email protected]>
2026-04-16 03:03 ` Amit Langote <[email protected]>
2026-04-16 05:17 ` Amit Langote <[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