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

sql: improve REGPROCEDURE cast performance #61082

Closed
mgartner opened this issue Feb 24, 2021 · 3 comments · Fixed by #61211
Closed

sql: improve REGPROCEDURE cast performance #61082

mgartner opened this issue Feb 24, 2021 · 3 comments · Fixed by #61211
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@mgartner
Copy link
Collaborator

ActiveRecord executes a query on the pg_type table when initiating a connection (full query below). This query has a filter with a ::REGPROCEDURE cast which is extremely slow to execute. Ideally we can improve the performance of this cast. In cockroach demo this cast adds a few hundred milliseconds of latency to the query. In a multi-node cluster, it adds 2+ seconds of latency.

Example (in cockroach demo).

defaultdb> SELECT count(*) FROM pg_type;
  count
---------
     75
(1 row)

Time: 2ms total (execution 2ms / network 0ms)

defaultdb> SELECT count(*) FROM pg_type WHERE typinput = 'array_in(cstring,oid,integer)'::REGPROCEDURE;
  count
---------
     36
(1 row)

Time: 528ms total (execution 527ms / network 0ms)

Complete ActiveRecord query

https://github.com/rails/rails/blob/b038bb27a6162184faef3af6e1c676a893651d9f/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L633

SELECT
  t.oid, t.typname, t.typelem, t.typdelim, t.typinput, r.rngsubtype, t.typtype, t.typbasetype
FROM
  pg_type AS t LEFT JOIN pg_range AS r ON oid = rngtypid
WHERE
  t.typname
  IN (
      'int2',
      'int4',
      'int8',
      'oid',
      'float4',
      'float8',
      'text',
      'varchar',
      'char',
      'name',
      'bpchar',
      'bool',
      'bit',
      'varbit',
      'timestamptz',
      'date',
      'money',
      'bytea',
      'point',
      'hstore',
      'json',
      'jsonb',
      'cidr',
      'inet',
      'uuid',
      'xml',
      'tsvector',
      'macaddr',
      'citext',
      'ltree',
      'line',
      'lseg',
      'box',
      'path',
      'polygon',
      'circle',
      'interval',
      'time',
      'timestamp',
      'numeric'
    )
  OR t.typtype IN ('r', 'e', 'd')
  OR t.typinput = 'array_in(cstring,oid,integer)'::REGPROCEDURE
  OR t.typelem != 0
@mgartner mgartner added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Feb 24, 2021
@mgartner
Copy link
Collaborator Author

It'd be great it this was backport-able to 20.2, but I'm not sure how realistic that is.

@rafiss
Copy link
Collaborator

rafiss commented Feb 24, 2021

this cast is implemented by issuing a separate query with the internal executor

func queryOidWithJoin(
ctx *EvalContext, typ *types.T, d Datum, joinClause string, additionalWhere string,
) (*DOid, error) {
ret := &DOid{semanticType: typ}
info := regTypeInfos[typ.Oid()]
var queryCol string
switch d.(type) {
case *DOid:
queryCol = "oid"
case *DString:
queryCol = info.nameCol
default:
return nil, errors.AssertionFailedf("invalid argument to OID cast: %s", d)
}
results, err := ctx.InternalExecutor.QueryRow(
ctx.Ctx(), "queryOidWithJoin",
ctx.Txn,
fmt.Sprintf(
"SELECT %s.oid, %s FROM pg_catalog.%s %s WHERE %s = $1 %s",
info.tableName, info.nameCol, info.tableName, joinClause, queryCol, additionalWhere),
d)
if err != nil {

since the internal executor has a different (empty) descriptor collection than the conn_executor that is running the main query, this means that each cast operation causes descriptor lookups to be performed.

we encountered essentially the same problem here: #59880

we should be able to do something similar and not use the internal executor to implement this cast.

@ajwerner
Copy link
Contributor

ajwerner commented Feb 25, 2021

Roughly every use of the `InternalExecutor` in the `EvalContext` makes me sad. Do we need it? Could we remove it entirely? That doesn't mean we'd never delegate to it via some interface -- but also we should cut down on that. What are the important cases where we really need to delegate to the internal executor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants