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,tree,eval: update oid functions to use uint32 #82568

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jun 8, 2022

fixes #41904

See individual commits.

rafiss added 3 commits June 7, 2022 22:24
Postgres always uses 32 bits, so we shouldn't use a 64 bit type to
represent this.

Release note: None
Make it take a uint32 (OID) instead of a DInt.

Release note: None
Use a uint32 instead of a DInt, so we don't accidentally perform any
precision-losing type conversions.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

rafiss added 3 commits June 8, 2022 21:56
This is safer than allowing in a caller to pass in any DInt.

Release note: None
@rafiss rafiss force-pushed the fix-new-oid-functions branch from 973974d to c48196c Compare June 9, 2022 01:56
@rafiss rafiss requested a review from otan June 9, 2022 15:12
@rafiss rafiss marked this pull request as ready for review June 9, 2022 15:12
@rafiss rafiss requested a review from a team June 9, 2022 15:12
@rafiss rafiss requested review from a team as code owners June 9, 2022 15:12
@rafiss
Copy link
Collaborator Author

rafiss commented Jun 14, 2022

tftr!

bors r=otan

@craig
Copy link
Contributor

craig bot commented Jun 14, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jun 14, 2022

Build succeeded:

@craig craig bot merged commit 448bba1 into cockroachdb:master Jun 14, 2022
@rafiss rafiss deleted the fix-new-oid-functions branch June 15, 2022 19:56
@yuzefovich
Copy link
Member

yuzefovich commented Oct 4, 2022

c48196c is responsible for a regression on a micro-benchmark:

BENCHTIMEOUT=24h PKG=./pkg/bench BENCHES=SQL/MultinodeCockroach/Upsert/count=1000 ./scripts/bench c48196c300087cc1^ c48196c300087cc1
...
+ ./dev bench ./pkg/bench --timeout=24h --filter=SQL/MultinodeCockroach/Upsert/count=1000 --count=10 --bench-mem -v --stream-output --ignore-cache
Switching to c48196c300087cc1
+ ./dev bench ./pkg/bench --timeout=24h --filter=SQL/MultinodeCockroach/Upsert/count=1000 --count=10 --bench-mem -v --stream-output --ignore-cache
name                                         old time/op    new time/op    delta
SQL/MultinodeCockroach/Upsert/count=1000-24    23.8ms ± 9%    25.9ms ± 9%   +8.78%  (p=0.003 n=10+10)

name                                         old alloc/op   new alloc/op   delta
SQL/MultinodeCockroach/Upsert/count=1000-24    11.2MB ±29%    12.2MB ±12%     ~     (p=0.089 n=10+10)

name                                         old allocs/op  new allocs/op  delta
SQL/MultinodeCockroach/Upsert/count=1000-24     54.9k ±14%     79.3k ±10%  +44.47%  (p=0.000 n=10+10)

Is this concerning?

@yuzefovich
Copy link
Member

Hm, I reverted c48196c, and it didn't seem to make a difference, at least on master branch. I'll leave it to you Rafi in case you do wanna look closer into this - it's possible that we have fixed a regression that was exposed by this PR in some other way.

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 4, 2022

Thanks for the investigation! I will explore it a bit later, but probably won't probe too much deeper

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.

sql: we support bigger range for OID than Postgres
4 participants