Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()

From: Amit Langote <amitlangote09(at)gmail(dot)com>
To: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
Cc: 胡传文 <463945512(at)qq(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()
Date: 2026-04-16 03:03:22
Message-ID: CA+HiwqGJ_hJneTeOVbaJs=ULoCjfgchjgLu8i_CEai4FX+8Vyw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On Thu, Apr 16, 2026 at 11:05 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > On Apr 15, 2026, at 16:28, 胡传文 <463945512(at)qq(dot)com> 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

Attachment Content-Type Size
v1-0001-Fix-incorrect-comment-in-JsonTablePlanJoinNextRow.patch application/octet-stream 1.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2026-04-16 03:08:19 Re: pg_overexplain produces invalid JSON with RANGE_TABLE option
Previous Message Amit Langote 2026-04-16 02:50:22 Re: GetCachedPlan() refactor: move execution lock acquisition out