Repository: pingcap/tispark (language: python)

Issue:
Do not reload table schema when update catalog cache
### What problem does this PR solve? <!--add issue link with summary if exists-->

TiSpark will reload all table schemas when init catalogCache. It will also reload all table schemas once we call listTable or getTable with the invalid cache.

One user reports that TiSpark hangs with more than 10000+ table schemas.

### What is changed and how it works?

Add a configuration `spark.tispark.load_tables` , default is true.
when set it to false, we will not load all tables when we reload the cache. table cache will only be loaded when the table is used.

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

 - I have tested it locally and it works.
 - User has tested and it works.



Discussion / hints:
[REVIEW NOTIFICATION]

This pull request has been approved by:

- xuanyu66
- zhangyangyu




To complete the [pull request process](https://book.prow.tidb.net/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.net/tichi/repos/pingcap/tispark/pulls/2667/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.net/tichi/repos/pingcap/tispark/pulls/2667/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=pingcap%2Ftispark).

<details>

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.
</details>

<!--Review Notification Identifier-->
@birdstorm PTAL It is a simple PR. 

I think we can just reload db cache when the schema version has changed. GetTable or ListTable method will find the table cache is empty, then just load all tables in its' own db rather than all dbs. These will reduce the pressure on users with a large number of tables.

I find you use relaod(true) which will reload all table scehmas in the following PR. I am afraid I miss something which needs it.
https://github.com/pingcap/tispark/pull/1623
https://github.com/pingcap/tispark/pull/2106

could you please take a look at this PR:

1. Is there anything I miss? Can I just reload without reloading the table cache?
2. Do you think we need a configuration to determine whether load tables cache?
> @birdstorm It is a simple PR. 
> 
> I think we can just reload db cache when the schema version has changed. GetTable or ListTable method will find the table cache is empty, then just load all tables in its' own db rather than all dbs. These will reduce the pressure on users with a large number of tables.
> 
> I find you use relaod(true) which will reload all table scehmas in the following PR. I am afraid I miss something which needs it.
> https://github.com/pingcap/tispark/pull/1623
> https://github.com/pingcap/tispark/pull/2106
> 
> could you please take a look at this PR:
> 
> 1. Is there anything I miss? Can I just reload without reloading the table cache?
> 2. Do you think we need a configuration to determine whether load tables cache?

@shiyuhang0 have you tested the case where there is rapid change in the schema version? e.g., Many tables are created and deleted at the same time, while the queries randomly occur on those tables? We added the function to reload catalog on tables because when we tested previously, it is possible for some of the queries to return “table not exist“ error even if the tables exist.
In theory, we do not need to reload tables every time because we should be able to resolve the ”table not exist” error in our error handler. It is just a reminder that the mentioned case should be tested and I will approve this PR considering that.
@birdstorm: Thanks for your review. The bot only counts approvals from reviewers and higher roles in [list](https://prow.tidb.net/tichi/repos/pingcap/tispark/pulls/2667/owners), but you're still welcome to leave your comments.

<details>

In response to [this](https://github.com/pingcap/tispark/pull/2667#pullrequestreview-1354277443):

>LGTM


Instructions for interacting with me using PR comments are available [here](https://prow.tidb.net/command-help).  If you have questions or suggestions related to my behavior, please file an issue against the [ti-community-infra/tichi](https://github.com/ti-community-infra/tichi/issues/new?title=Prow%20issue:) repository.
</details>
> > @birdstorm It is a simple PR.
> > I think we can just reload db cache when the schema version has changed. GetTable or ListTable method will find the table cache is empty, then just load all tables in its' own db rather than all dbs. These will reduce the pressure on users with a large number of tables.
> > I find you use relaod(true) which will reload all table scehmas in the following PR. I am afraid I miss something which needs it.
> > #1623
> > #2106
> > could you please take a look at this PR:
> > 
> > 1. Is there anything I miss? Can I just reload without reloading the table cache?
> > 2. Do you think we need a configuration to determine whether load tables cache?
> 
> @shiyuhang0 have you tested the case where there is rapid change in the schema version? e.g., Many tables are created and deleted at the same time, while the queries randomly occur on those tables? We added the function to reload catalog on tables because when we tested previously, it is possible for some of the queries to return “table not exist“ error even if the tables exist.

So, these PRs is aiming to fix the table cache miss (mostly because of the race I think). But, I can not find where is the race in the code :(
I think it is necessary to have a test. Is the  “table not exist“ easy to reproduce? I need to know if the race will not happen anymore or  it is just not be reproduced
> > > @birdstorm It is a simple PR.
> > > I think we can just reload db cache when the schema version has changed. GetTable or ListTable method will find the table cache is empty, then just load all tables in its' own db rather than all dbs. These will reduce the pressure on users with a large number of tables.
> > > I find you use relaod(true) which will reload all table scehmas in the following PR. I am afraid I miss something which needs it.
> > > #1623
> > > #2106
> > > could you please take a look at this PR:
> > > 
> > > 1. Is there anything I miss? Can I just reload without reloading the table cache?
> > > 2. Do you think we need a configuration to determine whether load tables cache?
> > 
> > @shiyuhang0 have you tested the case where there is rapid change in the schema version? e.g., Many tables are created and deleted at the same time, while the queries randomly occur on those tables? We added the function to reload catalog on tables because when we tested previously, it is possible for some of the queries to return “table not exist“ error even if the tables exist.
> 
> So, these PRs is aiming to fix the table cache miss. But, I can not find where is the race in the code :(
> I think it is necessary to have a test. Is the  “table not exist“ easy to reproduce? I need to know if the race will not happen anymore or  it is just not be reproduced

It should be easy to reproduce if the race still exists. After we have fixed the region cache issue, it is possible that the problem has been resolved since then.
In either case, I believe the case should be re-tested in this PR to ensure that.
/run-all-tests
/run-all-tests
/run-all-tests
/run-all-tests
/run-all-tests
/merge
This pull request has been accepted and is ready to merge. <details>Commit hash: d2b72970eb4225450c23f957b2bc921d9f9d6357</details>

Fix patch:
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 4175c9e2dc..c3fc8da69c 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -60,8 +60,11 @@ jobs:
 
     # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
     # If this step fails, then you should remove it and run the build manually (see below)
-    - name: Autobuild
-      uses: github/codeql-action/autobuild@v1
+#    - name: Autobuild
+#      uses: github/codeql-action/autobuild@v1
+    - name: build
+      run: |
+          mvn clean package -Dmaven.test.skip=true -B
 
     # ℹ️ Command-line programs to run using the OS shell.
     # 📚 https://git.io/JvXDl
diff --git a/core/src/main/scala/com/pingcap/tispark/TiConfigConst.scala b/core/src/main/scala/com/pingcap/tispark/TiConfigConst.scala
index a0eff484a2..00dafe06e6 100644
--- a/core/src/main/scala/com/pingcap/tispark/TiConfigConst.scala
+++ b/core/src/main/scala/com/pingcap/tispark/TiConfigConst.scala
@@ -85,4 +85,7 @@ object TiConfigConst {
   val DEFAULT_GC_MAX_WAIT_TIME: Long = 24 * 60 * 60
   val DEFAULT_GC_SAFE_POINT_TTL: Int = 5 * 60
 
+  // cache load
+  val LOAD_TABLES: String = "spark.tispark.load_tables"
+  val DEFAULT_LOAD_TABLES: Boolean = true
 }
diff --git a/core/src/main/scala/com/pingcap/tispark/utils/TiUtil.scala b/core/src/main/scala/com/pingcap/tispark/utils/TiUtil.scala
index 126ae160c4..6e64d4e941 100644
--- a/core/src/main/scala/com/pingcap/tispark/utils/TiUtil.scala
+++ b/core/src/main/scala/com/pingcap/tispark/utils/TiUtil.scala
@@ -237,6 +237,8 @@ object TiUtil {
       tiConf.setNewCollationEnable(conf.get(TiConfigConst.NEW_COLLATION_ENABLE).toBoolean)
     }
 
+    tiConf.setLoadTables(
+      conf.get(TiConfigConst.LOAD_TABLES, TiConfigConst.DEFAULT_LOAD_TABLES.toString).toBoolean)
     tiConf
   }
 
diff --git a/docs/userguide_3.0.md b/docs/userguide_3.0.md
index 4ab67e2130..dc603925f6 100644
--- a/docs/userguide_3.0.md
+++ b/docs/userguide_3.0.md
@@ -277,6 +277,7 @@ spark.sql("select t1.id,t2.id from spark_catalog.default.t t1 left join tidb_cat
 | `spark.tispark.replica_read.label`             | ""               | Only select TiKV store match specified labels. Format: label_x=value_x,label_y=value_y                                                                                                                                                                                                                                                                                                                                                            |
 | `spark.tispark.replica_read.address_whitelist` | ""               | Only select TiKV store with given ip addresses. Split mutil addresses by `,`                                                                                                                                                                                                                                                                                                                                                                      |
 | `spark.tispark.replica_read.address_blacklist` | ""               | Do not select TiKV store with given ip addresses. Split mutil addresses by `,`                                                                                                                                                                                                                                                                                                                                                                    |
+| `spark.tispark.load_tables`                     | true             | (experimental) Whether load all tables when we reload catalog cache. Disable it may cause table not find in scenarios where the table changes frequently.                                                                                                                                                                                                                                                                                           |
 
 ### TLS Configuration
 
@@ -385,13 +386,15 @@ TiSpark reads the range and hash partition table from TiDB.
 
 Currently, TiSpark doesn't support a MySQL/TiDB partition table syntax `select col_name from table_name partition(partition_name)`, but you can still use `where` condition to filter the partitions.
 
-TiSpark decides whether to apply partition pruning according to the partition type and the partition expression associated with the table. Currently, TiSpark partially apply partition pruning on range partition.
+TiSpark decides whether to apply partition pruning according to the partition type and the partition expression associated with the table.
+
+Currently, TiSpark partially apply partition pruning on range partition.
 
 The partition pruning is applied when the partition expression of the range partition is one of the following:
 
 + column expression
-+ `YEAR(col)` and its type is datetime/string/date literal that can be parsed as datetime.
-+ `TO_DAYS(col)` and its type is datetime/string/date literal that can be parsed as datetime.
++ `YEAR($argument)` where the argument is a column and its type is datetime or string literal
+  that can be parsed as datetime.
 
 If partition pruning is not applied, TiSpark's reading is equivalent to doing a table scan over all partitions.
 
diff --git a/tikv-client/src/main/java/com/pingcap/tikv/TiConfiguration.java b/tikv-client/src/main/java/com/pingcap/tikv/TiConfiguration.java
index 72fca98a06..3c77aee93b 100644
--- a/tikv-client/src/main/java/com/pingcap/tikv/TiConfiguration.java
+++ b/tikv-client/src/main/java/com/pingcap/tikv/TiConfiguration.java
@@ -48,6 +48,12 @@ public class TiConfiguration implements Serializable {
   private static final boolean DEF_IGNORE_TRUNCATE = true;
   private static final boolean DEF_TRUNCATE_AS_WARNING = false;
   private static final int DEF_MAX_FRAME_SIZE = 2147483647; // 2 GB
+  private boolean loadTables = true;
+
+  public boolean getLoadTables() {
+    return loadTables;
+  }
+
   private static final int DEF_INDEX_SCAN_BATCH_SIZE = 20000;
   private static final int DEF_REGION_SCAN_DOWNGRADE_THRESHOLD = 10000000;
   // if keyRange size per request exceeds this limit, the request might be too large to be accepted
diff --git a/tikv-client/src/main/java/com/pingcap/tikv/TiSession.java b/tikv-client/src/main/java/com/pingcap/tikv/TiSession.java
index e5da687399..e0ca05547e 100644
--- a/tikv-client/src/main/java/com/pingcap/tikv/TiSession.java
+++ b/tikv-client/src/main/java/com/pingcap/tikv/TiSession.java
@@ -197,9 +197,11 @@ public synchronized Catalog getOrCreateSnapShotCatalog(TiTimestamp ts) {
     if (snapshotCatalog == null) {
       snapshotCatalog =
           new Catalog(
-              this::createSnapshotWithSnapshotTimestamp, conf.isShowRowId(), conf.getDBPrefix());
+              this::createSnapshotWithSnapshotTimestamp,
+              conf.isShowRowId(),
+              conf.getDBPrefix(),
+              conf.getLoadTables());
     }
-    snapshotCatalog.reloadCache(true);
     return snapshotCatalog;
   }
 
@@ -208,7 +210,12 @@ public Catalog getCatalog() {
     if (res == null) {
       synchronized (this) {
         if (catalog == null) {
-          catalog = new Catalog(this::createSnapshot, conf.isShowRowId(), conf.getDBPrefix());
+          catalog =
+              new Catalog(
+                  this::createSnapshot,
+                  conf.isShowRowId(),
+                  conf.getDBPrefix(),
+                  conf.getLoadTables());
         }
         res = catalog;
       }
diff --git a/tikv-client/src/main/java/com/pingcap/tikv/catalog/Catalog.java b/tikv-client/src/main/java/com/pingcap/tikv/catalog/Catalog.java
index 723884f388..196ccac81b 100644
--- a/tikv-client/src/main/java/com/pingcap/tikv/catalog/Catalog.java
+++ b/tikv-client/src/main/java/com/pingcap/tikv/catalog/Catalog.java
@@ -41,13 +41,16 @@ public class Catalog implements AutoCloseable {
   private final Supplier<Snapshot> snapshotProvider;
   private CatalogCache metaCache;
   private static final AtomicLong lastUpdateTime = new AtomicLong(0);
+  private final boolean loadTables;
 
-  public Catalog(Supplier<Snapshot> snapshotProvider, boolean showRowId, String dbPrefix) {
+  public Catalog(
+      Supplier<Snapshot> snapshotProvider, boolean showRowId, String dbPrefix, boolean loadTables) {
     this.snapshotProvider = Objects.requireNonNull(snapshotProvider, "Snapshot Provider is null");
     this.showRowId = showRowId;
     this.dbPrefix = dbPrefix;
-    metaCache = new CatalogCache(new CatalogTransaction(snapshotProvider.get()), dbPrefix, false);
-    reloadCache(true);
+    this.loadTables = loadTables;
+    metaCache =
+        new CatalogCache(new CatalogTransaction(snapshotProvider.get()), dbPrefix, loadTables);
   }
 
   @Override
@@ -67,18 +70,14 @@ public void reloadCache(boolean loadTables) {
     }
   }
 
-  private void reloadCache() {
-    reloadCache(false);
-  }
-
   public List<TiDBInfo> listDatabases() {
-    reloadCache();
+    reloadCache(false);
     return metaCache.listDatabases();
   }
 
   public List<TiTableInfo> listTables(TiDBInfo database) {
     Objects.requireNonNull(database, "database is null");
-    reloadCache(true);
+    reloadCache(loadTables);
     if (showRowId) {
       return metaCache
           .listTables(database)
@@ -133,7 +132,7 @@ public TiTableInfo getTableFromCache(TiDBInfo database, String tableName) {
 
   public TiDBInfo getDatabase(String dbName) {
     Objects.requireNonNull(dbName, "dbName is null");
-    reloadCache();
+    reloadCache(false);
     return metaCache.getDatabase(dbName);
   }
 
@@ -148,7 +147,7 @@ public TiTableInfo getTable(String dbName, String tableName) {
   public TiTableInfo getTable(TiDBInfo database, String tableName) {
     Objects.requireNonNull(database, "database is null");
     Objects.requireNonNull(tableName, "tableName is null");
-    reloadCache(true);
+    reloadCache(loadTables);
     TiTableInfo table = metaCache.getTable(database, tableName);
     if (showRowId && table != null) {
       return table.copyTableWithRowId();


Test patch:
diff --git a/tikv-client/src/test/java/com/pingcap/tikv/catalog/CatalogTest.java b/tikv-client/src/test/java/com/pingcap/tikv/catalog/CatalogTest.java
index 3a520a2985..c8997216c0 100644
--- a/tikv-client/src/test/java/com/pingcap/tikv/catalog/CatalogTest.java
+++ b/tikv-client/src/test/java/com/pingcap/tikv/catalog/CatalogTest.java
@@ -70,8 +70,8 @@ public void listDatabasesTest() {
     helper.addDatabase(265, "other");
     helper.setSchemaVersion(667);
 
-    ReflectionWrapper wrapper = new ReflectionWrapper(cat);
-    wrapper.call("reloadCache");
+    ReflectionWrapper wrapper = new ReflectionWrapper(cat, boolean.class);
+    wrapper.call("reloadCache", false);
 
     dbs = cat.listDatabases();
     assertEquals(3, dbs.size());