Horton

Thread

Commits

  1. Fix wrong unsafe-flag test in check_output_expressions()

  1. Wrong unsafe-flag test in check_output_expressions()

    Richard Guo <guofenglinux@gmail.com> — 2026-06-01T08:27:06Z

    I happened to notice $subject when working on a bug-fix near-by.

   /* If subquery uses window functions, check point 4 */
   if (subquery->hasWindowFuncs &&
       (safetyInfo->unsafeFlags[tle->resno] &
        UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
       !targetIsInAllPartitionLists(tle, subquery))
   {
       /* not present in all PARTITION BY clauses, so mark it unsafe */
       safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
       continue;
   }

So point 4 tests UNSAFE_NOTIN_DISTINCTON_CLAUSE while setting
UNSAFE_NOTIN_PARTITIONBY_CLAUSE.  This does not seem correct to me.
Each check in this loop should guard on the same bit it is about to
set, as an idempotency optimization.

Fortunately, this does not lead to wrong plans.  When
UNSAFE_NOTIN_PARTITIONBY_CLAUSE is already set but
UNSAFE_NOTIN_DISTINCTON_CLAUSE is not, the guard fails to skip
targetIsInAllPartitionLists() and recomputes it, but setting the same
bit again changes nothing.  When UNSAFE_NOTIN_DISTINCTON_CLAUSE is
already set, point 4 is skipped and UNSAFE_NOTIN_PARTITIONBY_CLAUSE is
left unset; but such a column is already unsafe for pushdown via
UNSAFE_NOTIN_DISTINCTON_CLAUSE, so the outcome is unchanged.

Attached is the one-line fix.

- Richard

  2. Re: Wrong unsafe-flag test in check_output_expressions()

    Tender Wang <tndrwang@gmail.com> — 2026-06-02T01:12:55Z

    Richard Guo <guofenglinux@gmail.com> 于2026年6月1日周一 16:27写道：
>
> I happened to notice $subject when working on a bug-fix near-by.
>
>    /* If subquery uses window functions, check point 4 */
>    if (subquery->hasWindowFuncs &&
>        (safetyInfo->unsafeFlags[tle->resno] &
>         UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
>        !targetIsInAllPartitionLists(tle, subquery))
>    {
>        /* not present in all PARTITION BY clauses, so mark it unsafe */
>        safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
>        continue;
>    }
>
> So point 4 tests UNSAFE_NOTIN_DISTINCTON_CLAUSE while setting
> UNSAFE_NOTIN_PARTITIONBY_CLAUSE.  This does not seem correct to me.
> Each check in this loop should guard on the same bit it is about to
> set, as an idempotency optimization.

I don't know too much about this code, but per the nearby comments,
it seems UNSAFE_NOTIN_PARTITIONBY_CLAUSE is the correct flag to use.

>
> Fortunately, this does not lead to wrong plans.  When
> UNSAFE_NOTIN_PARTITIONBY_CLAUSE is already set but
> UNSAFE_NOTIN_DISTINCTON_CLAUSE is not, the guard fails to skip
> targetIsInAllPartitionLists() and recomputes it, but setting the same
> bit again changes nothing.  When UNSAFE_NOTIN_DISTINCTON_CLAUSE is
> already set, point 4 is skipped and UNSAFE_NOTIN_PARTITIONBY_CLAUSE is
> left unset; but such a column is already unsafe for pushdown via
> UNSAFE_NOTIN_DISTINCTON_CLAUSE, so the outcome is unchanged.
>
> Attached is the one-line fix.

Yes, your analysis is correct.
+1

-- 
Thanks,
Tender Wang

  3. Re: Wrong unsafe-flag test in check_output_expressions()

    David Rowley <dgrowleyml@gmail.com> — 2026-06-02T20:59:17Z

    On Mon, 1 Jun 2026 at 20:27, Richard Guo <guofenglinux@gmail.com> wrote:
>
> I happened to notice $subject when working on a bug-fix near-by.
>
>    /* If subquery uses window functions, check point 4 */
>    if (subquery->hasWindowFuncs &&
>        (safetyInfo->unsafeFlags[tle->resno] &
>         UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
>        !targetIsInAllPartitionLists(tle, subquery))
>    {
>        /* not present in all PARTITION BY clauses, so mark it unsafe */
>        safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
>        continue;
>    }

Yes, that's not intentional. Your proposed fix looks correct. Are you
happy to go ahead with pushing and backpatching that?

Wondering, did you spot that with eyes or tool assist?

David

  4. Re: Wrong unsafe-flag test in check_output_expressions()

    Richard Guo <guofenglinux@gmail.com> — 2026-06-02T22:13:03Z

    On Wed, Jun 3, 2026 at 5:59 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Mon, 1 Jun 2026 at 20:27, Richard Guo <guofenglinux@gmail.com> wrote:
> > I happened to notice $subject when working on a bug-fix near-by.
> >
> >    /* If subquery uses window functions, check point 4 */
> >    if (subquery->hasWindowFuncs &&
> >        (safetyInfo->unsafeFlags[tle->resno] &
> >         UNSAFE_NOTIN_DISTINCTON_CLAUSE) == 0 &&
> >        !targetIsInAllPartitionLists(tle, subquery))
> >    {
> >        /* not present in all PARTITION BY clauses, so mark it unsafe */
> >        safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
> >        continue;
> >    }

> Yes, that's not intentional. Your proposed fix looks correct. Are you
> happy to go ahead with pushing and backpatching that?

Thanks for taking a look.  Yeah, I'm happy to push that.  I'm kind of
unsure whether this needs to be back-patched, since it doesn't lead to
wrong plans, so it isn't a live bug in practice.

> Wondering, did you spot that with eyes or tool assist?

For this one, I spotted it with eyeballs, when I was working on
another patch that fixes qual pushdown past window functions (and
other grouping layers) with mismatched opfamily/collation.

- Richard

  5. Re: Wrong unsafe-flag test in check_output_expressions()

    David Rowley <dgrowleyml@gmail.com> — 2026-06-02T22:32:43Z

    On Wed, 3 Jun 2026 at 10:13, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Wed, Jun 3, 2026 at 5:59 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > Yes, that's not intentional. Your proposed fix looks correct. Are you
> > happy to go ahead with pushing and backpatching that?
>
> Thanks for taking a look.  Yeah, I'm happy to push that.  I'm kind of
> unsure whether this needs to be back-patched, since it doesn't lead to
> wrong plans, so it isn't a live bug in practice.

I'd prefer it to be fully gone in all versions that have it, if you
don't mind doing that.

David

  6. Re: Wrong unsafe-flag test in check_output_expressions()

    Richard Guo <guofenglinux@gmail.com> — 2026-06-03T00:51:29Z

    On Wed, Jun 3, 2026 at 7:32 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 3 Jun 2026 at 10:13, Richard Guo <guofenglinux@gmail.com> wrote:
> > On Wed, Jun 3, 2026 at 5:59 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > > Yes, that's not intentional. Your proposed fix looks correct. Are you
> > > happy to go ahead with pushing and backpatching that?

> > Thanks for taking a look.  Yeah, I'm happy to push that.  I'm kind of
> > unsure whether this needs to be back-patched, since it doesn't lead to
> > wrong plans, so it isn't a live bug in practice.

> I'd prefer it to be fully gone in all versions that have it, if you
> don't mind doing that.

Done.  Back-patched through v15, where the buggy check was first
introduced.

- Richard