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

OID Wraparound #232

Conversation

timchang514
Copy link
Contributor

@timchang514 timchang514 commented Oct 6, 2023

Description

This change adds a separate OID buffer for temp objects. Since temp objects are already session-local, we can allocate a separate buffer of OIDs set aside only for temp objects. This same buffer range can be used across all sessions.

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the PostgreSQL license, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@timchang514 timchang514 changed the title [BABEL-4132] OID Wraparound OID Wraparound Oct 6, 2023
@timchang514 timchang514 marked this pull request as draft October 6, 2023 20:23
@timchang514 timchang514 force-pushed the oid-rework branch 8 times, most recently from ce12282 to 2e0ce1b Compare October 13, 2023 18:51
@timchang514 timchang514 marked this pull request as ready for review October 13, 2023 19:58
src/backend/catalog/toasting.c Outdated Show resolved Hide resolved
src/backend/utils/misc/queryenvironment.c Outdated Show resolved Hide resolved
src/include/utils/rel.h Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
Copy link
Contributor

@robverschoor robverschoor left a comment

Choose a reason for hiding this comment

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

Please add test cases that address these cases:

  • exceeding 64K temp object in a session
  • a wraparound scenario
  • a scenario whereby the 64K range overlaps with an already-existing OID

@timchang514
Copy link
Contributor Author

timchang514 commented Oct 16, 2023

Please add test cases that address these cases:

  • exceeding 64K temp object in a session
  • a wraparound scenario
  • a scenario whereby the 64K range overlaps with an already-existing OID

I do plan to add these test cases (see quip doc for full test plan) - as mentioned, we'll need to add new integration tests. So these cases probably can't be part of this PR. One of the issues for example, creating over 64k temp objects in a session will time out in GitHub actions. With our own integration tests we can have more flexibility here. Especially with wraparound, if we're talking about just temp OID wraparound then that will be the same issue, we need to create enough temp objects to wrap around the buffer. General OID wraparound in postgres is even harder to trigger, we'd need to create billions of OIDs. Overlap is difficult to guarantee as well, it would basically necessitate postgres OID wraparound to test.

@lejaokri
Copy link
Contributor

This is a pretty sizeable code change and I am wondering if it is possible to have some concurrency tests as part of this PR? I am not sure if any of the Github Actions have concurrency tests in them, if not, can we do some manual tests just to verify? Describe the concurrency tests and the results in the Test Plan.

src/backend/utils/init/postinit.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
Copy link
Contributor

@lejaokri lejaokri left a comment

Choose a reason for hiding this comment

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

I am done reviewing for now.

src/backend/catalog/index.c Outdated Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
@timchang514 timchang514 force-pushed the oid-rework branch 3 times, most recently from acbdbef to ace3341 Compare October 24, 2023 06:11
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/include/access/transam.h Outdated Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
@timchang514 timchang514 force-pushed the oid-rework branch 2 times, most recently from 81072c0 to d5634cb Compare November 7, 2023 05:08
src/backend/catalog/index.c Outdated Show resolved Hide resolved
src/backend/commands/cluster.c Show resolved Hide resolved
src/backend/commands/cluster.c Show resolved Hide resolved
src/include/access/transam.h Show resolved Hide resolved
src/backend/access/transam/varsup.c Outdated Show resolved Hide resolved
src/backend/utils/misc/guc.c Show resolved Hide resolved
src/backend/catalog/catalog.c Outdated Show resolved Hide resolved
src/backend/catalog/index.c Outdated Show resolved Hide resolved
Signed-off-by: Tim Chang <[email protected]>

add full range of OID

Remove ability to alter buffer size GUC, correct error messages

Remove lc NULL check

Remove references to TempVariableCache

Revert "Remove lc NULL check"

This reverts commit 07c28e5.

Add proper check for toast tables

Added comments, added OID macros

Add temp table utilization warning

Use correct backend for index_create workaround

Add include
Signed-off-by: Tim Chang <[email protected]>
Signed-off-by: Tim Chang <[email protected]>
lejaokri
lejaokri previously approved these changes Jan 17, 2024
Copy link
Contributor

@lejaokri lejaokri left a comment

Choose a reason for hiding this comment

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

I dont have any more comment

@timchang514
Copy link
Contributor Author

Closing, this is merged in 16.x only for now.

2jungkook pushed a commit that referenced this pull request Sep 24, 2024
Normal OIDs were still being consumed in some cases during temp table creation even with temp oid buffer on. To fix this, ensure that the appropriate temp table dependent objects also use temp OID.

- Redirect type creation functions to use temp OID when it's a TSQL temp table.
- Redirect constraint creation to use temp OID in those cases as well.
- Remove workaround for indexes on temp tables. Previously, we needed to force indexes on temp tables to consume normal OIDs. With changes described in lines 1+2 above, we can now remove the workaround added in OID Wraparound #232 and properly allow indexes to use temp oid buffer.

Task: BABEL-4750
Signed-off-by: Tim Chang <[email protected]>
roshan0708 pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request Oct 18, 2024
…abelfish-for-postgresql#335)

Normal OIDs were still being consumed in some cases during temp table creation even with temp oid buffer on. To fix this, ensure that the appropriate temp table dependent objects also use temp OID.

- Redirect type creation functions to use temp OID when it's a TSQL temp table.
- Redirect constraint creation to use temp OID in those cases as well.
- Remove workaround for indexes on temp tables. Previously, we needed to force indexes on temp tables to consume normal OIDs. With changes described in lines 1+2 above, we can now remove the workaround added in OID Wraparound babelfish-for-postgresql#232 and properly allow indexes to use temp oid buffer.

Task: BABEL-4750
Signed-off-by: Tim Chang <[email protected]>
roshan0708 pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request Oct 18, 2024
…abelfish-for-postgresql#335)

Normal OIDs were still being consumed in some cases during temp table creation even with temp oid buffer on. To fix this, ensure that the appropriate temp table dependent objects also use temp OID.

- Redirect type creation functions to use temp OID when it's a TSQL temp table.
- Redirect constraint creation to use temp OID in those cases as well.
- Remove workaround for indexes on temp tables. Previously, we needed to force indexes on temp tables to consume normal OIDs. With changes described in lines 1+2 above, we can now remove the workaround added in OID Wraparound babelfish-for-postgresql#232 and properly allow indexes to use temp oid buffer.

Task: BABEL-4750
Signed-off-by: Tim Chang <[email protected]>
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.

4 participants