Add sanity checks for ENR entries. #506
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Task: BABEL-4776 Signed-off-by: Tim Chang <[email protected]>
xhfanhe
approved these changes
Dec 19, 2024
|
16x change: #500 |
xhfanhe
merged commit f424e4d
into
babelfish-for-postgresql:BABEL_5_X_DEV__PG_17_X
2 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #3245 to 5_X branches.
Description
Tests are running in babelfish-for-postgresql/babelfish_extensions#3245
This change adds a number of sanity checks when ENR entries are added. Previously, there were no checks here which meant that we could run into cases we had not considered where an ENR entry could depend on a non-ENR entry, and vice-versa. When adding these tests, I discovered that there are still 3 on-disk catalogs that can be referenced by ENR entries:
pg_collation
pg_authid
pg_opclass
These catalogs can't be trivially added to ENR, so I will create some followup tasks on properly handling these cases if possible.
Note that there are still some potential limitations. For example, the issue fixed in BABEL-4868 (see babelfish-for-postgresql/babelfish_extensions#3122) would not have been caught by this change due to the way that dependency entries are created for functions as column defaults.
A way to truly guarantee that all corner cases could be caught is to implement a sort of check in Postgres' dependency code. This would be a pretty sizable change and have potential performance impact, since we would need to a) add infrastructure to allow lookups to all tuples in ENR by OID, and b) search through this structure every time we try to add a dependency. I've opted to not include that for now due to the reasons mentioned, but if there is interest in that we can have a separate discussion on it.
Issues Resolved
BABEL-4776