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] The condition of "switching to per-db mode" should be refined #19182

Closed
1 task done
myang2021 opened this issue Sep 18, 2023 · 0 comments
Closed
1 task done

[YSQL] The condition of "switching to per-db mode" should be refined #19182

myang2021 opened this issue Sep 18, 2023 · 0 comments
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue

Comments

@myang2021
Copy link
Contributor

myang2021 commented Sep 18, 2023

Jira Link: DB-7979

Description

In recent debugging of GHI issue #18711 I found an issue inside function YBIsDBCatalogVersionMode(). During connection establishment time, the following events happen in order:

  1. RelationCacheInitialize()
  2. YbGetCatalogCacheVersionForTablePrefetching()
        /*
         * If per database catalog version mode is enabled, this will load the
         * catalog version of template1. It is fine because at this time we
         * only read shared relations and therefore can use any database OID.
         * We will update yb_catalog_cache_version to match MyDatabaseId once
         * the latter is resolved so we will never use the catalog version of
         * template1 to query relations that are private to MyDatabaseId.
         */
        YbUpdateCatalogCacheVersion(YbGetMasterCatalogVersion());
  1. RelationCacheInitializePhase2()
  2. MyDatabaseId is resolved
    if (MyDatabaseId != TemplateDbOid && YBIsDBCatalogVersionMode())
    {
        /*
         * Here we assume that the entire table pg_yb_catalog_version is
         * prefetched. Note that in this case YbGetMasterCatalogVersion()
         * returns the prefetched catalog version of MyDatabaseId which is
         * consistent with all the other tables that are prefetched.
         */
        uint64_t master_catalog_version = YbGetMasterCatalogVersion();
        Assert(master_catalog_version > YB_CATCACHE_VERSION_UNINITIALIZED);
        YbUpdateCatalogCacheVersion(master_catalog_version);
    }

The first call to YBIsDBCatalogVersionMode() is during connection setup is made in step 2 by YbGetCatalogCacheVersionForTablePrefetching(). If the cluster is already running in per-database catalog version mode, each connection still goes through the following code

        yb_last_known_catalog_cache_version = 1;
        YbUpdateCatalogCacheVersion(1);
        elog(DEBUG3, "switching to per-db mode");

This isn't needed if YBIsDBCatalogVersionMode() is called before step 6, because the version 1 set here will be overwritten by the call to YbUpdateCatalogCacheVersion in step 6. This is similar to a useless assignment that can be optimized away. Secondly, for template1, we should not reset catalog version to 1 because the upgrade script does not change the catalog version of template1. We do have internally generated connections to template1 and a user can also make an explicit connection to template1. In a connection to template1, if we reset template1's catalog version to 1, it can cause unnecessary catalog cache refresh.

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

  • I confirm this issue does not contain any sensitive information.
@myang2021 myang2021 added kind/enhancement This is an enhancement of an existing feature area/ysql Yugabyte SQL (YSQL) labels Sep 18, 2023
@myang2021 myang2021 self-assigned this Sep 18, 2023
@yugabyte-ci yugabyte-ci added the priority/medium Medium priority issue label Sep 18, 2023
myang2021 added a commit that referenced this issue Sep 21, 2023
Summary:
In recent debugging of GHI 18711, I found an issue inside function
`YBIsDBCatalogVersionMode()`. During normal connection setup time, the
following events happen in order:
(1) RelationCacheInitialize()
(2) YbGetCatalogCacheVersionForTablePrefetching()
(3)
```
        /*
         * If per database catalog version mode is enabled, this will load the
         * catalog version of template1. It is fine because at this time we
         * only read shared relations and therefore can use any database OID.
         * We will update yb_catalog_cache_version to match MyDatabaseId once
         * the latter is resolved so we will never use the catalog version of
         * template1 to query relations that are private to MyDatabaseId.
         */
        YbUpdateCatalogCacheVersion(YbGetMasterCatalogVersion());
```

(4) RelationCacheInitializePhase2()
(5) MyDatabaseId is resolved
(6)
```
    if (MyDatabaseId != TemplateDbOid && YBIsDBCatalogVersionMode())
    {
        /*
         * Here we assume that the entire table pg_yb_catalog_version is
         * prefetched. Note that in this case YbGetMasterCatalogVersion()
         * returns the prefetched catalog version of MyDatabaseId which is
         * consistent with all the other tables that are prefetched.
         */
        uint64_t master_catalog_version = YbGetMasterCatalogVersion();
        Assert(master_catalog_version > YB_CATCACHE_VERSION_UNINITIALIZED);
        YbUpdateCatalogCacheVersion(master_catalog_version);
    }
```

During normal connection setup (i.e., non-initdb), the first call to
`YBIsDBCatalogVersionMode()` is made in step (2) by
`YbGetCatalogCacheVersionForTablePrefetching()`. If the cluster is already
running in per-database catalog version mode, each connection still goes through
the following code that appears inside `YBIsDBCatalogVersionMode()`:

```
        yb_last_known_catalog_cache_version = 1;
        YbUpdateCatalogCacheVersion(1);
        elog(DEBUG3, "switching to per-db mode");
```

First, for template1, we should not reset catalog version to 1 because the
upgrade of the table pg_yb_catalog_version_table to per-database catalog version
mode does not change the catalog version of template1. We do have internally
generated connections to template1 and a user can also make an explicit
connection to template1. In a connection to template1, if we reset template1's
catalog version to 1, it can cause unnecessary catalog cache refresh.

Second, setting catalog version to 1 is not needed if
`YBIsDBCatalogVersionMode()` is called before step (6), because the version 1
set by this code block will be overwritten by the call to
`YbUpdateCatalogCacheVersion` in step (6). This is similar to a useless
assignment that can be optimized away.

I refined the condition of "switching to per-db mode" code block. If the cluster
is already running in per-database catalog version mode, we will not see the log
"switching to per-db mode" because the first call to `YBIsDBCatalogVersionMode()`
is made when MyDatabaseId is still `InvalidOid`. Therefore "switching to per-db
mode" is now more accurate and the log level is changed from DEBUG3 to INFO.
Jira: DB-7979

Test Plan:
1. ./yb_build.sh debug --cxx-test pg_catalog_version-test
2. YB_EXTRA_DAEMON_FLAGS="--ysql_enable_read_request_caching=true" ./yb_build.sh debug --cxx-test pg_catalog_version-test

Reviewers: jason

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D28623
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

2 participants