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] T-server catalog cache serves stale version of pg_database #19363

Closed
1 task done
kai-franz opened this issue Sep 29, 2023 · 1 comment
Closed
1 task done

[YSQL] T-server catalog cache serves stale version of pg_database #19363

kai-franz opened this issue Sep 29, 2023 · 1 comment
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue

Comments

@kai-franz
Copy link
Contributor

kai-franz commented Sep 29, 2023

Jira Link: DB-8163

Description

When a Postgres backend starts, it prefetches these 3 shared tables:

        YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_AUTH_MEMBERS);
	YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_DATABASE);
	YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_DB_ROLE_SETTINGS);

When the T-server cache is enabled, this request is cached there. When another backend process starts up, it will issue the same prefetch request, which will result in a hit in the T-server cache (assuming the catalog version has not changed).

This becomes a problem in the following scenario:

  1. Backend 1 is started and it connects to database db1.
  2. Backend 1 prefetches the above shared tables. This request is cached in the T-server cache.
  3. Backend 1 executes CREATE DATABASE db2, which does not increment the catalog version.
  4. Backend 2 is started and it connects to database db2.
  5. Backend 2 prefetches the shared tables, hitting the T-server cache. Since the cached request was made before db2 was created, backend 2 now has a stale version of pg_database.
  6. Backend 2 tries to CheckMyDatabase and fails, because it does not see db2 in the pg_database table.

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

  • I confirm this issue does not contain any sensitive information.
@kai-franz kai-franz added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Sep 29, 2023
@yugabyte-ci yugabyte-ci added kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue labels Sep 29, 2023
@kai-franz kai-franz self-assigned this Sep 29, 2023
kai-franz added a commit that referenced this issue Oct 13, 2023
Summary:
Adds MyDatabaseId to the T-server cache key to address an issue caused by different databases sharing T-server cache entries.

When a Postgres backend starts, it prefetches these 3 shared tables:
```
        YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_AUTH_MEMBERS);
	YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_DATABASE);
	YbRegisterTable(prefetcher, YB_PFETCH_TABLE_PG_DB_ROLE_SETTINGS);
```
These tables are then cached in the T-server cache, which is keyed by the database OID and the OIDs of these tables. Because these are shared tables, the OID of the template1 database is used. As a result, when another backend process starts up, it will issue the same prefetch request, which will result in a hit in the T-server cache (assuming the catalog version has not changed).

Here is how the issue manifests in detail by @tverona, (requires D28071, ysql_enable_read_request_caching=true):

### 1. Start with just `yugabytedb`. Connect to `yugabytedb` for the first time.
   * **a.** Relcacheinit file is built. So we preload a bunch of tables from master.
   * **b.** Create tserver cache entry for those tables (which includes `pg_database`). Key contains `yugabytedb` oid (since that’s part of the request).
   * **c.** Create `db1`.

### 2. Connect to `db1` for the first time.
   * **a.** Same flow as #1 above - we create a new relcacheinit file for `db1`.
   * **b.** We create another tserver cache entry (might be more than one, but just simplifying) with key containing `db1` oid.

### 3. Connect to `db1` for the 2nd time.
   * **a. With D28071:**
     - **i.** Relcache file is not built, since cache is not invalidated.
     - **ii.** We fetch the 3 tables (including `pg_database`) from master and create a new tserver cache entry, with key include `template0` dbid(?). Values include `db1`.
   * **b. Without D28071:**
     - **i.** We preload a bunch of tables for `db1`. We match on tserver cache entry from 2.b. We do not hit master.
   * **c.** Create `db2`.

### 4. Connect to `db2` for the first time.
   * **a.** Same flow as #1 above - we create a new relcacheinit file for `db2`.
   * **b.** We create another tserver cache entry (might be more than one, but just simplifying) with key containing `db2` oid.

### 5. Connect to `db2` for the 2nd time.
   * **a. With D28071:**
     - **i.** Relcache file is not built, since cache is not invalidated.
     - **ii.** We fetch the 3 tables (including `pg_database`) from master and match on the key in 3.a.ii. We do not hit master. We get back `pg_database` containing entries for `db1` but not `db2`.
     - **iii.** We fail later in `CheckMyDatabase`.
   * **b. Without the diff:**
     - **i.** We preload a bunch of tables for `db2`. We match on tserver cache entry from 4.b. We do not hit master.

By always including MyDatabaseId in the cache key, we avoid serving stale versions of shared relations to different databases.

**Upgrade/Rollback safety:**
Only PG to T-Server RPCs are changed.
Jira: DB-8163

Test Plan:
  # Connect to yugabyte
  # Connect to yugabyte
    # Create db1
  # Connect to db1
  # Connect to db1 <-- fails before this change with D28071, ysql_enable_read_request_caching=true

Reviewers: myang, dmitry

Reviewed By: dmitry

Subscribers: ybase, yql, tverona

Differential Revision: https://phorge.dev.yugabyte.com/D28945
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Oct 16, 2023
@tverona1
Copy link
Contributor

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

3 participants