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

release-23.1: randgen: disable generation of REGNAMESPACE type expressions #100771

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Apr 5, 2023

Backport 1/1 commits from #99168 on behalf of @msirek.

/cc @cockroachdb/release


Casting an OID to REGNAMESPACE runs this SQL using the internal executor:

SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1

When the GenerateConstrainedScans rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with GenerateConstrainedScans disabled,
behaves correctly. Disabling GenerateConstrainedScans all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:

dbDesc, err = p.byNameGetterBuilder().Get().Database(ctx, dbName)

var pgCatalogNamespaceTable = virtualSchemaTable{
comment: `available namespaces
https://www.postgresql.org/docs/9.5/catalog-pg-namespace.html`,
schema: vtable.PGCatalogNamespace,
populate: func(ctx context.Context, p *planner, dbContext catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
return forEachDatabaseDesc(ctx, p, dbContext, true, /* requiresPrivileges */
func(db catalog.DatabaseDescriptor) error {
return forEachSchema(ctx, p, db, true /* requiresPrivileges */, func(sc catalog.SchemaDescriptor) error {

// forEachDatabaseDesc calls a function for the given DatabaseDescriptor, or if
// it is nil, retrieves all database descriptors and iterates through them in

Whereas with a constrained scan, all descriptors are searched:

for _, fn := range []lookupFunc{
q.lookupVirtual,
q.lookupTemporary,
q.lookupSynthetic,
q.lookupUncommitted,
q.lookupCached,
q.lookupLeased,
} {

The correct result is from the constrained scan, because we currently
allow cross-database references. Once #55791 is fixed, both results
should match.

The fix alternatives are:

  1. disallow disabling of the GenerateConstrainedScans rule for internal SQL
  2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
testing_optimizer_disable_rule_probability setting.

Fixes #98322


Release justification: Test-only temporary fix to avoid test flakes until #55791 is fixed.

Casting an OID to REGNAMESPACE runs this SQL using the internal executor:
```
SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1
```
When the `GenerateConstrainedScans` rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with `GenerateConstrainedScans` disabled,
behaves correctly. Disabling `GenerateConstrainedScans` all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:
https://github.com/cockroachdb/cockroach/blob/7b341ffa678e4a22416e2274351fd56f415f5421/pkg/sql/virtual_schema.go#L602
https://github.com/cockroachdb/cockroach/blob/d10c3dd42c3dc40cad82792a30ae47fd2a663f43/pkg/sql/pg_catalog.go#L2099-L2106
https://github.com/cockroachdb/cockroach/blob/a7e9c4a68b81436d1f9382518d4267f74cbdac94/pkg/sql/information_schema.go#L2295-L2296

Whereas with a constrained scan, all descriptors are searched:
https://github.com/cockroachdb/cockroach/blob/af6a72a622ae05f3733b5db637403b3eaa9455f1/pkg/sql/catalog/descs/descriptor.go#L168-L175

The correct result is from the constrained scan, because we currently
allow cross-database references. Once #55791 is fixed, both results
should match.

The fix alternatives are:
1. disallow disabling of the `GenerateConstrainedScans` rule for internal SQL
2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
`testing_optimizer_disable_rule_probability` setting.

Fixes #98322

Release note: None
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-99168 branch from b1bb979 to 367b8dc Compare April 5, 2023 22:45
@blathers-crl
Copy link
Author

blathers-crl bot commented Apr 5, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

LGTM.

Don't forget to post to #release-backports and add a release justification.

@msirek
Copy link
Contributor

msirek commented Apr 6, 2023

backport approval request here

@msirek
Copy link
Contributor

msirek commented Apr 7, 2023

TFTR!

@msirek msirek merged commit 508ee2c into release-23.1 Apr 7, 2023
@msirek msirek deleted the blathers/backport-release-23.1-99168 branch April 7, 2023 16:32
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.

3 participants