Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YSQL] ERROR: invalid attnum ... on query with array_agg(json_build_object()) #22533

Closed
1 task done
karthik-ramanathan-3006 opened this issue May 24, 2024 · 3 comments
Closed
1 task done
Assignees
Labels

Comments

@karthik-ramanathan-3006
Copy link
Contributor

karthik-ramanathan-3006 commented May 24, 2024

Jira Link: DB-11462

Description

Setup

CREATE TABLE site (id uuid PRIMARY KEY);
CREATE TABLE address (id uuid PRIMARY KEY, site_id uuid, recorded_at timestamp without time zone);
CREATE INDEX idx_address_problem ON address (site_id HASH) INCLUDE (recorded_at);

Query

yugabyte=# EXPLAIN SELECT
        s.id,
        aa.addresses
FROM site s
JOIN (
    SELECT
        array_agg(json_build_object(
        'id', a.id,
        'site_id', a.site_id
        )) AS addresses
    FROM address a
    WHERE a.site_id = '01663b9e-0d6b-4954-b53c-470069c84f9c'
    AND a.recorded_at IS NULL
    ) aa ON true;

ERROR:  XX000: invalid attnum 3 for relation "s"
LOCATION:  get_variable, ruleutils.c:6903
Time: 340.877 ms

Notes
A combination of factors seem to cause this:

  • A covering index that "covers" all the columns used in the WHERE clause. (address.site_id, address.recorded_at)
  • A combination of array_agg() and json_build_object() (I haven't tested if it is those two specific functions or any two functions)
  • Some of the columns in the inner SELECT go to the main table (ie. address.id in the above example)

Issue Type

kind/bug

Warning: Please confirm that this issue does not contain any sensitive information

  • I confirm this issue does not contain any sensitive information.
@karthik-ramanathan-3006 karthik-ramanathan-3006 added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels May 24, 2024
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue and removed status/awaiting-triage Issue awaiting triage labels May 24, 2024
@kmuthukk
Copy link
Collaborator

Potential duplicate of #13117 ?

@tanujnay112
Copy link
Contributor

Issue goes away when yb_enable_expression_pushdown is disabled. It appears that the issue occurs when we try to deparse the Storage Index Filter on idx_address_problem. On the offending line, the variable a.recorded_at is said to have varno = 1 and varattno = 3. In this context, however, varno = 1 seems to be interpreted as table s. Any vars referring to table a such as in the index qual seem to have varno = 4.

@andrei-mart It seems like Postgres when deparsing the index qual, it deparses the indexqualorig field instead of indexqual. Do you think something similar needs to be done for the yb_idx_pushdown field?

andrei-mart added a commit that referenced this issue Aug 26, 2024
Summary:
The referenced IndexScan's indextlist wasn't fixed, so in the cases where
IndexScan was in a subquery the Vars in that list pointed to incorrect
RTE after RTEs from the subplan's root were moved to the common RTE
list.

Postgres does not use the indextlist, because all vars in the original
expression lists refer to the main table. It was changed in YB, when
expression pushdown support was added to the IndexScan. The pushdown
expressions associated with the index, should refer to the index relation
instead, and that kind of fix was done. But indextlist was forgotten,
and this list is needed when EXPLAIN looks up the main table columns for
Vars in the Storage Index List.

Also code was refactored to avoid fixing references unnecessarily if
index pushdown is not present.
Jira: DB-11462

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: tnayak

Reviewed By: tnayak

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37487
@andrei-mart
Copy link
Contributor

Problem is only with EXPLAIN, the query is executed correctly.
For issue to occur there should be IndexScan with a Storage Index Filter in a subquery.

jasonyb pushed a commit that referenced this issue Aug 28, 2024
Summary:
 0c5102e [doc][yba] xCluster Replication update (#23417)
 Excluded: a9466df [#22325] YSQL, QueryDiagnostics: Adding a catalog view for queryDiagnostics
 b9597b3 [#23612] YSQL: Fix java unit test misuse of == for string comparison
 bb72624 [#23613] DocDB: Framework for different vector index coordinate types, SIFT 1B hnsw_tool support
 12032f3 [PLAT-12510] Add option to use UTC for cron expression backup schedule time calculation
 Excluded: 141703a [#22533] YSQL: fix setrefs for index scan
 1bb8c62 [#23543] docdb: Update tablegroup manager in RepartitionTable
 1e28b8a [#23518] Do not include full snapshot info for list snapshot schedules RPC.
 e98c383 [PLAT-15048] Fix auto-master failover local test
 f606132 [doc][yba] Backup clarification (#23611)
 e80d60f [PLAT-14973] Precheck for node agent install to verify that we have correct permissions to execute in the installer directory
 5230f5a [#23630]yugabyted: Modiying the APIs required for the new Migrate Schema Page.
 0a310d3 [PLAT-15042] Add default pitr retention period
 aa15c81 [PLAT-12435] Adding a precheck for libselinux bindings for system python3
 525672e [#23632] DocDB: Unify GetFlagInfos and remove duplicate code
 4ab5ca0 [#23601] YSQL: Fix TestPreparedStatements tests with connection manager enabled
 57a7690 [PLAT-12222][PLAT-15036][PLAT-14333] Add connection pooling support for create universe API
 3407682 [PLAT-10119]: Do not allow back-tick for DB password in YBA

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Differential Revision: https://phorge.dev.yugabyte.com/D37578
andrei-mart added a commit that referenced this issue Aug 28, 2024
Summary:
Original commit: 141703a / D37487

- setrefs.c
  - set_plan_refs
    - The functions fix_upper_expr, fix_scan_list used in the modified code
      take additional parameter num_exec in pg15.

The referenced IndexScan's indextlist wasn't fixed, so in the cases where
IndexScan was in a subquery the Vars in that list pointed to incorrect
RTE after RTEs from the subplan's root were moved to the common RTE
list.

Postgres does not use the indextlist, because all vars in the original
expression lists refer to the main table. It was changed in YB, when
expression pushdown support was added to the IndexScan. The pushdown
expressions associated with the index, should refer to the index relation
instead, and that kind of fix was done. But indextlist was forgotten,
and this list is needed when EXPLAIN looks up the main table columns for
Vars in the Storage Index List.

Also code was refactored to avoid fixing references unnecessarily if
index pushdown is not present.
Jira: DB-11462

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: jason, tfoucher

Reviewed By: jason

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37583
andrei-mart added a commit that referenced this issue Aug 28, 2024
Summary:
Original commit: 141703a / D37487
The referenced IndexScan's indextlist wasn't fixed, so in the cases where
IndexScan was in a subquery the Vars in that list pointed to incorrect
RTE after RTEs from the subplan's root were moved to the common RTE
list.

Postgres does not use the indextlist, because all vars in the original
expression lists refer to the main table. It was changed in YB, when
expression pushdown support was added to the IndexScan. The pushdown
expressions associated with the index, should refer to the index relation
instead, and that kind of fix was done. But indextlist was forgotten,
and this list is needed when EXPLAIN looks up the main table columns for
Vars in the Storage Index List.

Also code was refactored to avoid fixing references unnecessarily if
index pushdown is not present.
Jira: DB-11462

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: tnayak

Reviewed By: tnayak

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37592
andrei-mart added a commit that referenced this issue Aug 28, 2024
Summary:
Original commit: 141703a / D37487
The referenced IndexScan's indextlist wasn't fixed, so in the cases where
IndexScan was in a subquery the Vars in that list pointed to incorrect
RTE after RTEs from the subplan's root were moved to the common RTE
list.

Postgres does not use the indextlist, because all vars in the original
expression lists refer to the main table. It was changed in YB, when
expression pushdown support was added to the IndexScan. The pushdown
expressions associated with the index, should refer to the index relation
instead, and that kind of fix was done. But indextlist was forgotten,
and this list is needed when EXPLAIN looks up the main table columns for
Vars in the Storage Index List.

Also code was refactored to avoid fixing references unnecessarily if
index pushdown is not present.
Jira: DB-11462

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: tnayak

Reviewed By: tnayak

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37602
timothy-e added a commit that referenced this issue Sep 10, 2024
Summary:
The same issue and fix as was present in #22533 (fixed by D37487 / 141703a) that was required for Index Scans was also required for Bitmap Index Scans.

I thought about pulling this common logic into a function that would handle both, but since the type of an `splan` is either an IndexScan or a YbBitmapIndexScan, and making it work for both seems like overcomplicating it.
Jira: DB-12514

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: amartsinchyk

Reviewed By: amartsinchyk

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D37630
timothy-e added a commit that referenced this issue Sep 11, 2024
…x Scan

Summary:
Original commit: 3bf9301 / D37630

* setrefs.c
  * set_plan_refs: case T_YbBitmapIndexScan
    * upstream PG's 41efb8340877e8ffd0023bb6b2ef22ffd1ca014d added a new
      parameter num_exec to fix_upper_expr
    * 3bf9301 / D37630 puts existing fix_upper_expr calls under an if statement and adds a new fix_scan_list call
    * Solution: add the new parameter to fix_upper_expr and fix_scan_list, keep the logical flow of D37630

The same issue and fix as was present in #22533 (fixed by D37487 / 141703a) that was required for Index Scans was also required for Bitmap Index Scans.

I thought about pulling this common logic into a function that would handle both, but since the type of an `splan` is either an IndexScan or a YbBitmapIndexScan, and making it work for both seems like overcomplicating it.
Jira: DB-12514

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: jason, tfoucher

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D37945
timothy-e added a commit that referenced this issue Sep 17, 2024
Summary:
Original commit: 3bf9301 / D37630
The same issue and fix as was present in #22533 (fixed by D37487 / 141703a) that was required for Index Scans was also required for Bitmap Index Scans.

I thought about pulling this common logic into a function that would handle both, but since the type of an `splan` is either an IndexScan or a YbBitmapIndexScan, and making it work for both seems like overcomplicating it.
Jira: DB-12514

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressMisc#testPgRegressMiscSerial'

Reviewers: amartsinchyk

Reviewed By: amartsinchyk

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D37937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants