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

Speedup query to do ApiKey grants #15927

Merged

Conversation

amiedes
Copy link
Contributor

@amiedes amiedes commented Nov 12, 2020

Related: https://app.clubhouse.io/cartoteam/story/117080/performance-issues-with-datasets-r-scope

What does this PR do?

Previously, when generating the GRANTs associated to an API key, we were doing a query similar to the following to retrieve the sequences associated with each of the tables the API key had access to:

SELECT
  n.nspname, c.relname
FROM
  pg_depend d
  JOIN pg_class c ON d.objid = c.oid
  JOIN pg_namespace n ON c.relnamespace = n.oid
WHERE
  d.refobjsubid > 0 AND
  d.classid = 'pg_class'::regclass AND
  c.relkind = 'S'::"char" AND
  d.refobjid IN (
    (QUOTE_IDENT('amiedes') || '.' || QUOTE_IDENT('africa_adm0'))::regclass
  );

For an API key with access to 150 tables, this query took ~200ms. This means for the 150 tables it was around 35 seconds.

This PR combines all the queries to retrieve the list of sequences to give the GRANTs in a single query, so it is faster:

SELECT
  n.nspname, c.relname
FROM
  pg_depend d
  JOIN pg_class c ON d.objid = c.oid
  JOIN pg_namespace n ON c.relnamespace = n.oid
WHERE
  d.refobjsubid > 0 AND
  d.classid = 'pg_class'::regclass AND
  c.relkind = 'S'::"char" AND
  d.refobjid IN (
    (QUOTE_IDENT('amiedes') || '.' || QUOTE_IDENT('africa_adm0'))::regclass,
    (QUOTE_IDENT('amiedes') || '.' || QUOTE_IDENT('air_quality_data_2017'))::regclass,
    (QUOTE_IDENT('amiedes') || '.' || QUOTE_IDENT('air_quality_stations_2017'))::regclass,lass
  );

Now the same query takes ~2 seconds.

@amiedes amiedes force-pushed the bug/ch117080/performance-issues-with-datasets-r-scope branch 3 times, most recently from 103a84b to 2dd2796 Compare November 12, 2020 15:22
@amiedes amiedes force-pushed the bug/ch117080/performance-issues-with-datasets-r-scope branch from 2dd2796 to 9f0f892 Compare December 9, 2020 08:03
end
end

user.db_service.sequences_for_tables(tables).each do |sequence|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old sequences_for_table(schema, table_name) did N queries to retrieve all the sequences for all the tables. The new user.db_service.sequences_for_tables(tables) does the same but in a single query.

@@ -585,26 +591,6 @@ def remove_from_redis(key = redis_key)
redis_client.del(key)
end

def sequences_for_table(schema, table)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new version of this (sequences_for_tables) has been moved to the DB service.


describe '#public_user_roles' do
subject { db_service.public_user_roles }
subject(:public_user_roles) { db_service.public_user_roles }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are mostly linter fixes, the important part is under describe '#sequences_for_tables' do

@amiedes amiedes requested a review from thedae December 9, 2020 08:10
@amiedes amiedes marked this pull request as ready for review December 9, 2020 08:11
Copy link
Contributor

@thedae thedae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@amiedes
Copy link
Contributor Author

amiedes commented Dec 9, 2020

Acceptance in staging

API key creation continues to work as expected:

{
    "id": "75f6c827-6933-4b4f-bf9f-78e29aad4a2e",
    "token": "JWTDtOqbc_d3IogeGIY4wQ",
    "user_id": "8c8ec7bb-29a8-432c-90ee-5d2a1ae472de",
    "type": "regular",
    "name": "Test",
    "db_role": "carto_role_94bb1a0408a21405ba67d88fbb21019b",
    "db_password": "2063f7789c301dc4cc981c560ce0870a992eafdd",
    "grants": [
      {
        "type": "apis",
        "apis": [
          "maps",
          "sql"
        ]
      },
      {
        "type": "database",
        "tables": [
          {
            "name": "dataset_ciudades_2",
            "schema": "amiedes-sequences1",
            "permissions": [
              "select",
              "update",
              "insert",
              "delete"
            ]
          },
          {
            "name": "dataset_ciudades",
            "schema": "amiedes-sequences1",
            "permissions": [
              "select",
              "update",
              "insert",
              "delete"
            ]
          },
          {
            "name": "dataset_ciudades_1",
            "schema": "amiedes-sequences1",
            "permissions": [
              "select",
              "update",
              "insert",
              "delete"
            ]
          }
        ],
        "schemas": [

        ]
      }
    ],
    "created_at": "2020-12-09 09:53:51 UTC",
    "updated_at": "2020-12-09 09:53:51 UTC"
  }

Will do more performance testing in production with the real application.

@amiedes amiedes merged commit 9c0ae27 into master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants