[YSQL] Bitmap Scans use redundant table filter for partial indexes #23859
Assignees
Labels
2024.1.3_blocker
2024.2 Backport Required
area/ysql
Yugabyte SQL (YSQL)
kind/bug
This issue is a bug
priority/medium
Medium priority issue
Comments
timothy-e
added
area/ysql
yugabyte-ci
added
kind/bug
timothy-e
added a commit
that referenced
this issue
Sep 11, 2024
…emote filters Summary: Original commit: 4ea354b / D36484 Merge conflicts: * createplan.c: * `create_bitmap_subplan` declaration: * D36484 removed some of the parameters that were added by YB * Upstream realigned declarations * resolution: remove the parameters but follow new indentation practice * `create_indexscan_plan` declaration: * D36484 moved it to `planmain.h` * Upstream realigned declarations * resolution: move it to `planmain.h` * `create_bitmap_subplan`: * D36484 removed references to `indexpushdownquals` * pg15's cd1df46 moved one of the references lower in the file * resolution: remove from the new location * `costsize.c` * `get_bitmap_index_quals` * D36484 copied the approach of `create_bitmap_subplan` to get index quals. * `ipath->indexquals` was removed in pg15. * Resolution: Rewrote the BitmapIndexScan case, referencing pg15's create_bitmap_subplan * planmain.h: * declarations * D36484 moved `create_indexscan_plan` from `createplan.h` and added `yb_get_bitmap_index_quals` * Upstream changed indentation of neighbouring declarations * because these new declarations were not spaced out from their neighbours, the changes caused conflicts. Include the new declarations. * yb_pg_select.out * D36484 mistakenly removed logic to account for quals implied by partial indexes, resulting in redundant storage filters being added. This is tracked by #23859. In this current diff, the BitmapIndexScan case of get_bitmap_index_quals needed to be rewritten for resolving costsize.c conflicts, and the rewritten code includes handling of partial indexes. The redundant storage filters are only introduced on master, not in the pg15 branch. * yb-pg15's 2750e71 removed yb ordering * Solution: use the pg15 test file, because the redundant filters are not added by this code. ----- Prior to this diff, Bitmap Scan CBO did not handle remote filter costs. Evaluating a remote filter was given no cost, so it was effectively "free". Therefore AND conditions were seen as better to use remote filters for, rather than Bitmap And, because Bitmap And nodes add cost but remote filters added no cost. To fix this, I 1. determine which quals will be evaluated by the index (as an index cond or a remote filter) in `yb_get_bitmap_index_quals`. This means that the previous change to `create_bitmap_subplan` that collected pushdown expressions can be reverted. The new function has a simpler interface, and allows for better comparing the given qual against the quals evaluated by the plan tree - now some redundant filters can be skipped. (more on this below) 2. determine which quals remain to be addressed by the Bitmap Table Scan node. 3. Account for the cost of evaluating those filters in the same manner that yb_cost_seqscan does, using `cost_qual_eval`. This allows for Bitmap And plans to be a reasonable alternative to remote filter plans. **Moving the discourage modifier:** I also changed the DISCOURAGE_MODIFIER to affect the startup and runtime costs of Bitmap Table Scan instead of the index scans. Now we no longer incentivise Bitmap Scan plans with more work done in the Bitmap Table Scan node. **Removing redundant tests: ** Removed some unstable tests from the yb_bitmap_scan_flags regress tests. They failed with the addition of this change, but actually don't provide anything meaningful. They answer the question: "what will the planner choose when both options are disabled?" which depends on exact details of how each plan is costed. That does not belong in a regress test. **Removing redundant remote filters**: the planner identifies which nodes are resposible for evaluating the given conditions. The Bitmap Table scan node should evaluate anything that's not already implied by the conditions on the Bitmap Index Scans. However, with the addition of a storage index filters on Bitmap Index Scans, this logic was slightly faulty. Two lists of clauses were created, one for index conditions and one for index remote filters. Consider the conditons: `a < 10 OR (b < 10 AND remote_fllter(b))`. This might create a plan like: ``` Bitmap Table Scan Storage Filter: xxx -> Bitmap Or -> Bitmap Index Scan Index Cond: a < 10 -> Bitmap Index Scan Index Cond: b < 10 Storage Index Filter: remote_flter(b)) ``` The list of index conditions was: `OR(a < 10, b < 10)`. The list of index filters was: `remote_filter(b)`. Neither of these indiviudally implied the original condition, so the planner decided it was necessary to apply the remote filters. With this change, a single unified list of index conditions and storage index filters is returned. In this case, the list is `OR(a < 10, AND(b < 10, remote_filter(b))`. Since the original condition is implied by (actually equivalent to) this expression, the planner can now easily determine that the original conditions are already met, so there is no need to add a storage filter. Jira: DB-12438 Test Plan: ``` ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressYbBitmapScans' ./yb_build.sh --java-test 'org.yb.pgsql.TestPgCostModelSeekNextEstimation' ``` TAQO report: {F279017} Bitmap scan is often the most optimal plan for these queries, but less frequently the default plan with this change because of the movement of the discourage factor and the addition of the remote filter cost. While these are regressions, the feature is not yet in EA and further CBO changes are incoming, including [[ #23565 | #23565 ]] to remove the discourage factor entirely. This is a helpful step in the right direction. Reviewers: jason, tfoucher, mtakahara Reviewed By: jason, mtakahara Subscribers: mtakahara, yql, dsherstobitov Differential Revision: https://phorge.dev.yugabyte.com/D37658
timothy-e
added a commit
that referenced
this issue
Sep 16, 2024
Summary: One of the things that `create_bitmap_subplan` does is generates a list of conditions that are guaranteed by the tree of Bitmap Ands, Bitmap Ors, and Bitmap Index Scans. This is used to determine the clauses that remain to be checked by the Bitmap Heap Scan / YB Bitmap Table Scan. 4ea354b / D36484 introduced `yb_get_bitmap_index_quals` that just generates the list of conditions. This makes the filter information available for use during the costing phase, before the actual plan nodes need to be constructed. `create_bitmap_subplan` accounted for the clauses of partial indexes. `yb_get_bitmap_index_quals` missed including this part, which caused the planner to believe the the conditions that are implied by the partial index conditions still need to be rechecked. For example, ```lang=sql create table t (a int, b int); create index idx_a_partial on t (a asc) where b < 10; create index idx_b_partial on t (b asc) where a < 10; /*+ Set(yb_enable_bitmapscan true) BitmapScan(t) */ explain select * from t where b < 10 and a < 10; ``` Generates a bitmap subplan that looks as follows: ``` -> Bitmap Index Scan on idx_b_partial (cost=0.00..0.92 rows=8 width=0) Index Cond: (b < 10) ``` We know that `b < 10` is satisfied (by index condition) and `a < 10` is satisfied (by partial index clause). However, `yb_get_bitmap_index_quals` did not report the partial index clause, so it told the Bitmap Table Scan that only `b < 10` was satisfied - requiring the Bitmap Table Scan to use a Storage Filter to check `a < 10`. ``` QUERY PLAN ------------------------------------------ YB Bitmap Table Scan on t Storage Filter: (a < 10) -> Bitmap Index Scan on idx_b_partial Index Cond: (b < 10) (4 rows) ``` With the change in this diff, `yb_get_bitmap_index_quals` reports both clauses, so the Bitmap Table Scan knows that both conditions are already met and it does not need to add a remote filter. ``` QUERY PLAN ------------------------------------------ YB Bitmap Table Scan on t -> Bitmap Index Scan on idx_b_partial Index Cond: (b < 10) (3 rows) ``` **How did this regression get in?** I noticed `yb_pg_select` was failing in D36484. I saw the problem was with Storage Filters but did not pay much attention to whether the storage filters were added or removed. The diff was able to remove some redundant storage filters from other tests, so I assumed it was doing the same here. Jira: DB-12766 Test Plan: ``` ybd --java-test 'org.yb.pgsql.TestPgRegressPgSelect#testPgRegressPgSelect' ``` Reviewers: mtakahara Reviewed By: mtakahara Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D37983
jasonyb
pushed a commit
that referenced
this issue
Sep 16, 2024
…ter-merge Merge YB master commit a0e2c4a titled [PLAT-15156] Pass pg_max_mem_mb during configure phase and committed 2024-09-13T13:27:29+05:30 into YB pg15. YB pg15 initial merge refers to 55782d5. - heap.c: - heap_create_with_catalog: - "Use binary-upgrade override": YB master commits 516ead0 and 16941de change the logic for the if condition to use heap_pg_class_oids_supplied. Upstream PG 37b2764593c073ca61c2baebd7d85666e553928b refactors how RELKIND are handled, moving logic into the if, and upstream PG 9a974cbcba005256a19991203583a94b4f9a21a9 rewords comment. Apply both. - "There might be no TOAST table, so we have to test for it": (same except there is no comment rewording) - RELKIND_HAS_STORAGE(relkind): (refer to FOO) - index.c: - index_create - "Use binary-upgrade override": (same as heap.c function heap_create_with_catalog "Use binary-upgrade override", the comment rewording part) - "Override the index relfilenode": (refer to FOO) - yb_system_views.sql: - CREATE VIEW yb_local_tablets AS: YB master 1a3c113 adds view yb_wait_event_desc, and YB master a9466df adds view yb_query_diagnostics_status. YB pg15 initial merge moves PG view definitions to system_views.sql. Adjacent conflict. - nodeModifyTable.c: - ExecUpdate: - else if (IsYBRelation(resultRelationDesc)): YB master 8bae488 updates the comment explaining what cols_marked_for_update does. YB pg15 be8504d moves that code to function YBExecUpdateAct. Apply in the new location. - if (updateCxt.crossPartUpdate): YB master 8bae488 frees the bitmapset cols_marked_for_update. YB pg15 be8504d moves the code into function YBExecUpdateAct. Apply in the new location. - ybOptimizeModifyTable.c: - YBAreDatumsStoredIdentically (formerly called YBEqualDatums): YB master b57e3c6 removes fcinfo logic that YB pg15 be8504d adjusts. Remove. - createplan.c: - declarations: - create_indexscan_plan: YB master 4ea354b moved this function declaration to planmain.h. Upstream PG changed indentation. Apply to new location. - create_bitmap_subplan: YB master 4ea354b removed some parameters; upstream PG changed indentation. - create_bitmap_subplan: YB master 4ea354b removed indexpushdownquals code. YB pg15 cd1df46 moved one of such code lower. Remove in the new location. - setrefs.c: - set_plan_refs: - case T_IndexScan: upstream PG 41efb8340877e8ffd0023bb6b2ef22ffd1ca014d adds param num_exec to fix_upper_expr and fix_scan_list. YB master 141703a adds a fix_scan_list function call and indents code containing fix_upper_expr function calls under an if case. - case T_YbBitmapIndexScan: (same but with YB master 3bf9301) - syscache.c: - yb_cache_table_name_table: YB master 9889df7 adds yb_cache_table_name_table to yb_catalog_cache_tables listing catalog tables that have catcaches. Upstream PG adds three new catalog tables and six new catcaches between 11.2 and 15.2. Add the new entries to these lists. Also, adjacent conflict with YB pg15 bea1ffb that removes pin cache code. - yb_ash.c: - yb_ash_ProcessUtility: YB pg15 f1c1a65 changes completionTag to qc. YB master e6337e4 touches various parts of this function. - yb_query_diagnostics.c: - includes: add "common/pg_prng.h" due to the YbQueryDiagnostics_ExecutorStart conflict below. - YbQueryDiagnosticsShmemInit: - LWTRANCHE_YB_QUERY_DIAGNOSTICS: YB master a9466df modifies LWLockRegisterTranche function call and renames variable yb_query_diagnostics_lock to bundles_in_progress_lock. YB pg15 602cb2d replaces LWLockRegisterTranche calls with an entry within the lwlock.c BuiltinTrancheNames array. Drop the LWLockRegisterTranche change; keep the variable rename. - LWTRANCHE_YB_QUERY_DIAGNOSTICS_CIRCULAR_BUFFER: YB master a9466df adds LWLockRegisterTranche function call. Upstream PG 29c3e2dd5a6aeaf1a23d7d83d665501e2dcc6955 replaces LWLockRegisterTranche calls with BuiltinTrancheNames array. Remove LWLockRegisterTranche call and instead add an entry into the lwlock.c BuiltinTrancheNames array similarly to what YB pg15 602cb2d did for LWTRANCHE_YB_QUERY_DIAGNOSTICS. - YbQueryDiagnostics_ExecutorStart: YB master 40689bc adds code involving random and MAX_RANDOM_VALUE. Upstream PG 3804539e48e794781c6145c7f988f5d507418fa8 replaces this pattern with pg_prng_double. Transform similarly. - DumpToFile: YB pg15 602cb2d adds extra param 0 to FileWrite function call. Adjacent conflict with YB master 9f8acff. - lwlock.c: - BuiltinTrancheNames: add entry for LWTRANCHE_YB_QUERY_DIAGNOSTICS_CIRCULAR_BUFFER (see yb_query_diagnostics.c) - pg_dump.c: - dumpCompositeType: adjacent conflict between YB master 516ead0 removing if case and upstream PG 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adding an extra argument to binary_upgrade_set_type_oids_by_type_oid. - dumpTableSchema: YB master 516ead0 makes a slight optimization on code that YB pg15 180e7ae removes. Remove. - dumpSequence: adjacent conflict between YB master 516ead0 removing if case and upstream PG f3faf35f370f558670c8213a08f2683f3811ffc7 replacing binary_upgrade_set_type_oids_by_rel_oid call with comment. - planmain.h: - create_indexscan_plan: (receive changes from createplan.c). Also rewrap. - yb_get_bitmap_index_quals: YB master 4ea354b adds this function. Adjacent conflict with upstream PG changing indentation. Reindent this function's declaration as well. - yb_ysql_dump.data.sql: (refer to BAR) - yb_ysql_dump_colocated_database.data.sql: (refer to BAR) - yb_ysql_dump_legacy_colocated_database.data.sql: (refer to BAR) - yb_ysql_dumpall.data.sql: (refer to BAR) - yb_pg_rules.out: - yb_wait_event_desc: YB master 1a3c113 adds output for the new view yb_wait_event_desc. YB pg15 946a8a9 adds TODO in same area. Adjacent conflict. - yb_pg_select.out: - "partial index implies clause, but bitmap scan must recheck predicate anyway": YB master 4ea354b mistakenly removed logic to account for quals implied by partial indexes, resulting in redundant storage filters being added. This is tracked by #23859. In this merge, during costsize.c merge resolution, this issue is fixed. So the redundant storage filters are only present on master. Modify the expected output to not expect storage filter. - where (unique2 = 11 and stringu1 < 'B') or unique1 = 0: YB pg15 2750e71 removes ybview wrapper on a query that YB master 4ea354b changes expected output of. That is no longer relevant without the ybview, so drop the incoming master change. - yb_reset_analyze.out: - "Grant permissions for this database.": adjacent conflict between YB master 58fd26e adding ALTER DATABASE SET queries and YB pg15 602cb2d grant access permission to the test users. - yb_pg_metrics.c: - enum statementType: YB master 6daf129 sets CatCacheIdMisses_End to CatCacheIdMisses_78. Adapt according to YB pg15 merge 602cb2d, accounting for 6 additional catcache entries. Furthermore, YB master 9889df7 adds a new section CatCacheTableMisses. Adapt it similarly for pg15 by adding three new catalog tables. - yb_pg_publication.out: - CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;: YB master 22b6a47 causes WARNING about wal_level not being "logical" to go away. This warning is only present in YB pg15 because YB pg15 5c5a2a0 was not able to suppress this WARNING for this one case (see that commit message for more details). - yb_publication.out: - CREATE PUBLICATION testpub FOR ALL TABLES;: (same) - yb_pg_pg_stat_statements.out: - Check WAL is generated for the above statements: "DROP TABLE pgss_test" row's wal column values change. Likely related to YB master 22b6a47. Not sure how exactly it relates, but since it makes the test closer to the original pg_stat_statements.out, take it without question. - yb_colocated_tables_with_tablespaces.{sql,out}: - Explicitly specifying pg_default tablespace must not create a new tablegroup: YB master 5eb925b adds these queries with SELECT * from system table, and upstream PG changes that to include the oid column. We don't want to show the oid column since that can easily change, so replace * with explicit column names. - typecmds.c: - AssignTypeMultirangeArrayOid: upstream PG 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adds handling of multi-range data types. Adjust the logic in accordance with YB master 516ead0. (Note that another instance of this issue in AssignTypeMultirangeOid was already resolved separately by YB pg15 2e57f19.) - costsize.c: - yb_get_bitmap_index_quals: YB master 4ea354b copied the approach of create_bitmap_subplan to get index quals. ipath->indexquals was removed in YB pg15. Rewrite the BitmapIndexScan case, referencing YB pg15's create_bitmap_subplan. - postgres.c: - quickdie: YB master 4b4c201 adds quickdie as SIGTERM signal handler for pg_cron. quickdie is not built to expect any signal besides SIGQUIT. Adjust the logic to handle SIGTERM. - pgstat.c: - pgstat_get_wait_event_type: YB master 9f8acff removes IsYugaByteEnabled calls. Upstream PG a333476b925134f6185037eaff3424c07a9f466f moves this function to wait_event.c. Apply to the new location. - pgstat_get_wait_event: (same) - wait_event.c: (receive changes from pgstat.c) - relcache.c: - RelationSetNewRelfilenode: (resolve similarly to typecmds.c, new code added by upstream PG 4ab5dae9472c5c6ada5f5627ec0e0f9f965ade28) - syscache.h: - enum YbCatalogCacheTable: (see syscache.c explanation) - wait_states-itest.cc: - various tests: YB master f77dd6a and 45c9cf8 enabled tests to run on TSAN. Upstream PG b0b39f72b9904bcb80f97b35837ccff1578aa4b8 and 5599f40d259a275d3202b10aa6ffb9a36fd8a940 add pg_GSS_have_cred_cache which calls external function gss_acquire_cred from krb5 library. There seems to be data race in krb5 library when the tests try to create concurrent PG connections, don't enable the TSAN for these tests yet. - pg_ash-test.cc: (same) - pg_libpq-test.cc: - GetCatalogTableNameFromIndexName: YB master 9889df7 adds a test that has a list of all the PG catalog tables. Upstream PG adds three new catalog tables. Add the new tables to this list and, while we're at it, reformat it to have one table per line so this becomes easier to update in the future. - TestYsqlUpgrade.java: - dmlsUpdatePgCache: YB pg15 bea1ffb removes test pinnedObjectsCacheIsUpdated that YB master dcf1821 touches. Remove. - createPgTablegroupTableIfNotExists: YB pg15 4638d7d modifies the function. YB master dcf1821 moves it lower. Apply both. - class TableInfo: YB pg15 93f65c4 adds arrayTypeOid handling. YB master dcf1821 moves it higher. Apply both. FOO: YB master 516ead0 preserves some OIDs during ysql_dump. Upstream PG 9a974cbcba005256a19991203583a94b4f9a21a9 changes pg_dump to preserve relfilenodes in the same code path that was enabled by that master commit. Disable this part for YB. BAR: - instructions to preserve relfilenodes - PG upstream 9a974cbcba005256a19991203583a94b4f9a21a9 adds code to preserve relfilenodes - conflict about whether to preserve OIDs for sequences - YB master 516ead0 turns on output for setting pg_type OID for sequences - Upstream PG f3faf35f370f558670c8213a08f2683f3811ffc7 removes the code that attempts to preserve OIDs for sequences - this was resolved elsewhere in favor of postgres/removing the preservation - changes to the formatting of some values - YB pg15 ed96733 changes the formatting of some values - handling of dumping partitioned tables - YB pg15 180e7ae changes handling of partitioned tables, using attach instead of create - changes in database/ysql_dump versions - these are dumped in the output and thus changed
jasonyb
pushed a commit
that referenced
this issue
Sep 16, 2024
…' into pg15 Summary: Merge YB master commit a0e2c4a titled [PLAT-15156] Pass pg_max_mem_mb during configure phase and committed 2024-09-13T13:27:29+05:30 into YB pg15. YB pg15 initial merge refers to 55782d5. - heap.c: - heap_create_with_catalog: - "Use binary-upgrade override": YB master commits 516ead0 and 16941de change the logic for the if condition to use heap_pg_class_oids_supplied. Upstream PG 37b2764593c073ca61c2baebd7d85666e553928b refactors how RELKIND are handled, moving logic into the if, and upstream PG 9a974cbcba005256a19991203583a94b4f9a21a9 rewords comment. Apply both. - "There might be no TOAST table, so we have to test for it": (same except there is no comment rewording) - RELKIND_HAS_STORAGE(relkind): (refer to FOO) - index.c: - index_create - "Use binary-upgrade override": (same as heap.c function heap_create_with_catalog "Use binary-upgrade override", the comment rewording part) - "Override the index relfilenode": (refer to FOO) - yb_system_views.sql: - CREATE VIEW yb_local_tablets AS: YB master 1a3c113 adds view yb_wait_event_desc, and YB master a9466df adds view yb_query_diagnostics_status. YB pg15 initial merge moves PG view definitions to system_views.sql. Adjacent conflict. - nodeModifyTable.c: - ExecUpdate: - else if (IsYBRelation(resultRelationDesc)): YB master 8bae488 updates the comment explaining what cols_marked_for_update does. YB pg15 be8504d moves that code to function YBExecUpdateAct. Apply in the new location. - if (updateCxt.crossPartUpdate): YB master 8bae488 frees the bitmapset cols_marked_for_update. YB pg15 be8504d moves the code into function YBExecUpdateAct. Apply in the new location. - ybOptimizeModifyTable.c: - YBAreDatumsStoredIdentically (formerly called YBEqualDatums): YB master b57e3c6 removes fcinfo logic that YB pg15 be8504d adjusts. Remove. - createplan.c: - declarations: - create_indexscan_plan: YB master 4ea354b moved this function declaration to planmain.h. Upstream PG changed indentation. Apply to new location. - create_bitmap_subplan: YB master 4ea354b removed some parameters; upstream PG changed indentation. - create_bitmap_subplan: YB master 4ea354b removed indexpushdownquals code. YB pg15 cd1df46 moved one of such code lower. Remove in the new location. - setrefs.c: - set_plan_refs: - case T_IndexScan: upstream PG 41efb8340877e8ffd0023bb6b2ef22ffd1ca014d adds param num_exec to fix_upper_expr and fix_scan_list. YB master 141703a adds a fix_scan_list function call and indents code containing fix_upper_expr function calls under an if case. - case T_YbBitmapIndexScan: (same but with YB master 3bf9301) - syscache.c: - yb_cache_table_name_table: YB master 9889df7 adds yb_cache_table_name_table to yb_catalog_cache_tables listing catalog tables that have catcaches. Upstream PG adds three new catalog tables and six new catcaches between 11.2 and 15.2. Add the new entries to these lists. Also, adjacent conflict with YB pg15 bea1ffb that removes pin cache code. - yb_ash.c: - yb_ash_ProcessUtility: YB pg15 f1c1a65 changes completionTag to qc. YB master e6337e4 touches various parts of this function. - yb_query_diagnostics.c: - includes: add "common/pg_prng.h" due to the YbQueryDiagnostics_ExecutorStart conflict below. - YbQueryDiagnosticsShmemInit: - LWTRANCHE_YB_QUERY_DIAGNOSTICS: YB master a9466df modifies LWLockRegisterTranche function call and renames variable yb_query_diagnostics_lock to bundles_in_progress_lock. YB pg15 602cb2d replaces LWLockRegisterTranche calls with an entry within the lwlock.c BuiltinTrancheNames array. Drop the LWLockRegisterTranche change; keep the variable rename. - LWTRANCHE_YB_QUERY_DIAGNOSTICS_CIRCULAR_BUFFER: YB master a9466df adds LWLockRegisterTranche function call. Upstream PG 29c3e2dd5a6aeaf1a23d7d83d665501e2dcc6955 replaces LWLockRegisterTranche calls with BuiltinTrancheNames array. Remove LWLockRegisterTranche call and instead add an entry into the lwlock.c BuiltinTrancheNames array similarly to what YB pg15 602cb2d did for LWTRANCHE_YB_QUERY_DIAGNOSTICS. - YbQueryDiagnostics_ExecutorStart: YB master 40689bc adds code involving random and MAX_RANDOM_VALUE. Upstream PG 3804539e48e794781c6145c7f988f5d507418fa8 replaces this pattern with pg_prng_double. Transform similarly. - DumpToFile: YB pg15 602cb2d adds extra param 0 to FileWrite function call. Adjacent conflict with YB master 9f8acff. - lwlock.c: - BuiltinTrancheNames: add entry for LWTRANCHE_YB_QUERY_DIAGNOSTICS_CIRCULAR_BUFFER (see yb_query_diagnostics.c) - pg_dump.c: - dumpCompositeType: adjacent conflict between YB master 516ead0 removing if case and upstream PG 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adding an extra argument to binary_upgrade_set_type_oids_by_type_oid. - dumpTableSchema: YB master 516ead0 makes a slight optimization on code that YB pg15 180e7ae removes. Remove. - dumpSequence: adjacent conflict between YB master 516ead0 removing if case and upstream PG f3faf35f370f558670c8213a08f2683f3811ffc7 replacing binary_upgrade_set_type_oids_by_rel_oid call with comment. - planmain.h: - create_indexscan_plan: (receive changes from createplan.c). Also rewrap. - yb_get_bitmap_index_quals: YB master 4ea354b adds this function. Adjacent conflict with upstream PG changing indentation. Reindent this function's declaration as well. - yb_ysql_dump.data.sql: (refer to BAR) - yb_ysql_dump_colocated_database.data.sql: (refer to BAR) - yb_ysql_dump_legacy_colocated_database.data.sql: (refer to BAR) - yb_ysql_dumpall.data.sql: (refer to BAR) - yb_pg_rules.out: - yb_wait_event_desc: YB master 1a3c113 adds output for the new view yb_wait_event_desc. YB pg15 946a8a9 adds TODO in same area. Adjacent conflict. - yb_pg_select.out: - "partial index implies clause, but bitmap scan must recheck predicate anyway": YB master 4ea354b mistakenly removed logic to account for quals implied by partial indexes, resulting in redundant storage filters being added. This is tracked by #23859. In this merge, during costsize.c merge resolution, this issue is fixed. So the redundant storage filters are only present on master. Modify the expected output to not expect storage filter. - where (unique2 = 11 and stringu1 < 'B') or unique1 = 0: YB pg15 2750e71 removes ybview wrapper on a query that YB master 4ea354b changes expected output of. That is no longer relevant without the ybview, so drop the incoming master change. - yb_reset_analyze.out: - "Grant permissions for this database.": adjacent conflict between YB master 58fd26e adding ALTER DATABASE SET queries and YB pg15 602cb2d grant access permission to the test users. - yb_pg_metrics.c: - enum statementType: YB master 6daf129 sets CatCacheIdMisses_End to CatCacheIdMisses_78. Adapt according to YB pg15 merge 602cb2d, accounting for 6 additional catcache entries. Furthermore, YB master 9889df7 adds a new section CatCacheTableMisses. Adapt it similarly for pg15 by adding three new catalog tables. - yb_pg_publication.out: - CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;: YB master 22b6a47 causes WARNING about wal_level not being "logical" to go away. This warning is only present in YB pg15 because YB pg15 5c5a2a0 was not able to suppress this WARNING for this one case (see that commit message for more details). - yb_publication.out: - CREATE PUBLICATION testpub FOR ALL TABLES;: (same) - yb_pg_pg_stat_statements.out: - Check WAL is generated for the above statements: "DROP TABLE pgss_test" row's wal column values change. Likely related to YB master 22b6a47. Not sure how exactly it relates, but since it makes the test closer to the original pg_stat_statements.out, take it without question. - yb_colocated_tables_with_tablespaces.{sql,out}: - Explicitly specifying pg_default tablespace must not create a new tablegroup: YB master 5eb925b adds these queries with SELECT * from system table, and upstream PG changes that to include the oid column. We don't want to show the oid column since that can easily change, so replace * with explicit column names. - typecmds.c: - AssignTypeMultirangeArrayOid: upstream PG 6df7a9698bb036610c1e8c6d375e1be38cb26d5f adds handling of multi-range data types. Adjust the logic in accordance with YB master 516ead0. (Note that another instance of this issue in AssignTypeMultirangeOid was already resolved separately by YB pg15 2e57f19.) - costsize.c: - yb_get_bitmap_index_quals: YB master 4ea354b copied the approach of create_bitmap_subplan to get index quals. ipath->indexquals was removed in YB pg15. Rewrite the BitmapIndexScan case, referencing YB pg15's create_bitmap_subplan. - postgres.c: - quickdie: YB master 4b4c201 adds quickdie as SIGTERM signal handler for pg_cron. quickdie is not built to expect any signal besides SIGQUIT. Adjust the logic to handle SIGTERM. - pgstat.c: - pgstat_get_wait_event_type: YB master 9f8acff removes IsYugaByteEnabled calls. Upstream PG a333476b925134f6185037eaff3424c07a9f466f moves this function to wait_event.c. Apply to the new location. - pgstat_get_wait_event: (same) - wait_event.c: (receive changes from pgstat.c) - relcache.c: - RelationSetNewRelfilenode: (resolve similarly to typecmds.c, new code added by upstream PG 4ab5dae9472c5c6ada5f5627ec0e0f9f965ade28) - syscache.h: - enum YbCatalogCacheTable: (see syscache.c explanation) - wait_states-itest.cc: - various tests: YB master f77dd6a and 45c9cf8 enabled tests to run on TSAN. Upstream PG b0b39f72b9904bcb80f97b35837ccff1578aa4b8 and 5599f40d259a275d3202b10aa6ffb9a36fd8a940 add pg_GSS_have_cred_cache which calls external function gss_acquire_cred from krb5 library. There seems to be data race in krb5 library when the tests try to create concurrent PG connections, don't enable the TSAN for these tests yet. - pg_ash-test.cc: (same) - pg_libpq-test.cc: - GetCatalogTableNameFromIndexName: YB master 9889df7 adds a test that has a list of all the PG catalog tables. Upstream PG adds three new catalog tables. Add the new tables to this list and, while we're at it, reformat it to have one table per line so this becomes easier to update in the future. - TestYsqlUpgrade.java: - dmlsUpdatePgCache: YB pg15 bea1ffb removes test pinnedObjectsCacheIsUpdated that YB master dcf1821 touches. Remove. - createPgTablegroupTableIfNotExists: YB pg15 4638d7d modifies the function. YB master dcf1821 moves it lower. Apply both. - class TableInfo: YB pg15 93f65c4 adds arrayTypeOid handling. YB master dcf1821 moves it higher. Apply both. FOO: YB master 516ead0 preserves some OIDs during ysql_dump. Upstream PG 9a974cbcba005256a19991203583a94b4f9a21a9 changes pg_dump to preserve relfilenodes in the same code path that was enabled by that master commit. Disable this part for YB. BAR: - instructions to preserve relfilenodes - PG upstream 9a974cbcba005256a19991203583a94b4f9a21a9 adds code to preserve relfilenodes - conflict about whether to preserve OIDs for sequences - YB master 516ead0 turns on output for setting pg_type OID for sequences - Upstream PG f3faf35f370f558670c8213a08f2683f3811ffc7 removes the code that attempts to preserve OIDs for sequences - this was resolved elsewhere in favor of postgres/removing the preservation - changes to the formatting of some values - YB pg15 ed96733 changes the formatting of some values - handling of dumping partitioned tables - YB pg15 180e7ae changes handling of partitioned tables, using attach instead of create - changes in database/ysql_dump versions - these are dumped in the output and thus changed Test Plan: Tests that fail: - integration-tests_basic_upgrade-test BasicUpgradeTest.*: these tests are expected to fail and will be fixed in pg15-upgrade branch. - JAVA org.yb.pgsql.TestYsqlMetrics: likely broken by YB master 8178372. It will be addressed in the future. - SHELL test_yb_bitmap_scans: fails on fastdebug gcc11, getting fixed on master (D38080) Jenkins: rebase: pg15 Reviewers: tfoucher Reviewed By: tfoucher Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D38054
timothy-e
added a commit
that referenced
this issue
Sep 17, 2024
…on partial indexes Summary: Original commit: a913524 / D37983 One of the things that `create_bitmap_subplan` does is generates a list of conditions that are guaranteed by the tree of Bitmap Ands, Bitmap Ors, and Bitmap Index Scans. This is used to determine the clauses that remain to be checked by the Bitmap Heap Scan / YB Bitmap Table Scan. 4ea354b / D36484 introduced `yb_get_bitmap_index_quals` that just generates the list of conditions. This makes the filter information available for use during the costing phase, before the actual plan nodes need to be constructed. `create_bitmap_subplan` accounted for the clauses of partial indexes. `yb_get_bitmap_index_quals` missed including this part, which caused the planner to believe the the conditions that are implied by the partial index conditions still need to be rechecked. For example, ```lang=sql create table t (a int, b int); create index idx_a_partial on t (a asc) where b < 10; create index idx_b_partial on t (b asc) where a < 10; /*+ Set(yb_enable_bitmapscan true) BitmapScan(t) */ explain select * from t where b < 10 and a < 10; ``` Generates a bitmap subplan that looks as follows: ``` -> Bitmap Index Scan on idx_b_partial (cost=0.00..0.92 rows=8 width=0) Index Cond: (b < 10) ``` We know that `b < 10` is satisfied (by index condition) and `a < 10` is satisfied (by partial index clause). However, `yb_get_bitmap_index_quals` did not report the partial index clause, so it told the Bitmap Table Scan that only `b < 10` was satisfied - requiring the Bitmap Table Scan to use a Storage Filter to check `a < 10`. ``` QUERY PLAN ------------------------------------------ YB Bitmap Table Scan on t Storage Filter: (a < 10) -> Bitmap Index Scan on idx_b_partial Index Cond: (b < 10) (4 rows) ``` With the change in this diff, `yb_get_bitmap_index_quals` reports both clauses, so the Bitmap Table Scan knows that both conditions are already met and it does not need to add a remote filter. ``` QUERY PLAN ------------------------------------------ YB Bitmap Table Scan on t -> Bitmap Index Scan on idx_b_partial Index Cond: (b < 10) (3 rows) ``` **How did this regression get in?** I noticed `yb_pg_select` was failing in D36484. I saw the problem was with Storage Filters but did not pay much attention to whether the storage filters were added or removed. The diff was able to remove some redundant storage filters from other tests, so I assumed it was doing the same here. Jira: DB-12766 Test Plan: ``` ybd --java-test 'org.yb.pgsql.TestPgRegressPgSelect#testPgRegressPgSelect' ``` Reviewers: mtakahara Reviewed By: mtakahara Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D38104
jasonyb
pushed a commit
that referenced
this issue
Sep 17, 2024
Summary: f97d7d5 [#23770] [#23797] [#23837] YSQL: Fix most non-regress tests when run with Connection Manager ee2b108 [docs] reverted PR 23909 changes (#23941) bd80f4e [#23924] DocDB: Address recent flakiness of PgGetLockStatusTest.TestGetWaitStart Excluded: f0a5db7 [#20908] YSQL: Introduce interface to optimize index non-key column updates 6556498 [#23897] xClusterDDLRepl: Support create table with partition by primary key e2b1d28 [#23363] Changing the default gflags of YSQL memory configuration. e717f43 [#23925] DocDB: Address recent regression of test PgWaitQueuesTest.MultiTabletFairness 09b7702 [#23940] YSQL: Replace copyright string from YugaByteDB to YugabyteDB add83ef [#23947] Update callhome URL to use https 69db717 Update faq page (#23704) Excluded: 063dbe5 [#23786] YSQL: yb_make_next_ddl_statement_nonincrementing Excluded: a913524 [#23859] YSQL: Remove redundant Bitmap Scan filters on partial indexes 41f5afd [PLAT-15097]: Delete System platform DB table while disabling ysql d6bbf59 [#23890] docdb: Add filtering for bootstrap intent iterators based on min_replay_txn_start_ht 0b37479 [#23770] YSQL: Stabalize TestPgExplainAnalyze#testExplainAnalyzeOptions the test with ysql connection manager Test Plan: Jenkins: rebase: pg15-cherrypicks Reviewers: jason, tfoucher Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D38123
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Jira Link: DB-12766
Description
Introduced by 4ea354b / D36484
Issue Type
kind/bug
Warning: Please confirm that this issue does not contain any sensitive information
The text was updated successfully, but these errors were encountered: