ExecuteUpdate/Delete produce invalid SQL when table is present both as the primary and in a join

[roji]

Test NonSharedModelBulkUpdatesTestBase.Replace_ColumnExpression_in_column_setter:

public virtual async Task Replace_ColumnExpression_in_column_setter(bool async)
{
    var contextFactory = await InitializeAsync<Context28671>();

    await AssertUpdate(
        async,
        contextFactory.CreateContext,
        ss => ss.Set<Owner>().SelectMany(e => e.OwnedCollections),
        s => s.SetProperty(o => o.Value, "SomeValue"),
        rowsAffectedCount: 0);
}

... causes the following SQL to get generated:

UPDATE "OwnedCollection" AS o0
SET "Value" = 'SomeValue'
FROM "Owner" AS o,
	"OwnedCollection" AS o0
WHERE o."Id" = o0."OwnerId"

... which fails because OwnedCollection is present twice (duplicate alias o0).

The SQL Server SQL generation always includes the primary table in the FROM, and only ever outputs the alias for the primary table:

UPDATE [o0]  
SET [o0].[Value] = N'SomeValue'  
FROM [Owner] AS [o]  
INNER JOIN [OwnedCollection] AS [o0] ON [o].[Id] = [o0].[OwnerId]

Ideally, the SQL wouldn't even reference the Owner table (dotnet/efcore#33946). But in the meantime, on the PG side the fix is most likely to have a step which checks if the primary table is present in a join, and to remove the join if so.

[ChrisJollyAU]

@roji Can give you a hand with this as well. It worked out of the box when I added that in a couple of months ago but probably because I had fixed it earlier as part of some other tests back in Nov 2023.

Currently get the following SQL for that test

UPDATE `Owner` AS `o`
INNER JOIN `OwnedCollection` AS `o0` ON `o`.`Id` = `o0`.`OwnerId`
SET `o0`.`Value` = 'SomeValue'

Take a look at CirrusRedOrg/EntityFrameworkCore.Jet@8723fe8
and
CirrusRedOrg/EntityFrameworkCore.Jet@08509af

[roji]

Note that this looks like a dup of dotnet/efcore#33947 for SQLite.

[roji]

@ChrisJollyAU that's odd, the table that needs to be updated in this test is OwnedCollection, not Owner (PG and SQLite have a different problem where the primary/target table (OwnedCollections) also appears in the JOIN). Do you have some custom logic transforming the query in some way so that it's trying to update Owner instead?

[ChrisJollyAU]

I do have some custom stuff regarding tables (and also validity since I can't use things like TOP directly in a DELETE)

Unlike SQL Server, Jet doesn't have the FROM portion so the part after UPDATE is the whole table expression that you are updating/operating on. Jet does have a concept of an updateable query because of this (using DISTINCT, UNION, scalar subqueries make it read-only)

Think its fine (mostly/probably) as SET o0.Value = 'SomeValue' references via alias OwnedCollection

[roji]

Unlike SQL Server, Jet doesn't have the FROM portion so the part after UPDATE is the whole table expression that you are updating/operating on.

Yeah, neither do most databases (e.g. PG, Sqlite) - SQL Server is a bit special on this.

Think its fine (mostly/probably) as SET o0.Value = 'SomeValue' references via alias OwnedCollection

I'm not sure, but IIRC usually only the table immediately after UPDATE (so Owner in your case) can be updated... I'd be surprised if it's possible to update joined tables (can you check?). If so, is it even possible to update multiple tables with the same UPDATE statement?

[ChrisJollyAU]

Unlike SQL Server, Jet doesn't have the FROM portion so the part after UPDATE is the whole table expression that you are updating/operating on.

Yeah, neither do most databases (e.g. PG, Sqlite) - SQL Server is a bit special on this.

Just checked the docs and it looks like they both can do an UPDATE-FROM.
https://www.postgresql.org/docs/current/sql-update.html
https://www.sqlite.org/lang_update.html

In fact sqlite's doc says "The SQLite implementation strives to be compatible with PostgreSQL". I actually have a sqlite implementation now that passes that UPDATE test. Should be easy to implement then for efcore.pg

If so, is it even possible to update multiple tables with the same UPDATE statement?

Might be possible. Would have to test

[roji]

Just checked the docs and it looks like they both can do an UPDATE-FROM.

That's different from the SQL Server syntax; in PG, the FROM indeed can be used to join additional tables (and the PG provider indeed translates to to it, code). But the target/primary table is distinct and must be specified directly after UPDATE; you cannot have it in a FROM and then reference only its alias after the UPDATE, like we do for SQL Server (I assume SQLite works the same way). That's basically the problem with this issue, and that's why SQL Server is the only provider where this test is passing.

I'll have generally have to dig deeper into this issue to figure out the best way to fix it across all providers...

[ChrisJollyAU]

Its' actually a simple fix - 1 line change for sqlite. Trying to see how it would work for efcore.pg now

[ChrisJollyAU]

Okay so in the base QuerySqlGenerator in the VisitUpdate function you have the following snippet

if (ReferenceEquals(updateExpression.Table, joinExpression?.Table ?? table))
{
    LiftPredicate(table);
    continue;
}

The ReferenceEquals part is actually returning false. As far as I can it it is actually trying to check if the two objects are the same instance. In this case we have 2 objects, NOT the same instance but both still referencing the same table and alias

It just needs the if statement changed to the following
if (updateExpression.Table.Alias == (joinExpression?.Table.Alias ?? table.Alias))

and sqlite automatically gets that and the test Replace_ColumnExpression_in_column_setter succeeds

You can do the same in NpgsqlQuerySqlGenerator.

I did try to make your VisitUpdate the same as the base to see if you could just rely on that but there were a couple of other tests not working
Update_with_two_inner_joins
Update_base_property_on_derived_type
Update_base_type_with_OfType

[roji]

@ChrisJollyAU sounds great! We should in general no longer be doing reference equality checks in the expression tree - in EF 9.0 we've started moving away from reference identity altogether, which was the source of lots of referential integrity bugs, and makes it difficult to achieve full immutability in the tree. So this is probably a remnant from pre-8.0 days.