Limited update/delete part 2

[steve-chavez]

Continuing the work on #2195.

As agreed on #2195 (comment), limit now works on views if they do include an explicit order.

Steps

• Works on views with explicit ?order
• Remove the default ctid fallback(tables will also need an explicit ?order)
• Apply an implicit row_count equal to the limit

[steve-chavez]

Apply an implicit row_count equal to the limit

Added the above mechanism but I'm failing to come up with a test that proves when can it happen.

It's possible to construct other examples that do the opposite: Deleting more rows than were requested.

@wolfgangwalther Can you come up with a view that does the above(discussed in #2195 (comment))

[steve-chavez]

Also on #2195 (comment)

In a later step we could add an implicit #2164 to force a hard-limit on the mutated rows using something like !row_count=lte.. This would basically throw an error if the user used some non-unique set of columns, but only if it actually leads to a mutation bigger than the requested limit.

Hm, now I wonder if that can happen.. it doesn't seem like it, but I'll try.

[steve-chavez]

If the order is on a non-unique set of columns we cannot know for sure which items will be deleted.. however the LIMIT will still apply.

So for now it's looking like it's not necessary to apply the row_count.

[steve-chavez]

Btw, this one seems to need #1929. It's likely that an user will test limit=0 and it will be weird to fail with the HTTP Range error message - because PATCH/DELETE don't need to follow the Range header semantics.

[wolfgangwalther]

If the order is on a non-unique set of columns we cannot know for sure which items will be deleted.. however the LIMIT will still apply.

That's not true, I gave an example already: #2195 (comment)

The problem is in the join: Even though the CTE might be limited correctly, the join will target more rows, because the join condition is not on unique columns.

[wolfgangwalther]

It's possible to construct other examples that do the opposite: Deleting more rows than were requested.

@wolfgangwalther Can you come up with a view that does the above(discussed in #2195 (comment))

It's certainly possible with a "not-a-simple-view", i.e. one that is not auto-updateable. In this case, to even support mutations, we need some INSTEAD OF triggers - and then behavior is pretty much undefined anyway, because PostgREST doesn't know what will happen. Even more important in that case, that the responsibility for the correctly limited request is on the user, because only they know what columns they need to make the ordering/join on.

Here's an example:

create table ta (ca int primary key);
create table tb (cb int primary key);
create view v as select * from ta, tb;

create function instead_of_delete() returns trigger
language plpgsql as $$
begin
  delete from ta where ta.ca = old.ca;
  return old;
end
$$;

create trigger ... instead of delete ...;

-- Suppose v has:
-- ca | cb
-- 10 | 10
-- 11 | 10
-- 10 | 11
-- 11 | 11

Run a delete with limit=1 and you will delete one row from ta - but two rows from v.

---

edit: I guess my example is bad. Here, it's not really the "limited delete" feature that is causing this. It's just the instead of trigger doing that. The same thing will happen when doing a DELETE FROM v WHERE ca=10 AND cb=10 - you expect a single row, but it's actually deleting multiple rows. Most importantly, it will remove two rows from v, but the RETURNING clause will only return one. So in this case the row_count filter would not even work, because it doesn't know about those extra rows deleted.

[steve-chavez]

That's not true, I gave an example already: #2195 (comment)

Nice, I included that example in a test.

Here, it's not really the "limited delete" feature that is causing this. It's just the instead of trigger doing that.
So in this case the row_count filter would not even work, because it doesn't know about those extra rows deleted.

Cool, thanks for clarifying that out. So a test on a view with that kind of instead trigger is not necessary.

[steve-chavez]

By moving the order/limit conditionals to the ApiRequest, we gained some performance:

head vs main now

throughput	400	399

head vs main before

throughput	377	382