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

Fix upgrade script to resolve MVU issue from 15.11 #3465

Conversation

Anikait143
Copy link
Contributor

Description

  • Fix MVU issue from 15.11 by fixing upgrade script babelfishpg_tsql--4.2.0--4.3.0.sql to prepend PG_CATALOG schema on string_agg function call

Issues Resolved

BABEL-5595

Authored-by: Anikait Agrawal [email protected]
Signed-off-by: Anikait Agrawal [email protected]

Test Scenarios Covered

  • Use case based -

  • Boundary conditions -

  • Arbitrary inputs -

  • Negative test cases -

  • Minor version upgrade tests -

  • Major version upgrade tests -

  • Performance tests -

  • Tooling impact -

  • Client tests -

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 Apache 2.0 and PostgreSQL licenses, 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.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 13195831508

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.005%) to 74.978%

Files with Coverage Reduction New Missed Lines %
contrib/babelfishpg_tds/src/backend/tds/tdsutils.c 1 72.43%
contrib/babelfishpg_tds/src/backend/tds/tdscomm.c 2 76.03%
Totals Coverage Status
Change from base Build 13193989600: -0.005%
Covered Lines: 47079
Relevant Lines: 62790

💛 - Coveralls

@@ -6719,7 +6719,7 @@ CREATE OR REPLACE VIEW information_schema_tsql.columns_internal AS
CAST(ext.orig_name AS sys.nvarchar(128)) AS "TABLE_SCHEMA",
CAST(
COALESCE(
(SELECT string_agg(
(SELECT pg_catalog.string_agg(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should fix the issue. Although I think it would be better to add PG_CATALOG for all the string_agg even if it is not invoked from inside COALESCE function as that would be a good coding practice. There are such occurrences in other upgrade scripts where we call string_agg instead of PG_CATALOG.string_agg which are not invoked from inside COALESCE function. I think we should update that.

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 now, we have verified locally as well as in pre-prod upgrades and this change fixes the issue, as far as other occurences are concerned we have closely verified that either of the 2 definitions would work correctly with no differences and for prepending all such occurences there is a long list of previous such function calls which will not affect us but we can update them as a part of future changes

@@ -6719,7 +6719,7 @@ CREATE OR REPLACE VIEW information_schema_tsql.columns_internal AS
CAST(ext.orig_name AS sys.nvarchar(128)) AS "TABLE_SCHEMA",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the search path to sys, pg_catalog, public to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs more investigation as to why we have different search paths, will take this up later

Copy link
Contributor

@ahmed-shameem ahmed-shameem left a comment

Choose a reason for hiding this comment

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

Let's update the description of the PR as this is not only true for 15.11 but from previous versions as well.

@rishabhtanwar29 rishabhtanwar29 merged commit 9f7c2e9 into babelfish-for-postgresql:BABEL_5_X_DEV Feb 7, 2025
46 checks passed
@rishabhtanwar29 rishabhtanwar29 deleted the babel-5595-5x branch February 7, 2025 10:16
Anikait143 added a commit to amazon-aurora/babelfish_extensions that referenced this pull request Feb 7, 2025
…tgresql#3465)

Fix MVU issue from 15.11 by fixing upgrade script `babelfishpg_tsql--4.2.0--4.3.0.sql` to prepend `PG_CATALOG` schema on string_agg function call.

Task: BABEL-5595
Signed-off-by: Anikait Agrawal <[email protected]>
rishabhtanwar29 pushed a commit that referenced this pull request Feb 7, 2025
Fix MVU issue from 15.11 by fixing upgrade script `babelfishpg_tsql--4.2.0--4.3.0.sql` to prepend `PG_CATALOG` schema on string_agg function call.

Task: BABEL-5595
Signed-off-by: Anikait Agrawal <[email protected]>
Anikait143 added a commit to amazon-aurora/babelfish_extensions that referenced this pull request Feb 7, 2025
…tgresql#3465)

Fix MVU issue from 15.11 by fixing upgrade script `babelfishpg_tsql--4.2.0--4.3.0.sql` to prepend `PG_CATALOG` schema on string_agg function call.

Task: BABEL-5595
Signed-off-by: Anikait Agrawal <[email protected]>
priyansx pushed a commit that referenced this pull request Feb 7, 2025
Fix MVU issue from 15.11 by fixing upgrade script `babelfishpg_tsql--4.2.0--4.3.0.sql` to prepend `PG_CATALOG` schema on string_agg function call.

Task: BABEL-5595

Signed-off-by: Anikait Agrawal <[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.

5 participants