sql: support indexes in regclass cast and pg_table_is_visible by rafiss · Pull Request #90649 · cockroachdb/cockroach

rafiss · 2022-10-25T19:59:33Z

sql: remove deprecated function from DatabaseCatalog

The ParseQualifiedTableName function was deprecated and unused.

sql: support casts from index name to regclass

Release note (sql change): Casts from index name to regclass are now
supported. Previously, only table names could be cast to regclass.

sql: add index on pg_namespace(oid)

This will allow us to efficiently join to this table.

sql: fix pg_table_is_visible to handle indexes

This uses the internal executor now to do the introspection using
pg_class where everything can be looked up all at once.

There's an increase in number of round trips, but it's a constant
factor.

Release note (bug fix): Fixed the pg_table_is_visible builtin function
so it correctly reports visibility of indexes based on the current
search_path.

cockroach-teamcity · 2022-10-25T19:59:48Z

This change is

rafiss · 2022-10-25T20:00:21Z

@ajwerner i added you here since this is going to intersect a bit with some changes you did in #90116

ajwerner

This change may have surprising performance implications. When this cast is executed, it's now no longer going to be able to leverage leases. Before we'd be doing mostly point-lookups in the happy path from in-memory data structures. Now we're going to be fetching a bunch of descriptors from the key-value store and then materializing a virtual table.

I get the motivation, but it just generally scares me a lot.

The answer I keep coming to in my head is that we need a toolkit to cache and version whole hierarchies of descriptors and we need to build data structures over them to make this sort of thing cheap. We're totally not there and I don't know when we'll get the time to be there.

How do we validate that this change is not going to trash performance of some workload?

ajwerner · 2022-10-25T21:26:33Z

pkg/sql/sem/eval/parse_doid.go

 		}
-		id, err := evalCtx.Planner.ResolveTableName(ctx, &tn)
+
+		if tn.ExplicitCatalog {


Can you encapsulate this new logic? It's not very simple and we'll want to comment it.

rafiss

Before we'd be doing mostly point-lookups in the happy path from in-memory data structures. Now we're going to be fetching a bunch of descriptors from the key-value store and then materializing a virtual table.

I've modified this so the old happy path remains. The more expensive cast logic should only happen for indexes now. I feel like the index cast is useful for debugging, but isn't something that would be as commonly used in a hot path of a workload. Also, with the new internal executor work, it's sharing more of the descriptor leases.

How do we validate that this change is not going to trash performance of some workload?

I was hoping to see some results in the RTT benchmarks.

rafiss · 2022-10-26T14:49:52Z

pkg/sql/sem/eval/parse_doid.go

 		}
-		id, err := evalCtx.Planner.ResolveTableName(ctx, &tn)
+
+		if tn.ExplicitCatalog {


ajwerner · 2022-10-26T17:26:13Z

How do we validate that this change is not going to trash performance of some workload?

I was hoping to see some results in the RTT benchmarks.

This is a problem with the rttanalysis benchmark. It can show decreases in round-trips if your change leads to scanning all descriptors because we do that in one round-trip. It's often very expensive.

rafiss · 2022-10-26T17:28:10Z

oh no

    rtt_analysis_bench.go:175: ORMQueries/django_table_introspection_4_tables: got 100, expected 2

ajwerner

This is cool. Thanks for adding it!

ajwerner · 2022-10-26T17:33:54Z

pkg/sql/sem/eval/parse_doid.go

+		  JOIN %[1]spg_catalog.pg_namespace AS n ON c.relnamespace = n.oid
+		  JOIN current_schemas AS cs ON cs.scname = n.nspname
+			WHERE c.relname = $1
+			AND $2 IS NOT NULL


when is $2 NULL here? I think this is tn.Schema() but I think that always returns a string value. What's the intention here?

it's never null and it should get constant-folded away

there's a comment up above:

// There is an unused $2 placeholder in the query so that the call to QueryRow can // be consolidated.

i can make it less confusing by making an args... slice that is different in the conditional branches, and passing it through

rafiss · 2022-10-27T17:49:23Z

ORMQueries/django_table_introspection_4_tables: got 100, expected 2 is caused by the join to pg_namespace. The extra lookups theoretically could be avoided if we added a pg_namespace(oid) index, but right now that is not possible because schema OIDs are made through the OID hasher. But I don't see why we can't change the OIDs for schemas to be the descriptor IDs.

rafiss · 2022-10-31T19:34:19Z

This PR is currently blocked on #91012

yuzefovich · 2022-11-30T01:47:36Z

I opened up #92713 to fix #91012, it'd be nice to double check that it unblocks this PR.

rafiss · 2022-12-02T23:06:15Z

thanks @yuzefovich - your fix has unblocked this

rafiss · 2022-12-05T16:40:25Z

@ajwerner if you're still up to review this, this is RFAL. the failure should be resolved by #92993

The ParseQualifiedTableName function was deprecated and unused. Release note: None

Release note (sql change): Casts from index name to regclass are now supported. Previously, only table names could be cast to regclass.

ajwerner

but see how you feel about my suggestion regarding temp schemas

Reviewed 1 of 16 files at r14, 1 of 16 files at r20, 3 of 3 files at r21, 4 of 6 files at r22, 8 of 8 files at r23, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @msbutler and @rafiss)

pkg/sql/pg_catalog.go line 2083 at r23 (raw file):

	indexes: []virtualIndex{
		{
			// incomplete is true because this index does not contain temporary schemas.

Does this matter? I think, if we can, it'd be better to say this is complete and just say our temp table implementation is somewhat broken. Given nobody ought to use it, that seems like a better tradeoff to me. Also, this will find the local session's temp namespace, right? WDYT?

pkg/sql/pg_catalog.go line 2107 at r23 (raw file):

						// No schema found, so no rows.
						//nolint:returnerrcheck
						return false, nil

nit: assign err to nil here and you can avoid having to defeat the linter

Code quote:

						// No schema found, so no rows.
						//nolint:returnerrcheck
						return false, nil

pkg/sql/pg_catalog.go line 2120 at r23 (raw file):

				} else if sc.SchemaKind() == catalog.SchemaPublic {
					// admin is the owner of the public schema.
					ownerOID = h.UserOid(username.MakeSQLUsernameFromPreNormalizedString("admin"))

nit: can we pull this out to a var rather than re-hashing every time?

rafiss

tftr! added the changes

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @msbutler)

pkg/sql/pg_catalog.go line 2083 at r23 (raw file):
i'm fine with that tradeoff.

Also, this will find the local session's temp namespace, right?

you're right, i didn't realize. that makes me feel better about marking it as complete. added a test.

pkg/sql/pg_catalog.go line 2107 at r23 (raw file):

Previously, ajwerner wrote…

nit: assign err to nil here and you can avoid having to defeat the linter

done

pkg/sql/pg_catalog.go line 2120 at r23 (raw file):

Previously, ajwerner wrote…

nit: can we pull this out to a var rather than re-hashing every time?

done

rafiss

bors r=ajwerner

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @msbutler)

rafiss · 2022-12-09T21:21:56Z

whoops missed a test ...

bors r-

craig · 2022-12-09T21:21:58Z

Canceled.

This will allow us to efficiently join to this table. Release note: None

This uses the internal executor now to do the introspection using pg_class where everything can be looked up all at once. There's an increase in number of round trips, but it's a constant factor. Release note (bug fix): Fixed the pg_table_is_visible builtin function so it correctly reports visibility of indexes based on the current search_path.

rafiss · 2022-12-09T23:15:36Z

turns out our implementation of the pg_is_other_temp_schema builtin directly depends on on the pg_namespace(oid) index containing temp schemas. so i've just added the fallback logic to the populate function.

bors r=ajwerner

craig · 2022-12-10T00:41:11Z

Build succeeded:

Bazel Essential CI (Cockroach)

rafiss requested a review from ajwerner October 25, 2022 19:59

rafiss requested a review from a team as a code owner October 25, 2022 19:59

rafiss requested review from a team and msbutler and removed request for a team October 25, 2022 19:59

This was referenced Oct 25, 2022

sql: pg_function_is_visible is not correct for UDFs #89546

Closed

builtins: fix pg_function_is_visible to work with UDFs #90657

Merged

rafiss force-pushed the fix-index-casts-builtins branch from b46116e to 601932b Compare October 25, 2022 21:03

ajwerner reviewed Oct 25, 2022

View reviewed changes

rafiss force-pushed the fix-index-casts-builtins branch from 601932b to a07c766 Compare October 26, 2022 14:49

rafiss commented Oct 26, 2022

View reviewed changes

rafiss requested a review from ajwerner October 26, 2022 14:55

rafiss force-pushed the fix-index-casts-builtins branch from a07c766 to 335361c Compare October 26, 2022 17:09

ajwerner reviewed Oct 26, 2022

View reviewed changes

rafiss mentioned this pull request Oct 27, 2022

sql: use schema desc ID for OIDs in vtables #90793

Merged

rafiss force-pushed the fix-index-casts-builtins branch from 335361c to 24b3395 Compare October 29, 2022 05:48

rafiss requested a review from a team October 29, 2022 05:48

rafiss force-pushed the fix-index-casts-builtins branch from ce8d714 to e709a03 Compare October 29, 2022 18:44

rafiss force-pushed the fix-index-casts-builtins branch from e709a03 to fe34fe4 Compare November 1, 2022 01:32

rafiss mentioned this pull request Nov 29, 2022

sql/opt: incorrect query results with virtual table lookup semi join #91012

Closed

rafiss force-pushed the fix-index-casts-builtins branch 3 times, most recently from 45efe60 to 73c9dd7 Compare December 2, 2022 21:45

rafiss requested a review from ajwerner December 2, 2022 23:06

rafiss force-pushed the fix-index-casts-builtins branch from 73c9dd7 to 4b743aa Compare December 2, 2022 23:11

rafiss force-pushed the fix-index-casts-builtins branch from 4b743aa to 904f867 Compare December 7, 2022 18:12

rafiss added 2 commits December 9, 2022 13:58

sql: remove deprecated function from DatabaseCatalog

2a59c10

The ParseQualifiedTableName function was deprecated and unused. Release note: None

sql: support casts from index name to regclass

14748fe

Release note (sql change): Casts from index name to regclass are now supported. Previously, only table names could be cast to regclass.

rafiss force-pushed the fix-index-casts-builtins branch from 904f867 to 3b66cdb Compare December 9, 2022 19:59

ajwerner approved these changes Dec 9, 2022

View reviewed changes

rafiss commented Dec 9, 2022

View reviewed changes

rafiss force-pushed the fix-index-casts-builtins branch from 3b66cdb to e3c687e Compare December 9, 2022 20:38

rafiss commented Dec 9, 2022

View reviewed changes

rafiss added 2 commits December 9, 2022 18:12

sql: add index on pg_namespace(oid)

aec217f

This will allow us to efficiently join to this table. Release note: None

rafiss force-pushed the fix-index-casts-builtins branch from e3c687e to 062845b Compare December 9, 2022 23:12

craig bot merged commit 072ec8d into cockroachdb:master Dec 10, 2022

cockroach-teamcity mentioned this pull request Dec 10, 2022

PR #90649 - sql: support casts from index name to regclass cockroachdb/docs#15794

Closed

rafiss deleted the fix-index-casts-builtins branch December 10, 2022 08:41

timgraham mentioned this pull request Dec 20, 2022

sql: Django introspection query performance regression #93955

Closed

Conversation

rafiss commented Oct 25, 2022 • edited by knz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sql: remove deprecated function from DatabaseCatalog

sql: support casts from index name to regclass

sql: add index on pg_namespace(oid)

sql: fix pg_table_is_visible to handle indexes

Uh oh!

cockroach-teamcity commented Oct 25, 2022

Uh oh!

rafiss commented Oct 25, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

rafiss Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner commented Oct 26, 2022

Uh oh!

rafiss commented Oct 26, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

rafiss Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

rafiss Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

rafiss commented Oct 27, 2022

Uh oh!

rafiss commented Oct 31, 2022

Uh oh!

yuzefovich commented Nov 30, 2022

Uh oh!

rafiss commented Dec 2, 2022

Uh oh!

rafiss commented Dec 5, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Dec 9, 2022

Uh oh!

craig bot commented Dec 9, 2022

Uh oh!

rafiss commented Dec 9, 2022

Uh oh!

craig bot commented Dec 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rafiss commented Oct 25, 2022 •

edited by knz

Loading