Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multi-deletes. #828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VaibhaveS
Copy link
Contributor

@VaibhaveS VaibhaveS commented Jun 28, 2024

Continuation of #465

Copy link
Owner

@dimitri dimitri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review: thanks for the work!

  1. can we share more code between the INSERT and the DELETE paths? it seems like we are duplicating code parts that are “infrastructure” (memory resizing, array elements handling, not “functional” bits)
  2. I'm not sure about using the WHERE ... OR ... approach for the DELETE statement with multiple target rows ; I think I would prefer a JOIN using VALUES

@dimitri dimitri requested review from dimitri and hanefi July 2, 2024 12:14
@dimitri dimitri added the enhancement New feature or request label Jul 2, 2024
@dimitri dimitri added this to the v0.17 milestone Jul 2, 2024
@VaibhaveS
Copy link
Contributor Author

  • can we share more code between the INSERT and the DELETE paths? it seems like we are duplicating code parts that are “infrastructure” (memory resizing, array elements handling, not “functional” bits)

Yep.

  • I'm not sure about using the WHERE ... OR ... approach for the DELETE statement with multiple target rows ; I think I would prefer a JOIN using VALUES

Can you share how the query would look like? I tried constructing one, but it was making things more complicated.

@hanefi
Copy link
Contributor

hanefi commented Jul 3, 2024

I could not think of any reasonable way to write a JOIN using VALUES. I stumbled upon a PostgreSQL docs page containing a note:

VALUES lists with very large numbers of rows should be avoided, as you might encounter out-of-memory failures or poor performance. VALUES appearing within INSERT is a special case (because the desired column types are known from the INSERT's target table, and need not be inferred by scanning the VALUES list), so it can handle larger lists than are practical in other contexts.

I wonder if it actually makes sense to add support for multi deletes. Can we run a simple benchmark to understand whether or not this adds value?

@VaibhaveS
Copy link
Contributor Author

Let me get back with the perf results, meanwhile @dimitri let us know your thoughts.

@dimitri dimitri modified the milestones: v0.17, v0.18 Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants