Skip to content

Commit

Permalink
[BACKPORT 2.18][#22356] YSQL: make yb_get_range_split_clause robust u…
Browse files Browse the repository at this point in the history
…sing PG TRY CATCH block

Summary:
`yb_get_range_split_clause` is used by `ysql_dump` to generate YSQL table and index SPLIT AT VALUES clause for multi-tablet range-partitioned relation during backup.
It fails to decode `GinNull` in GIN Index partition bounds.
With the change in this diff, `yb_get_range_split_clause` checks for the existence of `GinNull` in partition bounds. If `GinNull` exists in partition bounds,
an empty string is returned by `yb_get_range_split_clause`.
Also, there might be other unknown cases where partition bounds decoding can fail, which blocks YB backup.
Thus, protect the decoding in `yb_get_range_split_clause` with PG TRY CATCH block in case of decoding failure for the sake of YB backup.
If decoding fails, a warning is thrown and an empty string is returned by `yb_get_range_split_clause`.
Each time `yb_get_range_split_clause` returns an empty string for a multi-tablet range relation, during restore, the relation is repartitioned with correct partition boundaries.
Jira: DB-11260

Original commit: 7fbebe9 / D34965
- pg_yb_utils.c getRangeSplitPointsList: D23931 is not in branch 2.18, so simple change made to this function is not included in this backport.
- yb-backup-cross-feature-test.cc TestYSQLTabletSplitGINIndexWithGinNullInPartitionBounds: Since the test utility function `YBBackupTest::GetSplitPoints` is not in branch 2.18, tablet's partition key check logic in this test is slightly rewritten.
- ybc_pggate.cc GetSplitPoints: D25290 is not in branch 2.18, so `GinNull` existence check uses KeyEntryValue decoded from partition bounds instead.

Test Plan:
Jenkins: urgent
./yb_build.sh --cxx-test yb-backup-cross-feature-test --gtest_filter YBBackupTest.TestYSQLTabletSplitGINIndexWithGinNullInPartitionBounds

Reviewers: jason, mihnea

Reviewed By: jason

Subscribers: yql, ybase, fizaa

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D35064
  • Loading branch information
yifanguan committed May 15, 2024
1 parent d5491d9 commit e34a721
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 9 deletions.
59 changes: 54 additions & 5 deletions src/postgres/src/backend/utils/misc/pg_yb_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -2349,7 +2349,7 @@ getSplitPointsInfo(Oid relid, YBCPgTableDesc yb_tabledesc,
YbTableProperties yb_table_properties,
Oid *pkeys_atttypid,
YBCPgSplitDatum *split_datums,
bool *has_null)
bool *has_null, bool *has_gin_null)
{
Assert(yb_table_properties->num_tablets > 1);

Expand Down Expand Up @@ -2389,7 +2389,8 @@ getSplitPointsInfo(Oid relid, YBCPgTableDesc yb_tabledesc,

/* Get Split point values as Postgres datums */
HandleYBStatus(YBCGetSplitPoints(yb_tabledesc, type_entities,
type_attrs_arr, split_datums, has_null));
type_attrs_arr, split_datums, has_null,
has_gin_null));
}

/*
Expand All @@ -2409,10 +2410,11 @@ rangeSplitClause(Oid relid, YBCPgTableDesc yb_tabledesc,
StringInfo prev_split_point = makeStringInfo();
StringInfo cur_split_point = makeStringInfo();
bool has_null = false;
bool has_gin_null = false;

/* Get Split point values as Postgres datum */
getSplitPointsInfo(relid, yb_tabledesc, yb_table_properties, pkeys_atttypid,
split_datums, &has_null);
split_datums, &has_null, &has_gin_null);

/*
* Check for existence of NULL in split points.
Expand All @@ -2431,6 +2433,24 @@ rangeSplitClause(Oid relid, YBCPgTableDesc yb_tabledesc,
return;
}

/*
* Check for existence of GinNull in split points.
* We don't support (1) decoding GinNull into Postgres datum and
* (2) specify GinNull in SPLIT AT VALUES clause for both
* CREATE TABLE and CREATE INDEX.
* However, split points of GIN indexes generated by tablet splitting can have
* GinNull in its split points.
*/
if (has_gin_null)
{
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("GinNull value present in split points"),
errdetail("Specifying GinNull value in SPLIT AT VALUES clause is "
"not supported.")));
return;
}

/* Process Datum and use StringInfo to accumulate c-string data */
appendStringInfoString(str, "SPLIT AT VALUES (");
for (int split_idx = 0; split_idx < num_splits; ++split_idx)
Expand Down Expand Up @@ -2526,10 +2546,39 @@ yb_get_range_split_clause(PG_FUNCTION_ARGS)
* Get SPLIT AT VALUES clause for range relations with more than one tablet.
* Skip one-tablet range-partition relations such that this function
* return an empty string for them.
*
* For YB backup, if an error is thrown from a PG backend, ysql_dump will
* exit, generate an empty YSQLDUMP file, and block YB backup workflow.
* Currently, we don't have the functionality to adjust options used for
* ysql_dump on YBA and YBM, so we don't have a way to to turn on/off
* a backup-related feature used in ysql_dump.
* There are known cases which caused decoding of split points to fail in
* the past and are handled specifically now.
* (1) null values appear in split points after tablet splitting
* (2) GinNull values appear in split points after tablet splitting
* (3) duplicate split points appear after tablet splitting on an
* index's hidden column
* Thus, for the sake of YB backup, protect the split point decoding with
* TRY CATCH block in case decoding fails due to other unknown cases.
* Return an empty string if decoding fails.
*/
initStringInfo(&str);
if (yb_table_properties.num_tablets > 1)
rangeSplitClause(relid, yb_tabledesc, &yb_table_properties, &str);
PG_TRY();
{
if (yb_table_properties.num_tablets > 1)
rangeSplitClause(relid, yb_tabledesc, &yb_table_properties, &str);
}
PG_CATCH();
{
ereport(WARNING,
(errcode(ERRCODE_WARNING),
errmsg("cannot decode split point in SPLIT AT VALUES clause "
"of relation with oid %u", relid),
errdetail("Returning an empty string instead.")));
/* Empty string if split point decoding fails. */
resetStringInfo(&str);
}
PG_END_TRY();
range_split_clause = str.data;

PG_RETURN_CSTRING(range_split_clause);
Expand Down
88 changes: 88 additions & 0 deletions src/yb/tools/yb-backup/yb-backup-cross-feature-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,94 @@ TEST_F_EX(YBBackupTest,
LOG(INFO) << "Test finished: " << CURRENT_TEST_CASE_AND_TEST_NAME_STR();
}

// Test backup/restore when a range-partitioned GIN index undergoes manual tablet splitting and
// GinNullItem become part of its tablets' partition bounds.
// Any kind of GinNull (GinNullKey, GinEmptyItem, and GinNullItem) can be used for this test.
// In the case where GinNull is part of one partition bound of a GIN index, during backup,
// yb_get_range_split_clause fails to decode the partition bound, and YSQL_DUMP doesn't dump the
// SPLIT AT clause of the index.
// During restore, restoring snapshot to the index with different partition boundaries
// should be detected and handled by repartitioning the index. See CatalogManager::RepartitionTable.
// This test exercises that:
// 1. create a table and a GIN index
// 2. insert NULL data into the table
// 3. split the GIN index into 2 tablets to make GinNull become part of partition bounds
// 4. backup
// 5. drop table
// 6. restore
TEST_F_EX(YBBackupTest,
YB_DISABLE_TEST_IN_SANITIZERS(TestYSQLTabletSplitGINIndexWithGinNullInPartitionBounds),
YBBackupTestNumTablets) {
const string table_name = "mytbl";
const string index_name = "my_gin_idx";

// Create table and index
ASSERT_NO_FATALS(CreateTable(Format("CREATE TABLE $0 (v tsvector)", table_name)));
ASSERT_NO_FATALS(CreateIndex(Format("CREATE INDEX $0 ON $1 USING ybgin(v)",
index_name, table_name)));

// Verify the index has only one tablet
auto tablets = ASSERT_RESULT(test_admin_client_->GetTabletLocations(default_db_, index_name));
LogTabletsInfo(tablets);
ASSERT_EQ(tablets.size(), 1);
ASSERT_TRUE(CheckPartitions(tablets, {}));

// Insert data
const string insert_null_sql = Format(R"#(
DO $$$$
BEGIN
FOR i in 1..1000 LOOP
INSERT INTO $0 VALUES (NULL);
END LOOP;
END $$$$;
)#", table_name);
ASSERT_NO_FATALS(RunPsqlCommand(insert_null_sql, "DO"));

// Flush index
auto index_id = ASSERT_RESULT(GetTableId(index_name, "pre-split"));
ASSERT_OK(client_->FlushTables({index_id}, false, 30, false));

// Split the GIN index into two tablets and wait for its split to complete.
// The splits make GinNull become part of its tablets' partition bounds:
// tablet-1 boundaries: [ "", (GinNullItem, <ybctid>) )
// tablet-2 boundaries: [ (GinNullItem, <ybctid>), "" )
const auto num_tablets = 2;
ASSERT_OK(test_admin_client_->SplitTabletAndWait(
default_db_, index_name, /* wait_for_parent_deletion */ true, tablets[0].tablet_id()));

// Verify that it has two tablets:
tablets = ASSERT_RESULT(test_admin_client_->GetTabletLocations(default_db_, index_name));
LogTabletsInfo(tablets);
ASSERT_EQ(tablets.size(), num_tablets);

// Verify GinNull is in the split point.
// 'v' represents the KeyEntryType for kGinNull.
auto split_point = tablets[0].partition().partition_key_end();
ASSERT_EQ(split_point[0], 'v');

// Backup
const string backup_dir = GetTempDir("backup");
ASSERT_OK(RunBackupCommand(
{"--backup_location", backup_dir, "--keyspace", "ysql.yugabyte", "create"}));

// Drop the table
ASSERT_NO_FATALS(RunPsqlCommand(Format("DROP TABLE $0", table_name), "DROP TABLE"));

// Restore
ASSERT_OK(RunBackupCommand({"--backup_location", backup_dir, "restore"}));

// Validate
tablets = ASSERT_RESULT(test_admin_client_->GetTabletLocations(default_db_, index_name));
ASSERT_EQ(tablets.size(), 2);
auto post_restore_split_point = tablets[0].partition().partition_key_end();
// Rely on CatalogManager::RepartitionTable to repartition the GIN index with correct partition
// boundaries. Validate if repartition works correctly.
ASSERT_EQ(split_point,
post_restore_split_point);

LOG(INFO) << "Test finished: " << CURRENT_TEST_CASE_AND_TEST_NAME_STR();
}

class YBBackupAfterFailedMatviewRefresh : public YBBackupTest {
public:
void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
Expand Down
9 changes: 6 additions & 3 deletions src/yb/yql/pggate/ybc_pggate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ static Status GetSplitPoints(YBCPgTableDesc table_desc,
const YBCPgTypeEntity **type_entities,
YBCPgTypeAttrs *type_attrs_arr,
YBCPgSplitDatum *split_datums,
bool *has_null) {
bool *has_null, bool *has_gin_null) {
CHECK(table_desc->IsRangePartitioned());
const Schema& schema = table_desc->schema();
const auto& column_ids = table_desc->partition_schema().range_schema().column_ids;
Expand All @@ -731,6 +731,9 @@ static Status GetSplitPoints(YBCPgTableDesc table_desc,
CHECK(v.type() == docdb::KeyEntryType::kHighest);
split_datums[split_datum_idx].datum_kind = YB_YQL_DATUM_LIMIT_MAX;
}
} else if (v.type() == docdb::KeyEntryType::kGinNull) {
*has_gin_null = true;
return Status::OK();
} else {
CHECK(!docdb::IsSpecialKeyEntryType(v.type()));
split_datums[split_datum_idx].datum_kind = YB_YQL_DATUM_STANDARD_VALUE;
Expand Down Expand Up @@ -768,9 +771,9 @@ YBCStatus YBCGetSplitPoints(YBCPgTableDesc table_desc,
const YBCPgTypeEntity **type_entities,
YBCPgTypeAttrs *type_attrs_arr,
YBCPgSplitDatum *split_datums,
bool *has_null) {
bool *has_null, bool *has_gin_null) {
return ToYBCStatus(GetSplitPoints(table_desc, type_entities, type_attrs_arr, split_datums,
has_null));
has_null, has_gin_null));
}

YBCStatus YBCPgTableExists(const YBCPgOid database_oid,
Expand Down
2 changes: 1 addition & 1 deletion src/yb/yql/pggate/ybc_pggate.h
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ YBCStatus YBCGetSplitPoints(YBCPgTableDesc table_desc,
const YBCPgTypeEntity **type_entities,
YBCPgTypeAttrs *type_attrs_arr,
YBCPgSplitDatum *split_points,
bool *has_null);
bool *has_null, bool *has_gin_null);

// INDEX -------------------------------------------------------------------------------------------
// Create and drop index "database_name.schema_name.index_name()".
Expand Down

0 comments on commit e34a721

Please sign in to comment.