Horton

Re: Incorrect calculation of path fraction value in MergeAppend

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: Andrei Lepikhov <lepihov@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, "pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>, Tomas Vondra <tomas@vondra.me>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2025-05-18T11:03:49Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix tuple_fraction calculation in generate_orderedappend_paths()

  2. Consider fractional paths in generate_orderedappend_paths

Attachments

On Thu, May 8, 2025 at 2:58 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Thu, May 8, 2025 at 2:42 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
> >
> > On 7/5/2025 12:45, Álvaro Herrera wrote:
> >  > On 2025-May-07, Andrei Lepikhov wrote:
> >  >> The magic is how it finds a link to the thread. This time it doesn't
> >  > Weird, that doesn't seem to work very well if you try to search for
> >  > stuff.  But if you just specify the Message-Id without clicking
> >  > "search" then it works.  So here you go:
> >  > https://commitfest.postgresql.org/patch/5742/
> > Thanks! I think this memo should be pinned at the top of the page.
> >
> > On 7/5/2025 17:35, Junwang Zhao wrote:
> > > + /* Convert absolute limit to a path fraction */
> > > + if (path_fraction >= 1.)
> > > + path_fraction /= childrel->rows;
> > >
> > > maybe add `&& childrel->rows > 0.` to the if condition to
> > > avoid potential crash. I'm not sure if `childrel->rows` will
> > > be 0 but I see get_cheapest_fractional_path did this.
> > You may look at the 76281aa for the reason. Also, looking into the code,
> > I see that it is impossible now to find more places with rows == 0
> > except for the Dummy result node. Moreover, it just doesn't make sense
> > in the case of append nodes: why should the optimiser spend resources
> > and consider paths if it is impossible to pull any tuples from the subplan?
> >
> > So, I think checking assertion instead would be the better option. See
> > new version in the attachment.
>
> Agree, assertion is better, the new version LGTM.

I've slightly revised the patch: commit message and one comment.  I
think this is clearly a bugfix, and it should be backpatched.  I'm
going to push and backpatch it if no objections.

------
Regards,
Alexander Korotkov
Supabase