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 handling of defaultSchemaName on Postgresql #2812

Merged
merged 1 commit into from
May 13, 2022

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Apr 29, 2022

Description

When we first connect to postgresql, we call SET SEARCH_PATH=... to ensure the defaultSchemaName is first in the list.

However, when we roll back the connection (which we do throughout a liquibase run to ensure we have a clean connection) the search path on the connection gets rolled back to the initial value.

This PR restores the the reset of SEARCH_PATH in the rollback operation for postgreql

@nvoxland
Copy link
Contributor Author

Notes and test results:

Things to be aware of

  • I reproduced the problem by creating a changeset with a simple <sql>create table test (id int)</sql> change in it, then ran liquibase update with a defaultSchemaName=other and saw that the table was created in public rather than other
  • This rollback call will be made for postgresql subclasses such as cockroachdb and enterprisedb
  • It is the same init logic called at the beginning of every liquibase run, so I'm not worried about it
  • I don't think the existing test automation can handle checking this, so there is no automated test

Things to worry about

@github-actions
Copy link

Unit Test Results

  4 512 files  ±  0    4 512 suites  ±0   33m 26s ⏱️ - 4m 52s
  4 386 tests  - 92    4 172 ✔️  - 88     214 💤  - 4  0 ±0 
51 912 runs  ±  0  46 904 ✔️ +  4  5 008 💤  - 4  0 ±0 

Results for commit 363003e. ± Comparison against base commit ffd5cc1.

@kataggart kataggart added this to To Do in Conditioning++ via automation May 2, 2022
@nvoxland nvoxland requested review from suryaaki2 and XDelphiGrl and removed request for XDelphiGrl May 3, 2022 07:00
@ecarneiro01 ecarneiro01 self-assigned this May 9, 2022
@ecarneiro01
Copy link

To test this I used the following changelog file: dbchangelog.zip which also includes this SQL file: sqlStatements.zip across the different databases that were tested.

In the changelog file there's a table creation using the tag. Meanwhile in the included SQL script there's a table creation, column creation, constraint creation and an information insert.

The results after running an update with this changelog for each database and using a defaultSchemaName: other are the following:

  • Postgres
    • Verify successful creation of the database objects in the specified default schema
      • Table "Test" (through tag in the xml changelog). PASS
      • Tables "Person" and "Company" (through sqlStatements.sql script). PASS
      • Columns "Country" and "Worksfor_company_id" (through sqlStatements.sql script). PASS
      • Constraint "fk_person_worksfor" (through sqlStatements.sql script). PASS
      • Row insert in the table "Country" (through sqlStatements.sql script). PASS
  • CockroachDB
    • Verify successful creation of the database objects in the specified default schema
      • Table "Test" (through tag in the xml changelog). PASS
      • Tables "Person" and "Company" (through sqlStatements.sql script). PASS
      • Columns "Country" and "Worksfor_company_id" (through sqlStatements.sql script). PASS
      • Constraint "fk_person_worksfor" (through sqlStatements.sql script). PASS
      • Row insert in the table "Country" (through sqlStatements.sql script). PASS
  • EDB
    • Verify successful creation of the database objects in the specified default schema
      • Table "Test" (through tag in the xml changelog). PASS
      • Tables "Person" and "Company" (through sqlStatements.sql script). PASS
      • Columns "Country" and "Worksfor_company_id" (through sqlStatements.sql script). PASS
      • Constraint "fk_person_worksfor" (through sqlStatements.sql script). PASS
      • Row insert in the table "Country" (through sqlStatements.sql script). PASS

Also, to ensure that the PR #1864 fix was still working propertly, I also executed the test cases that were used to test those changes:

Verify successful update for tables, indexes and inert statements with spatial types and spatial methods:

  • Box2D PASS
  • GEOMETRY PASS
  • GIST PASS
  • ST_GeomFromText PASS

Verify a successful update for a procedure with spatial parameters (BOX2D, BOX3D, GEOMETRY and GEOMETRY_DUMP). PASS

Verify a successful rollback of a procedure with spatial parameters (BOX2D, BOX3D, GEOMETRY and GEOMETRY_DUMP). PASS

Verify a successful generate-changelog for tables, indexes and insert statements with spatial types and spatial methods. PASS

In the following file there's changelog directory with the generated updatesql and snapshot of the database after running the update command for each database (Postgres, Postgis, EDB and CockroachDB).
2812.zip


Environment information
Liquibase Version: [Core: //preserve-postgresql-searchpath/2459/363003/2022-04-29 21:12+0000, Pro: master/1076/a66427/2022-04-29T17:02:29Z] running under jdk-11.0.15.1
CockroachDB CCL v21.2.7 (x86_64-unknown-linux-gnu)
PostgreSQL 14.2 (Debian 14.2-1.pgdg110+1) on x86_64-pc-linux-gnu
PostGIS 3.1.4+dfsg-3.pgdg110+1 using Postgres 13.5-1.pgdg110+1
EDB repo ghcr.io/enterprisedb/postgresql:14-debian (which pulled Postgres 14.3-1.pgdg100+1)

@tooptoop4
Copy link

did this change da won dun break me version 4.11 ?

{"log":"  Position: 28 [Failed SQL: (0) SET SEARCH_PATH TO public, $user, public]\n","stream":"stderr","time":"2022-06-07T09:45:20.52837886Z"}
{"log":"\n","stream":"stderr","time":"2022-06-07T09:45:20.528382791Z"}```

@kataggart
Copy link
Contributor

@tooptoop4 Would you mind opening a new issue reporting the details of your error so we can better track it and help resolve? Since this PR is already merged and released I don't want your issue to get lost. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment