Skip to content

Commit 853a0af

Browse files
[BACKPORT 2024.2.7][#30104] YSQL: Use pg_index tuple as truth source for indpred and indexprs
Summary: ===Cherrypick Notes (2024.2 --> 2024.2.7)=== Clean cherrypick, no conflicts ===Backport Notes (2025.1 --> 2024.2)=== - Lint related conflicts in ybcplan.c. Resolve by keeping style as in 2024.2 - File pg_yb_index_check-test.cc not present in 2024.2. Resolve by adding file with boilerplate code needed to run test. ===Backport Notes (2025.2 --> 2025.1)=== Clean merge, no conflicts ===Backport Notes (master --> 2025.2)=== Clean merge, no conflicts ===Bug === Consider the following schema and data: ``` CREATE TABLE test (k INT PRIMARY KEY, v1 INT, v2 INT, boolcol BOOL); CREATE UNIQUE INDEX test_v1_idx ON test (v1) WHERE boolcol = true; INSERT INTO test VALUES (1, 1, 1, true); ``` The following query produces an index consistency on `test_v1_idx` when run in a separate connection (that has not queried table `test` yet): ``` INSERT INTO test VALUES (1, 1, 1, true) ON CONFLICT (k) DO UPDATE SET boolcol = false; ``` Output: ``` yugabyte=# INSERT INTO test VALUES (1, 1, 1, true) ON CONFLICT (k) DO UPDATE SET boolcol = false; INSERT 0 1 yugabyte=# SELECT yb_index_check('test_v1_idx'::regclass); ERROR: XX002: index contains spurious row DETAIL: index: 'test_v1_idx', ybbasectid: '\x47121048800000012121' ``` Since a table row corresponding to (k=1) exists, the above query executes the DO UPDATE clause which causes the index row corresponding to v1=1 to no longer satisfy the predicate. Therefore, it is expected that the the index row `v1=1` is deleted. However, this operation is incorrectly skipped, leading to a spurious row in the index. ===Cause === The Yugabyte planner has an optimization ([D34040](https://phorge.dev.yugabyte.com/D34040) / [63f471a](https://phorge.dev.yugabyte.com/rYBDB63f471a02c128ab8fb79b6f97a742cf0580cfb32))[1] to skip updating an index on `INSERT ... ON CONFLICT ... DO UPDATE` when it determines that the contents of the index row remain unmodified by the UPDATE. For partial indexes, this involves inspection of both the columns (key and non-key) in the index as well as its index predicate. This is done by reading various attributes in the index's `Relation` object. Vanilla postgres has a plan-time optimization[2] that skips the multi-step planning process for trivial INSERT queries. Such queries touch a single relation, and the values to be inserted are fetched from a Result scan. Such queries may optionally involve an ON CONFLICT clause. A side effect of this optimization is that the relation's catalog information (its attributes, indexes, constraints) are not loaded into the planner's data structures (RelOptInfo) which in turn causes the index's expression trees to not be loaded into the `rd_indpred` and `rd_indexprs` fields of index's `Relation`. Instead, these expression trees are lazily loaded during query execution. This lazily loading ensures that the second instance of the query in a given backend/connection will already have the expressions loaded at planning time. As a result of this postgres optimization [2], in this specific example, `rd_indpred` and `rd_indexprs` are not populated (pointer is NULL) when the planner inspects[1] the index for columns that are involved in the update. Since no other columns in the index are modified by the query, the planner[1] incorrectly concludes that the index is unmodified and skips updating it. The skipped updates lead to "spurious" or "missing" rows in case of partial indexes and "binary mismatch" in expression indexes. This bug is similar in nature to D47590 / 2b3e502. ===Fix=== This revision switches to using the pg_index tuple of the index as a source of truth; consulting (`pg_index.indpred`, `pg_index.indexprs`) instead of the cached `Relation.rd_indpred` and `Relation.rd_indexprs` fields. [1] - [ybplan.c --> func YbUpdateComputeIndexColumnReferences](https://github.com/yugabyte/yugabyte-db/blob/master/src/postgres/src/backend/optimizer/util/ybplan.c) [2] - [planmain.c --> func query_planner](https://github.com/yugabyte/yugabyte-db/blob/master/src/postgres/src/backend/optimizer/plan/planmain.c) Backport-to: 2025.2, 2025.1, 2024.2 Original commit: 51ffdaa / D49910 Test Plan: Run the following test: ``` ./yb_build.sh --cxx-test pgwrapper_pg_yb_index_check-test --gtest-filter 'PgYbIndexCheckTest.YbPartialExpressionIndexNewConnUpdate' ``` Reviewers: jason, sanketh, myang, #db-approvers Reviewed By: myang, #db-approvers Subscribers: mihnea, svc_phabricator, yql, smishra Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D50060
1 parent 63c7dc6 commit 853a0af

2 files changed

Lines changed: 85 additions & 17 deletions

File tree

src/postgres/src/backend/optimizer/util/ybcplan.c

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -881,27 +881,19 @@ YbUpdateComputeIndexColumnReferences(const Relation rel,
881881
* over the respective expression trees. It is possible that the
882882
* expression or predicate isn't applicable to this update, but the cost
883883
* of validating that will be prohibitively expensive.
884+
* Note: It is not guaranteed that indexDesc will contain the expression
885+
* trees at this point. Therefore, the NULL values of rd_indpred or
886+
* rd_indexprs cannot be relied upon. Instead, directly lookup the
887+
* pg_index entry of the index which should have been cached by now.
888+
* This is done in YbComputeIndexExprOrPredicateAttrs().
884889
*/
885890
Bitmapset *extraattrs = NULL;
886891

887-
/*
888-
* The index predicate and expressions, if any, are expected to be loaded
889-
* already at this point. We don't invoke RelationGetIndexPredicate() /
890-
* RelationGetIndexExpressions() here as these functions return a copy
891-
* of the expression tree which is overkill for the read-only
892-
* examination here.
893-
*/
894-
if (indexDesc->rd_indpred)
895-
{
896-
YbComputeIndexExprOrPredicateAttrs(
897-
&extraattrs, indexDesc, Anum_pg_index_indpred, offset);
898-
}
892+
YbComputeIndexExprOrPredicateAttrs(
893+
&extraattrs, indexDesc, Anum_pg_index_indpred, offset);
899894

900-
if (indexDesc->rd_indexprs)
901-
{
902-
YbComputeIndexExprOrPredicateAttrs(
903-
&extraattrs, indexDesc, Anum_pg_index_indexprs, offset);
904-
}
895+
YbComputeIndexExprOrPredicateAttrs(
896+
&extraattrs, indexDesc, Anum_pg_index_indexprs, offset);
905897

906898
if (extraattrs)
907899
{
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Copyright (c) YugaByte, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
4+
// in compliance with the License. You may obtain a copy of the License at
5+
//
6+
// http://www.apache.org/licenses/LICENSE-2.0
7+
//
8+
// Unless required by applicable law or agreed to in writing, software distributed under the License
9+
// is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
10+
// or implied. See the License for the specific language governing permissions and limitations
11+
// under the License.
12+
13+
#include <gtest/gtest.h>
14+
#include "yb/yql/pgwrapper/pg_mini_test_base.h"
15+
16+
namespace yb {
17+
namespace pgwrapper {
18+
19+
class PgYbIndexCheckTest : public PgMiniTestBase {
20+
};
21+
22+
TEST_F(PgYbIndexCheckTest, YbPartialExpressionIndexNewConnUpdate) {
23+
auto conn = ASSERT_RESULT(Connect());
24+
// Pre-existing connections to test the update of the partial and expression indexes.
25+
auto conn_partial1 = ASSERT_RESULT(Connect());
26+
auto conn_expr1 = ASSERT_RESULT(Connect());
27+
28+
// Create a table with two indexes: a partial and an expression index.
29+
ASSERT_OK(conn.Execute("CREATE TABLE test (id INT PRIMARY KEY, v1 INT, v2 INT, v3 BOOL )"));
30+
ASSERT_OK(conn.Execute("CREATE UNIQUE INDEX idx_partial ON test (v1) WHERE v3 = true"));
31+
ASSERT_OK(conn.Execute("CREATE INDEX idx_expr ON test ((v2 + 100))"));
32+
33+
// Insert two rows into the table, such that one satsifies the predicate, while the other doesn't.
34+
ASSERT_OK(conn.Execute("INSERT INTO test VALUES (1, 1, 1, true)"));
35+
ASSERT_OK(conn.Execute("INSERT INTO test VALUES (2, 2, 2, false)"));
36+
37+
// New connections to test the update of the partial and expression indexes.
38+
auto conn_partial2 = ASSERT_RESULT(Connect());
39+
auto conn_expr2 = ASSERT_RESULT(Connect());
40+
41+
// Flip the membership of the rows in the partial index.
42+
// (k=1): present --> absent
43+
// (k=2): absent --> present
44+
// Note that the primary key is used as the arbiter index to ensure that the planner doesn't load
45+
// the partial/expression index as part of evaluating the arbiter columns.
46+
ASSERT_OK(conn_partial1.Execute(
47+
"INSERT INTO test VALUES (1, 1, 1, true) ON CONFLICT (id) DO UPDATE SET v3 = false"));
48+
ASSERT_OK(conn_partial2.Execute(
49+
"INSERT INTO test VALUES (2, 2, 2, false) ON CONFLICT (id) DO UPDATE SET v3 = true"));
50+
51+
// Update the index rows of the expression index.
52+
// (k=1), v2: 1 + 100 --> 10 + 100
53+
// (k=2), v2: 2 + 100 --> 20 + 100
54+
ASSERT_OK(conn_expr1.Execute(
55+
"INSERT INTO test VALUES (1, 1, 1, true) ON CONFLICT (id) DO UPDATE SET v2 = 10"));
56+
ASSERT_OK(conn_expr2.Execute(
57+
"INSERT INTO test VALUES (2, 2, 2, false) ON CONFLICT (id) DO UPDATE SET v2 = 20"));
58+
59+
// Validate that the indexes are consistent.
60+
ASSERT_OK(conn.Fetch("SELECT yb_index_check('idx_partial'::regclass)"));
61+
ASSERT_OK(conn.Fetch("SELECT yb_index_check('idx_expr'::regclass)"));
62+
63+
// Verify the table data is correct
64+
auto expr1_result = ASSERT_RESULT(conn.FetchRow<int>("SELECT v2 FROM test WHERE v2 + 100 = 110"));
65+
auto expr2_result = ASSERT_RESULT(conn.FetchRow<int>("SELECT v2 FROM test WHERE v2 + 100 = 120"));
66+
ASSERT_EQ(expr1_result, 10);
67+
ASSERT_EQ(expr2_result, 20);
68+
69+
auto partial1_result = ASSERT_RESULT(conn.FetchRow<bool>("SELECT v3 FROM test WHERE id = 1"));
70+
auto partial2_result = ASSERT_RESULT(conn.FetchRow<bool>("SELECT v3 FROM test WHERE id = 2"));
71+
ASSERT_FALSE(partial1_result);
72+
ASSERT_TRUE(partial2_result);
73+
}
74+
75+
} // namespace pgwrapper
76+
} // namespace yb

0 commit comments

Comments
 (0)