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

Set PG dialect While Running FKEY checks #518

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

Conversation

shah-nirmit
Copy link
Contributor

@shah-nirmit shah-nirmit commented Jan 21, 2025

Description

On TSQL Endpoint, When Using Temp Table Inside Trigger, we plan the Referential Integrity(RI) check queries in PG_DIALECT but were not executing them using the same dialect they were planned in , which led to improper plan invalidation and incorrect namespace search path resolution.

Fixes babelfish-for-postgresql/babelfish_extensions#3058

Issues Resolved

BABEL-5270

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.

@@ -1783,8 +1783,7 @@ bool IsTsqlTableVariable(Relation relation)
{
return sql_dialect == SQL_DIALECT_TSQL
&& relation
&& relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP
&& relation->rd_rel->relname.data
&& relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to fix this due to following build error

queryenvironment.c: In function ‘IsTsqlTableVariable’:
queryenvironment.c:1787:17: error: the comparison will always evaluate as ‘true’ for the address of ‘data’ will never be NULL [-Werror=address]
 1787 |                 && relation->rd_rel->relname.data
      |                 ^~
In file included from ../../../../src/include/postgres.h:45,
                 from queryenvironment.c:23:
../../../../src/include/c.h:742:25: note: ‘data’ declared here
  742 |         char            data[NAMEDATALEN];
      | 

ref

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original check was aimed at being NULL safe, so we should still add

&& relation->rd_rel->relname

Although I am not sure how relname would ever be NULL, but still good to have.

@@ -15,6 +15,7 @@ jobs:
run: |
sudo apt update --fix-missing -y
sudo apt install -y libipc-run-perl
sudo apt-get install -y libreadline-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was getting build error

checking for library containing pthread_barrier_wait... none required
checking for library containing readline... no
configure: error: readline library not found
If you have readline already installed, see config.log for details on the
failure.  It is possible the compiler isn't looking in the proper directory.
Use --without-readline to disable readline support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to configure engine with --without-readline.

@@ -430,11 +430,11 @@ RI_FKey_check(TriggerData *trigdata)
* referenced side. The reason is that our snapshot must be fresh in
* order for the hack in find_inheritance_children() to work.
*/
ri_PerformCheck(riinfo, &qkey, qplan,
RUN_AS_PSQL(ri_PerformCheck(riinfo, &qkey, qplan,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to change RUN_AS_PSQL macro a bit.
We should directly set sql_dialect using its global variable rather than set_config.

@@ -1783,8 +1783,7 @@ bool IsTsqlTableVariable(Relation relation)
{
return sql_dialect == SQL_DIALECT_TSQL
&& relation
&& relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP
&& relation->rd_rel->relname.data
&& relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original check was aimed at being NULL safe, so we should still add

&& relation->rd_rel->relname

Although I am not sure how relname would ever be NULL, but still good to have.

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.

[Bug]: Incorrect schema name rewriting in RI trigger queries
2 participants