WIP: Schema support #1466

arrowd · 2023-01-17T19:32:10Z

This is a first take on #1454

It adds entitySchema field and teach sqlite backend to use it. ~~I'm a bit worried that I had to import Internal modules, so I decided to ask for early feedback.~~

arrowd · 2023-02-01T17:08:39Z

At this stage the schema support is added to Sqlite and PostgreSQL backend. There is a simple test that passes for these backends too.

thanhvh2205 · 2023-02-16T05:42:32Z

@arrowd Currently, we have a hardcode doesTableExist function. I also need help creating a new table with the same name as the existing one but in a new schema. I think your PR can be added to the doesTableExist function. What do you think?

arrowd · 2023-02-16T10:06:56Z

I'm not sure what function you're talking about. I did adapt doesTableExist for PostgreSQL: https://github.com/yesodweb/persistent/pull/1466/files#diff-3a6af517565f4d723a74260939388b56ca107830b6a9ae68db5a877ca084f520L598

thanhvh2205 · 2023-02-16T11:27:56Z

@arrowd Sorry, you are right. Your PR will be able to solve my current problem. Can you tell me when the PR will be ready? I hope it can be merged soon.

arrowd · 2023-02-18T12:47:05Z

At the moment this PR lacks 2 things:

MySQL and MongoDB changes.
A bit more tests

Once this is done a review from Persistent developers would be required, which may take some time.

parsonsmatt

This is a fantastic start! Apologies for the delayed review.

With the current change set, this is a minor bump to persistent, which will require new lower bounds on persistent-sqlite and persistent-postgresql to take advantage of the feature.

parsonsmatt · 2023-06-01T14:04:02Z

persistent/Database/Persist/Types/Base.hs

+    , entitySchema  :: !(Maybe Text)
+    -- ^ The schema name of the database table.


Let's make another newtype SchemaNameDB for this - will make it easier to disambiguate what is and isn't a schema vs any other bit of text

parsonsmatt · 2023-06-01T14:06:47Z

persistent/Database/Persist/Sql/Types.hs

@@ -34,6 +34,7 @@ data Column = Column
 -- | This value specifies how a field references another table.
 --
 -- @since 2.11.0.0
+-- TODO: what about schema name there?


This does seem important to resolve. Unfortunately, the ColumnReference type is exposed, meaning that we will incur a major version bump if we modify it.

parsonsmatt · 2023-06-01T14:09:11Z

persistent-postgresql/Database/Persist/Postgresql.hs

Since ColumnReference isn't supported yet, this addition will fail for things that need to use it - which is specifying foreign key relationships in migrations. I suspect that a test that works on this schema would fail:

mkPersist sqlSettings [persistLowerCase| Foo schema=foo_schema name Int Bar foo FooId |]

parsonsmatt · 2023-06-01T14:14:10Z

persistent-postgresql/Database/Persist/Postgresql.hs

+                    let mbRefdef = find (\ent -> getEntityDBSchema ent == getEntityDBSchema edef
+                                            && getEntityDBName ent == crTableName colRef)
+                                        defs
+                        (schema, _) = schemaNamePair edef
+                    in case mbRefdef of
                        Just refdef
                            | Just _oldName /= fmap fieldDB (getEntityIdField edef)
                            ->
                            [AddReference
-                                (crTableName colRef)
+                                (schema, crTableName colRef)


Oh, nice, you handle this appropriately here. Well done.

parsonsmatt · 2023-06-01T14:16:22Z

persistent-postgresql/Database/Persist/Postgresql.hs

@@ -731,6 +738,7 @@ mayDefault def = case def of
    Just d -> " DEFAULT " <> d

 type SafeToRemove = Bool
+type SchemaEntityName = (Text, EntityNameDB)


I'd prefer to see a newtype or a data with named fields to make this more explicit

parsonsmatt · 2023-06-01T14:22:42Z

persistent-sqlite/Database/Persist/Sqlite.hs

+schemaNamePair' :: Maybe Text -> EntityNameDB -> SchemaEntityName
+schemaNamePair' mbSchema entName = case mbSchema of
+    Nothing -> ("", entName)
+    Just "main" -> ("", entName)
+    Just "temp" -> ("temp", entName)
+    Just schema -> ("", EntityNameDB $ (schema <> "_") <> unEntityNameDB entName)


Intriguing - so sqlite only has two schema, main and temp? And we mimic that by using schema_table_name? I'd be inclined to use __ (two underscore) to make the distinction more clear - that way you can see foo__bar_baz and know that foo is the supposed schema, rather than foo_bar_baz potentially being either BarBaz schema=foo or FooBarBaz (no schema).

parsonsmatt · 2023-06-01T14:26:22Z

persistent-postgresql/Database/Persist/Postgresql.hs

+schemaNamePair :: EntityDef -> SchemaEntityName
+schemaNamePair ent = schemaNamePair' (getEntityDBSchema ent) (getEntityDBName ent)
+
+schemaNamePair' :: Maybe Text -> EntityNameDB -> SchemaEntityName
+schemaNamePair' mbSchema entName = case mbSchema of
+    Nothing -> ("", entName)
+    Just "public" -> ("", entName)
+    Just schema -> (schema, entName)


Uses of this function case on "" and treat it differently - I think that's a good reason to keep this as a Maybe Text instead of a Text.

I'm also curious about treating "public" differently here - can you explain why we don't just keep the public prefix? Presumably, the user must have specified that - ie Foo schema=public - and I'm hesitant to override a user choice.

arrowd · 2023-06-01T14:30:48Z

Thanks for the review. Currently I'm swamped with daily work, so I will not be able to address your comments and move this work forward. I'm hoping to get more free time in the second half of June.

arrowd force-pushed the schema branch 6 times, most recently from dbc038e to 166bdf9 Compare January 27, 2023 15:04

arrowd added 6 commits February 1, 2023 19:55

Introduce entitySchema field into EntityDef

2137624

Populate entitySchema with entity-level "schema=..." attribute

9fe94a2

Introduce backendSpecificSchemaEntityName override.

8970320

Schema support for Sqlite backend.

ddedf5d

Schema support for PostgreSQL backend

3e8fd0a

Add a simple test for schema functionality.

1942681

arrowd force-pushed the schema branch from 166bdf9 to 1942681 Compare February 1, 2023 16:56

arrowd mentioned this pull request Feb 12, 2023

Postgres schemas and migrations #661

Open

parsonsmatt reviewed Jun 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Schema support #1466

WIP: Schema support #1466

arrowd commented Jan 17, 2023 •

edited

Loading

arrowd commented Feb 1, 2023

thanhvh2205 commented Feb 16, 2023

arrowd commented Feb 16, 2023

thanhvh2205 commented Feb 16, 2023

arrowd commented Feb 18, 2023

parsonsmatt left a comment

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

parsonsmatt Jun 1, 2023

arrowd commented Jun 1, 2023

		, entitySchema :: !(Maybe Text)
		-- ^ The schema name of the database table.

WIP: Schema support #1466

Are you sure you want to change the base?

WIP: Schema support #1466

Conversation

arrowd commented Jan 17, 2023 • edited Loading

arrowd commented Feb 1, 2023

thanhvh2205 commented Feb 16, 2023

arrowd commented Feb 16, 2023

thanhvh2205 commented Feb 16, 2023

arrowd commented Feb 18, 2023

parsonsmatt left a comment

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

parsonsmatt Jun 1, 2023

Choose a reason for hiding this comment

arrowd commented Jun 1, 2023

arrowd commented Jan 17, 2023 •

edited

Loading