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

Clean up temp table naming, introduce new functions for readability #451

Conversation

timchang514
Copy link
Contributor

@timchang514 timchang514 commented Sep 25, 2024

Description

BABEL-5128

  • Reorganize queryenvironment.c to have better separation of different families of functions (ENR catalog search/insertion, cleanup, invalidation messages, rollback handling).
  • Added static function declarations at top of queryenvironment.c for better organization.
  • Refactored some function names to have consistent naming
  • Introduced new functions to clean up commonly used temp table checks across engine code

Note: tsql_setup_queryEnv and tsql_cleanup_queryEnv cannot be moved to hooks, since the internals of the QueryEnvironment struct are not visible outside of queryenvironment.c.

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 force-pushed the temp_table_code_cleanup branch 6 times, most recently from f7796cd to 37ee7df Compare September 26, 2024 18:16
@timchang514 timchang514 force-pushed the temp_table_code_cleanup branch from 05c4f1d to 178459a Compare October 2, 2024 21:47
src/backend/access/heap/heapam_visibility.c Show resolved Hide resolved
@@ -67,6 +67,15 @@ struct QueryEnvironment topLevelQueryEnvData;
struct QueryEnvironment *topLevelQueryEnv = &topLevelQueryEnvData;
struct QueryEnvironment *currentQueryEnv = NULL;

static void tsql_setup_queryEnv(QueryEnvironment *queryEnv);
Copy link
Contributor

Choose a reason for hiding this comment

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

will it make sense to put BBF changes in a separate file? if not possible due to static functions etc we can use wrappers.
Another possibility is to move most of the code to BBF extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main issue with moving BBF changes to a separate file is that the QueryEnvironment struct is meant to be private to the queryenvironment.c file. Moving everything to the extension would be possible if we move the QueryEnvironment struct into queryenvironment.h, making it publicly visible, but it has its own risks if community decides to refactor that code in some way.

queryEnv->namedRelList = NIL;
queryEnv->dropped_namedRelList = NIL;
queryEnv->savedCatcacheMessages = NIL;
tsql_setup_queryEnv(queryEnv);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hooks can help us reduce regression chances. We should never call a TSQL function in APG instance. I think it is worth having hooks so that we never execute these code path outside BBF env.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed to Surendra here btw.

@lejaokri
Copy link
Contributor

lejaokri commented Nov 5, 2024

I dont have anymore comments. approving.

* Add tuple to an ENR. It assumes that an ENR entry has been created with
* the relation name and relation oid.
*/
bool ENRAddTuple(Relation rel, HeapTuple tup)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same thought with Surendra, can we add more hook impl for functions like this one ?

@forestkeeper forestkeeper merged commit d01df35 into babelfish-for-postgresql:BABEL_4_X_DEV__PG_16_X Nov 7, 2024
2 checks passed
shardgupta pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request Nov 8, 2024
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.

5 participants