Skip to content

Keep no-match rows when pulling up a correlated aggregate subquery - #63

Open
Alena0704 wants to merge 168 commits into
REL_2_STABLEfrom
port-pr1928-rel2
Open

Keep no-match rows when pulling up a correlated aggregate subquery#63
Alena0704 wants to merge 168 commits into
REL_2_STABLEfrom
port-pr1928-rel2

Conversation

@Alena0704

Copy link
Copy Markdown

With the Postgres planner (optimizer=off or an ORCA fallback), a correlated scalar subquery with an aggregate is pulled up into an INNER join with a grouped subquery (convert_EXPR_to_join), which drops outer rows that have no match. The original subquery keeps them: it computes the aggregate over empty input, so e.g. COUNT yields 0 there:

    select ... from t1
            where t1.a > (select count(*) from t2 where t2.a = t1.d);

A row with no match in t2 must be compared as "t1.a > 0" and can pass, but the INNER join dropped it.

To fix this, pull the subquery up into a LEFT join, so no-match rows survive as null-extended rows, and build the comparison above the join out of the aggregates the subquery exports:

outer OP expr(CASE WHEN match_flag THEN agg ELSE empty_input_value END)

match_flag is a constant TRUE column added to the subquery, so it is NULL exactly for the rows the LEFT join null-extended. For those the CASE yields what the aggregate returns over an empty input, and the expression around the aggregates then computes what the subquery would have returned.

The empty-input value comes from pg_aggregate: an aggregate with no final function returns its initial transition value (0 for count() and regr_count()), and one whose initial value is null returns NULL as long as its final function is strict (sum(), min(), max(), ...). For the rest -- avg() and friends, where the initial value is not null and only the final function knows that it turns into NULL -- the value cannot be derived, and the INNER join is kept as before.

Substituting the values into the expression and using that as a single ELSE branch is not enough: the result is a constant expression, and constant folding evaluates it while planning, so
"(select 10 / count(*) ...)" would fail with "division by zero" even though the query itself never divides by zero.

The comparison runs above the LEFT join as a filter, not as the join condition: as a join qual it would null-extend matched rows that fail it, and the empty-input value would let them back in.

The LEFT join is not always needed. If a no-match row cannot pass the comparison anyway -- "1 = (select count(*) ...)" turns into "1 = 0" for it -- dropping it is fine and the INNER join is kept as before. This is detected by substituting the empty-input value into the comparison and constant-folding it, which is only done where the folding cannot evaluate anything that would not be evaluated anyway. Ordinary sum/avg/min/max comparisons fall into this group: their empty-input value is NULL, and a comparison with NULL does not pass, so those plans do not change.

The pull-up bails out and the sublink runs as a SubPlan, as before, when the comparison cannot be placed above the join (the sublink is in an outer join's ON clause), when the subquery's targetlist is correlated, and when its expression contains

  • a window function, which after the pull-up would run over all of the groups instead of over the single row an ungrouped aggregate produces for one outer row,

  • an ordered-set or hypothetical-set aggregate, whose empty-input value does not follow from the transition machinery -- "rank(x) within group (order by y)" returns 1,

  • an aggregate of the subquery inside a sub-select, which the rewrite above cannot reach.

Adapted from open-gpdb open-gpdb/gpdb#397 and Greengage GreengageDB/greengage#546.

Co-Authored-By: excaliiibur [excaliiibur@foxmail.com]

usernamedt and others added 30 commits June 19, 2026 11:28
We inherited this issue from PostgreSQL.

PostgreSQL uses glibc to sort strings. In version glibc=2.28, collations
broke down badly (in general, there are no guarantees when updating glibc).
Changing collations breaks indexes. Similarly, a cluster with different
collations also behaves unpredictably.

What and when something has changed in glibc can be found
on https://github.com/ardentperf/glibc-unicode-sorting
Also there is special postgresql-wiki https://wiki.postgresql.org/wiki/Locale_data_changes
And you tube video https://www.youtube.com/watch?v=0E6O-V8Jato

In short, the issue can be seen through the use of bash:

( echo "1-1"; echo "11" ) | LC_COLLATE=en_US.UTF-8 sort

gives the different results in ubunru 18.04 and 22.04.

There is no way to solve the problem other than by not changing the symbol order.
We freeze symbol order and use it instead of glibc.

Here the solution https://github.com/postgredients/mdb-locales.

In this PR I have added PostgreSQL patch that replaces all glibc
locale-related calls with a calls to an external libary. It activates
using new configure parameter --with-mdblocales, which is off by
default.

Using custom locales needs libmdblocales1 package and mdb-locales
package with symbol table.

Build needs libmdblocales-dev package with headers.
* MDB admin patch & tests

This patch introcudes new pseudo-pre-defined role "mdb_admin".

Introduces 2 new function:
extern bool mdb_admin_allow_bypass_owner_checks(Oid userId,  Oid ownerId);
extern void check_mdb_admin_is_member_of_role(Oid member, Oid role);

To check mdb admin belongship and role-to-role ownership transfer
correctness.

Our mdb_admin ACL model is the following:

* Any roles user or/and roles can be granted with mdb_admin
* mdb_admin memeber can tranfser ownershup of relations,
namespaces and functions to other roles, if target role in neither:
superuser, pg_read_server_files, pg_write_server_files nor
pg_execute_server_program.

This patch allows mdb admin to tranfers ownership on non-superuser objects

* f
This commit introduces new mdb internal role mdb_superuser.

Role is capaple of:

GRANT/REVOKE any set of priviledges to/from any object in database.
Has power of pg_database_owner in any database, including:
DROP any object in database (except system catalog and stuff)

Role is NOT capaple of:

Create database, role, extension or alter other roles with such
priviledges.

Transfer ownership to /pass has_priv of roles:

PG_READ_ALL_DATA
PG_WRITE_ALL_DATA
PG_EXECUTE_SERVER_PROGRAM
PG_READ_SERVER_FILES
PG_WRITE_SERVER_FILES
PG_DATABASE_OWNER

Fix configure.ac USE_MDBLOCALES option handling

Apply autoreconf stuff

Set missing ok parameter ito true while acquiring mdb_superuser oid

In regress tests, nobody creates mdb_superuser role, so missing ok is
fine

Allow mdb_superuser to have power of pg_database_owner

Allow mdb_superuser to alter objects and grant ACl to
objects, owner by pg_database_owner. Also, when acl check,
allow mdb_supersuer use pg_database_owner role power to pass check
This import old CI job from open-gpdb/gpdb + yezzey. This merely checks that yezzey and cloudberry compiles together with no issues.


As discussed privately we will redesign it later in another PR. Right now we copy existing code from open-gpdb in order to make sure all out future PR is Ok
* Allow group access for init CBDB

* Allow group access for segments CBDB

---------

Co-authored-by: Leonid Borchuk <xifos@qavm-f9b691f5.qemu>
Co-authored-by: Leonid Borchuk <xifos@qavm-f9b691f5.qemu>
* Add yezzey build option

* Move yezey to commit 4c6b5b8

---------

Co-authored-by: Leonid Borchuk <xifos@qavm-f9b691f5.qemu>
When expanding a cluster, gpexpand copies the postgresql.conf file directly
from the template segment (content 0). This causes issues for tools like
wal-g which use a --content-id flag in archive_command and
restore_command.

Previously, new segments inherited --content-id=0 from the template.
This caused them to push WAL segments to the wrong location, potentially
overwriting segment 0's segments.

This fix ensures the content ID in archive_command and restore_command
is updated to match the new segment's ID during expansion. If the commands
do not contain the --content-id flag, they remain unchanged.
To the MWP cbdb version
* Move yezzey forward to full support Cloudberry
Historically Yandex Greenplum allows non-superuser no managed resource groups.

So, a regular non-superuser role allowed to run pg_resgroup_move_query(), and tune CPU/memory limits if granted with mdb_admin.  Such feature was introduced as early as 6.22, see also gpdb commit 3ac99962. 

This commit introduces same feature for managed Cloudberry. 

To disallow altering predefined roles, fixed-OID hardening is used, reserving 8067 OID to be an mdb_admin role OID. We choose this (efficiently a catalog change) over complex bookkeeping what CREATEROLE can do and what is disallowed. 

We use Yandex managed predefined roles bootstrap util via auxiliary contrib extension, based on what Yandex Postgres fork does, see also pg-sharding/cpg repo.

Co-authored-by: Andrey Borodin <x4mmm@yandex-team.ru>
Co-authored-by: reshke <reshke@double.cloud>
… outputs

- privileges.out: update expected output for terminate_nothrow test to
  show 4 background processes (autovacuum launcher, dtx recovery process,
  logical replication launcher, login monitor) instead of 0 rows, matching
  Cloudberry's actual pg_terminate_backend behavior

- output/misc.source: change expected value from 't' to 'f' for
  mdb_locale_enabled() since CI builds without --with-mdblocales
  (ENABLE_MDBLOCALES defaults to false in configure-cloudberry.sh)
- `gp_interconnect_stats` — aggregated statistics across all segments;
- `gp_interconnect_stats_per_segment` — statistics grouped by segment;
- `gp_interconnect_stats_per_segment_per_host` — statistics grouped by host and segment.

Based on the implementation from OpenGPDB (open-gpdb/gpdb#109).
This commit performs a comprehensive license compliance cleanup to align
with release requirements, which are pointed out by Incubator PMC
review.

The main changes include:

1. Add License Headers: Added the standard Apache License Version 2.0
  header to numerous source files that were missing it. This covers
  multiple file types, including YAML, Markdown, SQL, C/C++, Python,
  and shell scripts. These files are originally created by the
  cloudberry community.
2. Simplify LICENSE and NOTICE:
  - Restructured the root LICENSE file for better clarity.
  - Cleaned up the NOTICE file by removing redundant information which
    have been listed in the LICENSE.
3. Remove the unused deployment docs from the `deploy/build`, which can
  help us manage the file licenses.
4. Update RAT Configuration: Updated `pom.xml` to reflect the changes of
   the file license headers and attribution.

See: apache#1236
For a selected list of PG system views (started with 'pg_'prefix ), we
will create a corresponding 'gp_' view for each one in the list.
Each 'gp_' view is basically a UNION ALL of the results of running the
corresponding 'pg_' view on all segments (including the coordinator).

Note that, these views do not aggregate the results. The aggregate
version of the views will be named with a '_summary' appendix (such
as 'gp_stat_all_tables_summary').

To add a new 'pg_' view to this list, simply put the name in file
'src/backend/catalog/system_views_gp.in'. This commit adds an initial
list of views that we think make sense to have 'gp_' views.

With this change, we also remove the existing definition of
gp_stat_archiver view and let it be generated automatically.
We also had gp_stat_replication but it carries additional column than
pg_stat_replication so it cannot use the automatic way.
Some pg_ views have been modified by cbdb: the gp_segment_id
colmun has been added to them. So they are failed to be transformed
from the pg_ views to gp_ views (see commit
5028222620d410fe3d4c60f732a599e269006968)
So just remove them from system_vies_gp.in. Maybe better to fix
them later.
We used to not have a very clear naming guideline for the existing
'pg_%' system views and the MPP versions of them. As an example,
we renamed PG's pg_stat_all_tables and pg_stat_all_indexes to have
an '_internal' appendix, and used their original names to collect
aggregated results from all segments (commit e6f9303).

However, with the previous commit, we now let all existing PG system
views to have their original names, while add corresponding 'gp_%'
views for the non-aggregated results from all segments, and
'gp_%_summary' views for aggregated results from all segments.

Therefore, we now revert pg_stat_all_tables and pg_stat_all_indexes
back to their original definitions, which just collect stats from
a single segment. Then, we add them to sytem_views_gp.in to produce
gp_stat_all_tables and gp_stat_all_indexes which collect non-aggregated
results from all segments. Finally, we rename the aggregate version of
those views to be gp_stat_all_tables_summary and gp_stat_all_indexes_summary.

Because views pg_stat_user_tables and pg_stat_user_indexes use the above
sumary views, we have to add _summary views for these two views as well.
We will add _summary for other system views later.

Modify regress test accordingly.
Added the following views:

gp_stat_progress_vacuum_summary
gp_stat_progress_analyze_summary
gp_stat_progress_cluster_summary
gp_stat_progress_create_index_summary

Also replaced pg_stat_progress_* views with gp_stat_progress_* views for
existing tests.
These summary views offer basic aggregation of the gp_stat_* views across Greenplum coordinator and
segments.

Aggregation logic applied as follows:
* Time related (last_%): use max()
* Transaction related, not innately summable (number of commits/rollbacks) : use max()
* Table specific: sum()/numsegments for replicated tables, sum() for
  distributed tables
* Innately summable stats, if no particular table is involved: use sum()
* pid: use coordinator's pid (not used here, but this is the convention in other gp_%_summary views)
Regenerate configure file from configure.ac by autoconf
…tats_ext_exprs. (apache#1551)

This is
postgres/postgres@c342538 commit, applied to Cloudberry. There was no issues in apply, only changes are to gporca expected output

original commit message follows

===

The catalog view pg_stats_ext fails to consider privileges for expression statistics.  The catalog view pg_stats_ext_exprs fails to consider privileges and row-level security policies.  To fix, restrict the data in these views to table owners or roles that inherit privileges of the table owner.  It may be possible to apply less restrictive privilege checks in some cases, but that is left as a future exercise.  Furthermore, for pg_stats_ext_exprs, do not return data for tables with row-level security enabled, as is already done for pg_stats_ext.

On the back-branches, a fix-CVE-2024-4317.sql script is provided that will install into the "share" directory.  This file can be used to apply the fix to existing clusters.

Bumps catversion on 'master' branch only.

Reported-by: Lukas Fittl
Reviewed-by: Noah Misch, Tomas Vondra, Tom Lane
Security: CVE-2024-4317
Backpatch-through: 14
setup_cdb_schema() checked errno after a readdir() loop without resetting
it beforehand. In some environments (e.g., Ubuntu 24.04), a stale errno
value from operations inside the loop (such as pg_realloc or pg_strdup)
could persist, causing readdir's normal termination to be misinterpreted
as a failure (e.g., "Function not implemented").

This commit fixes the issue by adopting the standard PostgreSQL idiom:
- Use "while (errno = 0, (file = readdir(dir)) != NULL)" to ensure errno
  is cleared strictly before each readdir() call.
- Move closedir() after the errno check to prevent it from overwriting
  the error code from readdir().
- Add defensive error checking for the closedir() call itself.

This ensures robust directory scanning and reliable error reporting
during cluster initialization.
tuhaihe and others added 22 commits June 29, 2026 16:10
Add libicu-devel package to Rocky Linux 8, 9, and 10 Dockerfiles
to provide ICU (International Components for Unicode) library
support required for PostgreSQL 16 kernel compilation.

This dependency is already present in Ubuntu 22.04 and Ubuntu 24.04
development images, ensuring consistency across all supported build
platforms for PostgreSQL 16 compilation requirements.
The command `gppkg --clean` fails with the following error: "'SyncPackages' object has no attribute 'ret'".

This occurs because `operations` was being passed positionally during the OperationWorkerPool initialization, which incorrectly bound it to the `should_stop` argument instead of `items` in the base WorkerPool class.

The solution is to  pass `operations` as a keyword argument..
…pache#1727)

* Fix: FDW OPTIONS encoding accepts symbolic names (issue apache#1726)

Both the FDW catalog reader (src/backend/access/external/external.c)
and the gp_exttable_fdw option validator
(gpcontrib/gp_exttable_fdw/option.c) parsed the "encoding" OPTIONS value
with atoi(). atoi("UTF8") returns 0 (PG_SQL_ASCII) and PG_VALID_ENCODING(0)
is true, so symbolic names like 'UTF8', 'utf-8', 'GBK' silently fell through
validation and were stored as SQL_ASCII at read time. By contrast, the
legacy CREATE EXTERNAL TABLE ... ENCODING ... path resolves names via
pg_char_to_encoding() and persists a numeric form into OPTIONS — only the
FDW OPTIONS entry point bypassed that translation.

Add a small shared helper parse_fdw_encoding_option(const char *) in
src/backend/access/external/external.c (declared in
src/include/access/external.h):

  - first try pg_char_to_encoding(name) — same logic as the legacy path;
  - otherwise try a strict numeric form via strtol() with end-of-string
    and PG_VALID_ENCODING() checks (atoi is intentionally avoided, since
    atoi("UTF8")==0 is the bug being fixed);
  - otherwise ereport(ERROR).

Both the validator and GetExtFromForeignTableOptions() call this helper.
On-disk values in pg_foreign_table.ftoptions are stored verbatim as the
user wrote them; correctness is established at read time. This avoids a
ProcessUtility_hook approach, which is unworkable here because the
extension's _PG_init runs lazily on the first dlopen, after the current
statement's hook check has already passed.

Affected scope: gp_exttable_fdw (used by gp_exttable_server). The
standalone pxf_fdw is unaffected — its validator already routes encoding
through ProcessCopyOptions, which is name-aware.

Behavior change on upgrade: existing rows whose ftoptions literally contain
encoding=<name> have, until now, been silently interpreted as SQL_ASCII.
After this fix they are interpreted as the named encoding. This will be
called out in the release notes; a detection query is provided in the PR
description for operators who wish to pin specific tables to numeric form
before upgrade.

Tests added in gpcontrib/gp_exttable_fdw/{input,output}/gp_exttable_fdw.source
cover encoding '6' / 'UTF8' / 'utf-8' / 'GBK' / 'bogus' and an
ALTER FOREIGN TABLE ... OPTIONS (SET encoding 'UTF8') path. The pre-existing
encoding '-1' error case has its expected error message updated to match
the new helper's wording.

* test: pad expected output headers to match psql separator widths

The new tests added in the previous commit had column header lines
without the trailing-space padding that psql's aligned output emits
to match the separator. The pre-existing ext_special_uri header
(' a | b') was also unintentionally stripped of its trailing space
during the same edit.

Pure whitespace fix. No behavior change.

* test: drop trailing blank line in gp_exttable_fdw expected output

pg_regress diffs the expected and actual .out files strictly, including
the final newline count. The new encoding test block ended with a
stray empty line (";\n\n") while psql produces ";\n", causing a 1-line
diff at end-of-file. Pure whitespace fix.

* test: reject mixed numeric+letters in FDW encoding option

Add a regression case for `encoding '6abc'`. atoi("6abc") would have
silently returned 6 (= UTF8), which is the class of bug that motivated
moving the FDW encoding option parser off atoi() and onto a strict
strtol() form in parse_fdw_encoding_option(). Without this test, the
strictness of the numeric path was not directly exercised — only the
"unknown name" path ('bogus') was.

Pure test addition; no code change. Lands the third of the reviewer's
suggestions on issue apache#1726 (the first two — strict strtol parsing and a
single shared helper between the validator and the read path — were
already in place in the original fix commit).

* ci: retrigger to clear flaky alter_distribution_policy

---------

Co-authored-by: chenqiang <chenqiang@hashdata.cn>
ClearAOCSFileSegInfo/ClearFileSegInfo (called from
ao_vacuum_rel_recycle_dead_segments) updates pg_aoseg rows via
simple_heap_update, which assigns the current CommandId to the new tuple.
AppendOptimizedTruncateToEOF then opens a catalog snapshot via
GetCatalogSnapshot, which also uses GetCurrentCommandId.  Because both
operations share the same CommandId, the just-zeroed rows are invisible
to the snapshot (cid >= snapshot->curcid), while the old rows with their
original non-zero EOF values remain visible.  TruncateAOSegmentFile then
sees a 0-byte physical file but a non-zero logical EOF and raises:

  "file size smaller than logical eof"

Advancing the command counter before AppendOptimizedTruncateToEOF
ensures the zeroed rows are visible to its catalog snapshot (their cid
is now strictly less than the new curcid).

Fixes: apache#1746
ReleaseSysCache(htup) was called before NameStr(staForm->stxname) was
read, returning a pointer into the already-released tuple buffer.
Copy the name with pstrdup() first, then release the cache entry.
This PR fixes the recovery flow when the internal WAL replication slot does not already exist on the source segment.

Before this change, both gpsegrecovery and gpconfigurenewsegment would start pg_basebackup first and only retry with slot creation after the backup failed. In practice, that meant a full base backup could run for a long time and then fail at the end because the slot was missing.

This change fixes that at the root:

adds a shared helper to check whether the replication slot already exists
creates the slot up front when needed, before pg_basebackup starts
removes the fallback second pg_basebackup attempt from both recovery paths
updates unit tests to cover the new behavior and the new failure mode

---------

Co-authored-by: Leonid <63977577+leborchuk@users.noreply.github.com>
The initial source is got from https://github.com/open-gpdb/gp_url_tools.
Fix decoding of UTF-16 surrogate pairs such as emoji in decode_url(). Make
url_tools_schema accessible to all users via GRANT USAGE ON SCHEMA. Add new
tests. Make refactoring.

There is the extension description in gpcontrib/gp_url_tools/README.md.
Specific option needed to pass parameters to configure script while building on a specific cloud farms
* Fix: MDB admin counterpatch

* Feat: Update comment and refactoring
Support GPDB varlena layout

This commit allows cloudbery use GPDB varlena layout, otherwise
upgrading from greenplum is blocked.
Additionally bump catalog version and write varlena layout metadata (gpdb6 compatible
or native Little Endian) in controldata. This allows us to prohibit using wrong binaries
with wrong data format, preventing data corruption.
For CI: add legacy varlena option to configure and default it to true for our builds
TRY_CONVERT(source_value, default_value) casts source_value to the type
of default_value and returns default_value whenever the cast fails,
which is the behaviour of TRY_CAST in SQL Server.  Without it a single
malformed value makes the whole query fail, and the plpgsql workarounds
that catch the error per row are several times slower.

    TRY_CONVERT('42'::text, NULL::int2)   -- returns 42::int2
    TRY_CONVERT('42d'::text, NULL::int2)  -- returns NULL::int2
    TRY_CONVERT('42d'::text, 1234::int2)  -- returns 1234::int2

The conversion to use is resolved the same way the parser resolves an
explicit cast in coerce_type(): a pg_cast entry, an I/O conversion or a
binary-compatible relabel, followed by the length coercion of the
target type.  Casts between array types and casts to domain types are
not supported and are reported as query errors, the same as a cast that
does not exist at all; only a failure caused by the converted data is
turned into the default value.

The cast itself runs inside a PG_TRY() block, since the datatype input
functions of Cloudberry cannot yet report a conversion failure without
throwing.

The extension is marked trusted, so the owner of a database can install
it without being a superuser.


See: PR#901 <apache#901>
(cherry picked from commit 420a628)

Co-authored-by: Vladimir Rachkin <vova@kpnn.ru>
* Initial commit

* Fix string out of bounds error and change functions to use text type

* Add readme

* Add debian packaging configuration

* Feat: Adapt uuid_cb for Cloudberry

* Refactor: Minor refactoring. Delete Windows adapt. Update README.md and comments

---------

Co-authored-by: Maxim Smyatkin <smyatkinmaxim@gmail.com>
* Import gp_relaccess_stats into gpcontrib from Greenplum

* Feat: Adapt gp_relaccess_stats for Cloudberry

* Fix: Switch to the old API

---------

Co-authored-by: Maxim Smyatkin <smyatkinmaxim@gmail.com>
It lets a session expect the live execution state of another running query without waiting for the finish.
* add: pgqs tests
* add: pgqs source files
* add: pgqs kernel patches
@Alena0704
Alena0704 marked this pull request as draft September 3, 2026 07:38
gp_percentile_cont_{float8,interval,timestamp,timestamptz}_transition and
gp_percentile_disc_transition all read five arguments in C: the running
transition state plus the four arguments of the gp_percentile_cont() and
gp_percentile_disc() aggregates.  pg_proc.dat, however, declares them with
four.  As transition functions they are called correctly, since the executor
supplies state + 4 arguments regardless of the catalog, but a direct SQL call
reaches past the end of the argument array: PG_GETARG_INT64(4) picks up
garbage, which yields wrong results, an assertion when the bogus peer count
makes the code pfree() a NULL pointer, or a segfault.

Correcting the declaration would change the catalog and force an initdb, which
is not acceptable on a stable branch, so check the argument count instead and
raise a plain error.  Direct calls were never useful - the functions only make
sense as the transition step of their aggregates - and 'percentile_* WITHIN
GROUP' queries are unaffected either way.

Co-authored-by: Georgy Shelkovy <g.shelkovy@arenadata.io>

Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770)
Both gp_percentile transition functions return the previous transition state
untouched when the row they are looking at is not one of the rows the
percentile is computed from.  On the very first call that state is NULL, and
returning it as a bare Datum(0) with isnull left false loses the flag: the
aggregate yields 0 instead of NULL for by-value types, and dereferences a NULL
pointer in the output function for by-reference ones - so an empty input set
crashed the backend for the interval, timestamp and timestamptz variants.

Co-authored-by: Georgy Shelkovy <g.shelkovy@arenadata.io>

Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770).
@Alena0704
Alena0704 marked this pull request as ready for review September 7, 2026 17:30
With the Postgres planner (optimizer=off or an ORCA fallback), a
correlated scalar subquery with an aggregate is pulled up into an INNER
join with a grouped subquery (convert_EXPR_to_join), which drops outer
rows that have no match. The original subquery keeps them: it computes
the aggregate over empty input, so e.g. COUNT yields 0 there:

	select ... from t1
		where t1.a > (select count(*) from t2 where t2.a = t1.d);

A row with no match in t2 must be compared as "t1.a > 0" and can pass,
but the INNER join dropped it.

To fix this, pull the subquery up into a LEFT join, so no-match rows
survive as null-extended rows, and rewrite the comparison to return the
same value the subquery would:

    outer OP CASE WHEN match_flag THEN expr ELSE empty_input_default END

match_flag is a constant TRUE column added to the subquery. For a matched
row the CASE returns the real expression; for a null-extended row the flag
is NULL and the CASE returns the empty-input default (0 for COUNT, NULL for
other aggregates).

The comparison runs above the LEFT join as a filter, not as the join
condition: as a join qual it would null-extend matched rows that fail it,
and the default would let them back in.

The LEFT join is not always needed. If a no-match row cannot pass the
comparison anyway -- e.g. "1 = (select count(*) ...)" turns into "1 = 0"
for it -- dropping it is fine and the INNER join is kept as before. This is
detected by substituting the empty-input default into the comparison and
constant-folding it. Ordinary sum/avg/min/max comparisons fall into this
group: their empty-input value is NULL, and a comparison with NULL does not
pass, so those plans do not change.

If the comparison cannot be placed above the join (the sublink is in an
outer join's ON clause) or the subquery's targetlist is correlated, the
pull-up bails out and the sublink runs as a SubPlan, as before.

Adapted from open-gpdb open-gpdb/gpdb#397 and
Greengage GreengageDB/greengage#546.

Co-Authored-By: excaliiibur [excaliiibur@foxmail.com]
(cherry picked from commit ffdcd78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.