Postgres - Add schema name support in x-migrations-table value (#95) #483

stephane-klein · 2020-11-28T11:32:38Z

@dhui what do you think about this commit which add x-schema-name URL Query support as suggest by this comment?

Is anything missing to approve this Pull Request? Some documention, unit test or other?

Best regards,

Stéphane

Edit:

The second version of this patch add schema name support in x-migrations-table value.

Now, you can use:

migrate ... -database ...?x-migrations-table=gomigrate.schema_migrations

Edit: alternative implementation: #533

stephane-klein · 2020-11-28T19:55:01Z

Warning, there is a bug in this Pull Request, I'm going to fix it.

stephane-klein · 2020-11-28T20:17:48Z

Warning, there is a bug in this Pull Request, I'm going to fix it.

Fixed.

dhui

Did you have issues with search_path? Thinking more about this, having 2 ways to set the same thing increases complexity.

database/postgres/README.md

stephane-klein · 2020-11-29T10:05:58Z

Did you have issues with search_path? Thinking more about this, having 2 ways to set the same thing increases complexity.

@dhui yes, I have some internal product constraint, I can't change the search_path value 😞

stephane-klein · 2020-11-29T10:29:18Z

Rename to x-migrations-schema-name if only used for the migrations table. Otherwise, if we use the name x-schema-name, we should set the current schema after connecting.

Document how this interacts with search_path
migratations -> migrations

stephane-klein · 2020-11-29T13:16:33Z

Did you have issues with search_path? Thinking more about this, having 2 ways to set the same thing increases complexity.

👍

I'm working on a refactoring of this Pull Request.

stephane-klein · 2020-11-29T14:28:32Z

The second version of this patch add schema name support in x-migrations-table value.

Now, you can use:
migrate ... -database ...?x-migrations-table=gomigrate.schema_migrations

@dhui what do you think about this second implementation?

dhui

Thanks for addressing my feedback; this approach is much better!

database/postgres/postgres.go

stephane-klein · 2020-12-06T14:24:50Z

@dhui new need review 🙂

database/postgres/postgres.go

stephane-klein · 2020-12-13T19:56:08Z

@dhui need review 😉

dhui

Thanks again for addressing another round of feedback!

database/postgres/postgres.go

phungleson · 2020-12-21T22:30:09Z

Hi @stephane-klein thanks for working on this, this feature is kind of neat.

I wonder if we can get this merge soon? thanks again, cheers

dhui · 2020-12-22T04:13:22Z

FYI, merges are blocked until we move off of TravisCI

stephane-klein · 2020-12-24T18:18:03Z

We need to handle the case when there are multiple .

@dhui What do you think about this proposition which support multiples .?

I have also added Test_quoteIdentifier test.

dhui

Thanks again for addressing my feedback!

database/postgres/postgres_test.go

dhui · 2020-12-25T06:25:37Z

database/postgres/postgres.go

+func (p *Postgres) quoteIdentifier(name string) string {
+	if p.config.MigrationsTableHasSchema {
+		firstDotPosition := strings.Index(name, ".")
+		if firstDotPosition != -1 {


Should we error if a . is not found?

Should we error if a . is not found?

@dhui Why? I suggest to use default schema if . not found in table name.

Do you think this is a bad idea? Do you prefer that schema name must be mandatory?

Falling back to search_path makes sense. The behavior isn't obvious based on the config name, so could you update the docs to mention that . is not required?

dhui · 2020-12-25T06:25:52Z

database/postgres/postgres.go

+	if p.config.MigrationsTableHasSchema {
+		firstDotPosition := strings.Index(name, ".")
+		if firstDotPosition != -1 {
+			return pq.QuoteIdentifier(name[0:firstDotPosition]) + "." + pq.QuoteIdentifier(name[firstDotPosition+1:])


We error if more than one . is found or document the behavior. e.g. the first dot is treated as the schema and table name separator.

Another approach I just thought of is to have an option to treat the identifier as already quoted. e.g. you'd pass in "myschema"."mytable" and we'd just check that the string starts and ends with ". Thoughts?

or document the behavior. e.g. the first dot is treated as the schema and table name separator.

@dhui I chose to document it.

Another approach I just thought of is to have an option to treat the identifier as already quoted. e.g. you'd pass in "myschema"."mytable" and we'd just check that the string starts and ends with ". Thoughts?

Do you think that this is a better approach ?

I think having an option to treat the identifier as already quoted and requiring that the identifier starts and ends with a " is a simpler approach since we're not parsing the identifer so there are fewer opportunities for unexpected behaviors
What do you think?

If we go with the pre-quoted identifier approach, what do you think of the config name x-migrations-table-quoted?

I think having an option to treat the identifier as already quoted and requiring that the identifier starts and ends with a " is a simpler approach since we're not parsing the identifer so there are fewer opportunities for unexpected behaviors
What do you think?

@dhui This is a new Pull Request with this implementation: #533

…g-migrate#95) Add x-migrations-table-has-schema to enable schema name support in x-migrations-table value.

stephane-klein · 2020-12-26T21:52:36Z

@dhui need review 🙂

stephane-klein · 2021-01-05T09:50:13Z

@dhui Small pump 😉

dhui · 2021-01-06T04:24:46Z

@stephane-klein Sorry for the delay, was busy over the holidays. I've replied to your comments. Thanks for your patience and for pushing this PR through!

rivvid · 2021-03-05T02:19:47Z

Would love to see this feature merged -- don't want to use the public schema to store the migrations list -- if you have more than 1 service sharing a DB but using different schemas, the current implementation will not work.

FAQinghere · 2021-03-24T13:06:11Z

I might be misunderstanding something, but it seems like we already provide a config for migrations, and it already has the option for the schema to be specified. So if we just change the existing implementation to respect the config value (see:

migrate/database/postgres/postgres.go

Line 446 in 511ae9f

    
           query = `CREATE TABLE IF NOT EXISTS ` + pq.QuoteIdentifier(p.config.MigrationsTable) + ` (version bigint not null primary key, dirty boolean not null)`

) then wouldn’t it work without adding all this complexity? Compare this to the line just above where it does SELECT current_schema() for the migrations connection (

migrate/database/postgres/postgres.go

Line 434 in 511ae9f

    
           query := `SELECT COUNT(1) FROM information_schema.tables WHERE table_name = $1 AND table_schema = (SELECT current_schema()) LIMIT 1`

). I think it should just respect p.config.SchemaName?

Sorry if this isn’t expressed correctly. I did this in a local fork maybe 18 months back on a project where I had to make sure migrations were saved in a table in a schema other than the one I was connected on, and I think I remember getting it to work with only a tiny tweak. But that was a long time ago and maybe I’m misremembering. IIRC I fixed it to respect that config value when creating/checking the migrations table. I used WithInstance directly so I controlled the connection, and was connected as admin user on admin schema, but the migrations were written to the non-admin schema that I set in the Config.SchemaName. It might have been just 1 or 2 lines maybe. Unfortunately I don’t have access to it anymore. Hopefully that contributed food for thought. If not, sorry for the noise.

stephane-klein · 2021-03-31T07:08:54Z

This PullRequest is replaced by #533

When #533 will be merged, I'll close this current Pull Request.

stephane-klein · 2021-04-02T07:27:06Z

then wouldn’t it work without adding all this complexity?

@FAQinghere simplest implementation here: #533

StevenACoffman · 2022-07-30T16:00:51Z

@stephane-klein #533 is merged, did you mean to close this PR?

dhui · 2022-08-02T05:22:33Z

As @StevenACoffman mentioned, this PR is superseded by #533

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from f8f6366 to e800222 Compare November 28, 2020 20:16

dhui requested changes Nov 29, 2020

View reviewed changes

database/postgres/README.md Outdated Show resolved Hide resolved

database/postgres/README.md Outdated Show resolved Hide resolved

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch 2 times, most recently from e6653da to f01f70f Compare November 29, 2020 10:28

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from f01f70f to 884e325 Compare November 29, 2020 14:23

stephane-klein changed the title ~~Postgres - Add x-schema-name URL Query support (#95)~~ Postgres - Add schema name support in x-migrations-table value (#95) Nov 29, 2020

dhui requested changes Nov 30, 2020

View reviewed changes

database/postgres/postgres.go Outdated Show resolved Hide resolved

database/postgres/postgres.go Outdated Show resolved Hide resolved

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch 2 times, most recently from 5355d15 to 7e41a9a Compare December 6, 2020 14:24

dhui requested changes Dec 7, 2020

View reviewed changes

database/postgres/postgres.go Outdated Show resolved Hide resolved

database/postgres/postgres.go Outdated Show resolved Hide resolved

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from 7e41a9a to 81fd8bd Compare December 13, 2020 19:55

dhui requested changes Dec 14, 2020

View reviewed changes

database/postgres/postgres.go Outdated Show resolved Hide resolved

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from 81fd8bd to 0c7e31d Compare December 24, 2020 18:13

dhui requested changes Dec 25, 2020

View reviewed changes

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from 0c7e31d to e21a1b8 Compare December 26, 2020 21:47

Postgres - Add schema name support in x-migrations-table value (golan…

820ec91

…g-migrate#95) Add x-migrations-table-has-schema to enable schema name support in x-migrations-table value.

stephane-klein force-pushed the 95-postgres-add-x-schema-name-url-query-option branch from e21a1b8 to 820ec91 Compare December 26, 2020 21:51

stephane-klein mentioned this pull request Mar 28, 2021

Postgres - add x-migrations-table-quoted url query option (#95) #533

Merged

dhui closed this Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postgres - Add schema name support in x-migrations-table value (#95) #483

Postgres - Add schema name support in x-migrations-table value (#95) #483

stephane-klein commented Nov 28, 2020 •

edited

Loading

stephane-klein commented Nov 28, 2020

stephane-klein commented Nov 28, 2020

dhui left a comment

stephane-klein commented Nov 29, 2020

stephane-klein commented Nov 29, 2020 •

edited

Loading

stephane-klein commented Nov 29, 2020

stephane-klein commented Nov 29, 2020

dhui left a comment

stephane-klein commented Dec 6, 2020

stephane-klein commented Dec 13, 2020

dhui left a comment

phungleson commented Dec 21, 2020

dhui commented Dec 22, 2020

stephane-klein commented Dec 24, 2020

dhui left a comment

dhui Dec 25, 2020

stephane-klein Dec 26, 2020

dhui Jan 6, 2021

dhui Dec 25, 2020

stephane-klein Dec 26, 2020

stephane-klein Dec 27, 2020

dhui Jan 6, 2021

stephane-klein Mar 28, 2021

stephane-klein commented Dec 26, 2020

stephane-klein commented Jan 5, 2021

dhui commented Jan 6, 2021

rivvid commented Mar 5, 2021 •

edited

Loading

FAQinghere commented Mar 24, 2021 •

edited

Loading

stephane-klein commented Mar 31, 2021

stephane-klein commented Apr 2, 2021

StevenACoffman commented Jul 30, 2022

dhui commented Aug 2, 2022

Postgres - Add schema name support in x-migrations-table value (#95) #483

Postgres - Add schema name support in x-migrations-table value (#95) #483

Conversation

stephane-klein commented Nov 28, 2020 • edited Loading

stephane-klein commented Nov 28, 2020

stephane-klein commented Nov 28, 2020

dhui left a comment

Choose a reason for hiding this comment

stephane-klein commented Nov 29, 2020

stephane-klein commented Nov 29, 2020 • edited Loading

stephane-klein commented Nov 29, 2020

stephane-klein commented Nov 29, 2020

dhui left a comment

Choose a reason for hiding this comment

stephane-klein commented Dec 6, 2020

stephane-klein commented Dec 13, 2020

dhui left a comment

Choose a reason for hiding this comment

phungleson commented Dec 21, 2020

dhui commented Dec 22, 2020

stephane-klein commented Dec 24, 2020

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephane-klein commented Dec 26, 2020

stephane-klein commented Jan 5, 2021

dhui commented Jan 6, 2021

rivvid commented Mar 5, 2021 • edited Loading

FAQinghere commented Mar 24, 2021 • edited Loading

stephane-klein commented Mar 31, 2021

stephane-klein commented Apr 2, 2021

StevenACoffman commented Jul 30, 2022

dhui commented Aug 2, 2022

stephane-klein commented Nov 28, 2020 •

edited

Loading

stephane-klein commented Nov 29, 2020 •

edited

Loading

rivvid commented Mar 5, 2021 •

edited

Loading

FAQinghere commented Mar 24, 2021 •

edited

Loading