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

[BABEL] Fix object resolution inside pltsql routines #517

Open
wants to merge 5 commits into
base: BABEL_5_X_DEV__PG_17_X
Choose a base branch
from

Conversation

tanscorpio7
Copy link
Contributor

@tanscorpio7 tanscorpio7 commented Jan 13, 2025

Description

Remove search path handling for pltsql functions in fmgr.c.
This will now be handled in extension code.

Also we were implicitly appending sys schema to search path
when dialect was TSQL but now we ensure search path will
always contain sys for TDS connections.
So to help performance, make implicit appending of sys to
search path logic work only for non TDS connections to
babelfish_db.

Engine PR #517

Extensions PR babelfish-for-postgresql/babelfish_extensions#3394

Issues Resolved

[]

Sign Offf

Signed-off-by: Tanzeel Khan [email protected]

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.

Comment on lines +4869 to +4876
/*
* We only append sys to search path implictly for non tds connection
* to babelfish database so no need to invalidate search path or its
* cache for tds connections
*/
if (is_bbf_tds_connection_hook && is_bbf_tds_connection_hook())
return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We implicitly append sys to search path when dialect was tsql and sys was not already in search path. This is bad since everytime sql_dialect changes we have to recompute the search path. From PG17 onwards its more problematic since we also invalidate the search path hast table on every dialect change.

But now we are ensuring search path is always correct for TDS connections in extensions code, so we do not need this for TDS connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we complete remove it? is there a use case to set it for PG connections with TSQL dialect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For PG connections when users execute pltsql procedures, they will implicitly get sys in their search path.
To maintain this behaviour, I have still kept this change for non tds connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

if it is pltsql procedure, wouldn't your new code take care of setting search path correctly?

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 new changes are only triggered for ptsql routines and TDS connections. We are not handling the postgres endpoint behaviour for pltsql routines in this PR. There are couple of reasons for not handling it in this PR:

  1. We need to define how search path would work in postgres endpoint for babelfish_db, which needs more thought.
  2. Switching db context is not possible for postgres endpoint because when you are resetting the db_context, the saved db name will be "" (empty), so we need to think of a different way to reset db_context for postgres endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

this needs discussion

@tanscorpio7 tanscorpio7 changed the title Fix object resolution inside pltsql routines [BABEL] Fix object resolution inside pltsql routines Jan 28, 2025
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.

2 participants