Multi-tenancy with different schema write in wrong schema for indexes

[jeanbaptistedalle]

Describe the bug

We discovered a bug when using a multi-tenant version of OrchardCore with the same database for each tenant but different schema. For now, I don't know if the issue is only on postgresql or on other kind of database too.

To Reproduce

Steps to reproduce the behavior:

1. Create a SaaS tenant (so it will fall on default schema, public for us because we used a postgresql database)
2. Create a second tenant, let's say tenant1, on the same database but in the schema "tenant1"
3. Create a third tenant, let's say tenant2, on the same database but in the schema "tenant2"
4. Try to create a content item on tenant1 and on tenant2. Depend on the last launched tenant, one of them will not act as wanted.
5. The content item will be successfully created on one of the tenant (let's say tenant1).
6. On the other one (let's say tenant2), an error will occurs as the content item will be created in tenant2.Document but the index will fail as it try to insert it in tenant1's indexes. For example, one of the error we got : PostgresException: 23503: insert or update on table "ContentItemIndex" violates foreign key constraint "fk_contentitemindex" DETAIL: Detail redacted as it may contain sensitive data. Specify 'Include Error Detail' in the connection string to include this information.
7. We discovered that, if we force the tenant that failed to create the content item to reload (by activating a new feature for exemple), we will not face the issue anymore on this tenant but now, the issue will be it on the other tenant.

We think that, somewhere, there is a mismatch between the schema, maybe with "OrchardCore.Data.IndexServiceCollectionExtensions.AddIndexProvider" but we're not sure that's the rootcause for now.

Expected behavior

Each tenant successfully save the Content Item in Document and in Indexes in the correct schema.

Screenshots

If applicable, add screenshots to help explain your problem.

[jtkech]

With Sql Server but by using different schemas I could repro the same kind of issues.

SqlException: The INSERT statement conflicted with the FOREIGN KEY constraint "FK_ContentItemIndex". The conflict occurred in database "oc-dev", table "tenant1.Document", column 'Id'. The INSERT statement conflicted with the FOREIGN KEY constraint "FK_AutoroutePartIndex". The conflict occurred in database "oc-dev", table "tenant1.Document", column 'Id'. The statement has been terminated. The statement has been terminated.

I will look at it this night

[jtkech]

Okay, when we differentiate by the table prefix we differentiate the FK, for example

tenant1_FK_ContentItemIndex

But if we only differentiate by the schema we still have

FK_ContentItemIndex

Same remark for primary keys, the table prefix is used but not the schema.

---

Not sure we can include the schema in the above keys, maybe open an issue in the YesSql repo.

In the meantime, while using schemas you still need to differentiate tenants with table prefixes.

But then, by using schemas we lose the table prefix conflict checking when creating a tenant.

---

If nothing need to be done on YesSql side we may need to update our tenant validator.

[sebastienros]

The index names should be unique. I would have expected the second index creation to fail, or was it just ignored and logged.

Is this statements accurate though @jeanbaptistedalle ?

the content item will be created in tenant2.Document but the index will fail as it try to insert it in tenant1's indexes

I don't think the record can be created in the wrong table.

[sebastienros]

The fix should be done in YesSql because it already prefixes the index names that is passed with the table prefix. It should use the schema too (then). If it was not adding the table prefix then it would have meant that this is the responsibility of the called to make the name unique. It does not.

Example: CreateIndex/AlterIndex("IDX_INDEXUSERS") => SCHEMA1_TENANT1_IDX_INDEXUSERS

Once the change is done, we can call CreateIndex again in a new migration step. We could do it only if a schema is set. Though I assume that calling it again with an existing name (in case the schema is not used) would be no-op as it did not break today.

Note that we don't expect users to have schemas without prefixes and multiple tenants otherwise nothing would work as this issue demonstrates. We might just want to clean things up correctly for the case when both schemas and table prefixes are used (delete old indexes).

[jtkech]

As I understood the problem would not be related to the inner indexes IDX_... => CreateIndex(), but related to index tables => CreateMapIndexTable().

The problem would not be the table name including the schema, but the foreign key (maybe also the primary key) that includes the table prefix (if it exists) but not the schema.

[tablePrefix_]FK_ContentItemIndex

[MikeAlhayek]

@jtkech I think we understand the issue.

The following works
CREATE TABLE dbo1.A
CREATE TABLE dbo2.A

But CREATE INDEX IDX_a ON TABLE dbo1.A and CREATE INDEX IDX_a ON TABLE dbo2.A will not work since the same SQL Index name is the same. I think @sebastienros understand the problem as per the discussion we had during the meetings.

What I think he suggests, if to change the command the creates the SQL constraint/Index not sql index table by prepending the schema "if it specified" to the name. So the command will change from CREATE INDEX IDX_a ON TABLE dbo1.A to CREATE INDEX dbo1_IDX_a ON TABLE dbo1.A

In the picture below, you'll notice we do not prefix the SQL index with the site name since we only have site1_AliasPartIndex table. But if we use schema, then we can have multiple tables with the same name dbo1.AliasPartIndex and dbo2.AliasPartIndex

so when a schema is used, we'll change IDX_AliasPartIndex_DocumentId to dbo1_IDX_AliasPartIndex_DocumentId or dbo2_IDX_AliasPartIndex_DocumentId

Note that we have to do the same thing with the primary key also since there is no prefix. However, we may have a problem when trying to create a primary key index with the new name while another primary key already exists.. but this may not matter since no one really use schema with no prefix since this is the first time we get a report of an issue.

[jtkech]

Okay cool then, but as I remember the exception was explicitly mentioning the foreign key FK, if I have time I will try to change the IDX_ name, but for now I'm bothered by Visual Studio that don't want to restore any package while testing dotnet 8 prview 6 ;)

[MikeAlhayek]

Hummm, just doing some googling and this is what I found. So the same table name and constraint name is unique to the schema.

That constraint names have to be unique to the schema (ie. two different schemas in the same database can both contain a constraint with the same name) is not explicitly documented. Rather you need to assume the identifiers of database objects must be unique within the containing schema unless specified otherwise.

What we are doing should work. Maybe the issue is that we are not using the schema value is some queues. I have to try to reproduce the issue

[jtkech]

Just did a quick test, after starting the app, it first works with any tenant but then when switching to another one it saves in the right Document table but populate the IndexTable of the previous tenant.

Look at the beginning of this command where we can see both [schema1] and [schema2].

exec sp_executesql N'insert into [schema1].[tenant_Document] ([Id], [Type], [Content], [Version])
values(@Id_1, @Type_1, @Content_1, @Version_1);insert into [schema2].[tenant_ContentItemIndex]
...

When I will have time I'll try to debug directly into YesSql locally, too lazy for now ;)

[jtkech]

In the meantime just saw that some commands are cached.

    private static readonly ConcurrentDictionary<CompoundKey, string> InsertsList = new();
    private static readonly ConcurrentDictionary<CompoundKey, string> UpdatesList = new();

And then used like this.

        var key = new CompoundKey(
            dialect.Name,
            type.FullName,
            _store.Configuration.TablePrefix,
            Collection);

        if (!InsertsList.TryGetValue(key, out var result))
        {
            ...
        }

So looks like the problem is that the CompoundKey doesn't include the schema.

[jtkech]

The solution would be to use

        var key = new CompoundKey(
            dialect.Name,
            type.FullName,
            _store.Configuration.Schema,
            _store.Configuration.TablePrefix,
            Collection);

By allowing new CompoundKey() to accept 5 parameters.

[sebastienros]

So the cache is the reason some documents are stored in the wrong index table. Good catch @jtkech

[sebastienros]

So the name of the index is fine, and the issue is only with caching, should be easy to fix

[jeanbaptistedalle]

The index names should be unique. I would have expected the second index creation to fail, or was it just ignored and logged.

Is this statements accurate though @jeanbaptistedalle ?

the content item will be created in tenant2.Document but the index will fail as it try to insert it in tenant1's indexes

I don't think the record can be created in the wrong table.

Sorry, I was off, but it depends, sometimes, when the same documentId already exists in the other schema, it does not fail, but the index in the tenant2 is wrong as some data are missing and it lead to others problems (like the user not visible in the User pages as it uses the UserIndex).

@jtkech Wow good job, I don't even known that there was cache on that !

[jeanbaptistedalle]

Sorry for the issue state, miss click on close with comment button ...

[MikeAlhayek]

@jtkech is this something you can fix soon? We are looking to release YesSql 3.3.1 very soon to fix another issue for OC 1.7.1. If you are able to push a PR on YesSql to fix this, that would be amazing.

[jtkech]

Okay I will look at it asap.

[jtkech]

Done sebastienros/yessql#501 (comment)