Re: Wrong unsafe-flag test in check_output_expressions()

От:

David Rowley <dgrowleyml@gmail.com>

Дата:

3 июня в 01:32:43

On Wed, 3 Jun 2026 at 10:13, Richard Guo  wrote:
>
> On Wed, Jun 3, 2026 at 5:59 AM David Rowley  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

Re: Wrong unsafe-flag test in check_output_expressions()

От:

David Rowley <dgrowleyml@gmail.com>

Дата:

2 июня в 23:59:17

On Mon, 1 Jun 2026 at 20:27, Richard Guo  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

Re: Wrong unsafe-flag test in check_output_expressions()

От:

Tender Wang <tndrwang@gmail.com>

Дата:

2 июня в 04:12:55

Richard Guo  于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

Re: Wrong unsafe-flag test in check_output_expressions()

От:

Richard Guo <guofenglinux@gmail.com>

Дата:

3 июня в 01:13:03

On Wed, Jun 3, 2026 at 5:59 AM David Rowley  wrote:
> On Mon, 1 Jun 2026 at 20:27, Richard Guo  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

Re: Wrong unsafe-flag test in check_output_expressions()

От:

Richard Guo <guofenglinux@gmail.com>

Дата:

3 июня в 03:51:29

On Wed, Jun 3, 2026 at 7:32 AM David Rowley  wrote:
> On Wed, 3 Jun 2026 at 10:13, Richard Guo  wrote:
> > On Wed, Jun 3, 2026 at 5:59 AM David Rowley  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

Wrong unsafe-flag test in check_output_expressions()

От:

Richard Guo <guofenglinux@gmail.com>

Дата:

1 июня в 11:27:06

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