Wrong unsafe-flag test in check_output_expressions()

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

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

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

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

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