Hi, While reviewing aggregate pushdown patch [1] we noticed that RelOptInfos for upper relations do not have relids set. create_foreignscan_plan() copies the relids from RelOptInfo into ForeignScan::fs_relids. That field is used to identify the RTIs covered by the ForeignScan. For example, postgresBeginForeignScan() uses it to get the user mapping 1281 /* 1282 * Identify which user to do the remote access as. This should match what 1283 * ExecCheckRTEPerms() does. In case of a join, use the lowest-numbered 1284 * member RTE as a representative; we would get the same result from any. 1285 */ 1286 if (fsplan->scan.scanrelid > 0) 1287 rtindex = fsplan->scan.scanrelid; 1288 else 1289 rtindex = bms_next_member(fsplan->fs_relids, -1); 1290 rte = rt_fetch(rtindex, estate->es_range_table); 1291 userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
Core functions also use this field to get RTIs covered by ForeignScan e.g. ExplainPreScanNode. Since this field is not set in an upper relation, when we push down an upper operation like grouping/aggregation, fs_relids remains NULL, causing undesirable results. In case of postgres_fdw aggregate pushdown, it crashed in rt_fetch(). We could prevent the crash by passing input_rel->relids to fetch_upper_rel() in create_grouping_path() as seen in the attached patch. preprocess_minmax_aggregates() needed to be fixed so that both these functions add paths to the same relation. I am sure, if we go this route, we will have to fix each call of fetch_upper_rel() in this manner. But I am not sure if it's intended to be so. The comment in fetch_upper_rel() says 847 * An "upper" relation is identified by an UpperRelationKind and a Relids set. 848 * The meaning of the Relids set is not specified here, and very likely will 849 * vary for different relation kinds. which seems to indicate that relids in upper relation will be different that those in the lower relations, but it doesn't say how. We need to fix the usages of fs_relids or the calls to fetch_upper_rel() for pushdown of upper operations. I am not sure which. Please suggest, how to move forward with this. [1] https://www.postgresql.org/message-id/flat/CANEvxPokcFi7OfEpi3%3DJ%2Bmvxu%2BPPAG2zXASLEkv5DzDPhSTk6A%40mail.gmail.com#CANEvxPokcFi7OfEpi3=j+mvxu+ppag2zxaslekv5dzdphst...@mail.gmail.com -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
pg_upper_relids.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
