search

palewire / django-postgres-copy

Quickly import and export delimited data with Django support for PostgreSQL's COPY command
https://palewi.re/docs/django-postgres-copy/
MIT License
180 stars 48 forks source link

Add support for temp_table_name override #146

Closed bcwaldon closed 2 years ago

bcwaldon commented 2 years ago

This enables clients that run more than one bulk import operation concurrently to avoid table naming conflicts.

bcwaldon commented 2 years ago

@palewire Hi there! I have been evaluating this library for some of my work and came across this little design issue. The content of this PR is the minimum viable solution, and there are many different ways to address the problem. Please let me know if 1) I am missing something about how these temporary tables act such that this is not actually an issue, and 2) how you would like this change to be made and tested. Thanks!

palewire commented 2 years ago

This seems reasonable to me. I haven't fully thought it thru, but I suppose an alternative would be having the library append some random string to the end of the table name, or something like that.

I'm not sure I have an opinion of whether that kind of approach would be better or worse, though I guess this new option might be nice even if we did it.

Unless you have further thoughts I think I'm okay with merging this. But I would like to see the option added to our documentation. Could.you please include that as well?

bcwaldon commented 2 years ago

This seems reasonable to me. I haven't fully thought it thru, but I suppose an alternative would be having the library append some random string to the end of the table name, or something like that.

I'm not sure I have an opinion of whether that kind of approach would be better or worse, though I guess this new option might be nice even if we did it.

I had that thought as well, but decided to go this direction first since it enables a user to choose to generate a random name themselves, or set a specific name if that is the intent.

Unless you have further thoughts I think I'm okay with merging this. But I would like to see the option added to our documentation. Could.you please include that as well?

Absolutely!

bcwaldon commented 2 years ago

@palewire I just pushed up what I believe is the doc change you requested. I have not been able to run tests locally, so please advise if that is something I need to do.

bcwaldon commented 2 years ago

Hiya @palewire - is there anything else needed here?

palewire commented 2 years ago

I'd like to run the tests more thoroughly yet. Otherwise it looks good. I'm on vacation out of the country for another week so I haven't been at my computer. Do you need the change urgently? If so I can find my laptop and shove it out.

bcwaldon commented 2 years ago

I'd like to run the tests more thoroughly yet. Otherwise it looks good. I'm on vacation out of the country for another week so I haven't been at my computer. Do you need the change urgently? If so I can find my laptop and shove it out.

No worries! It would be awesome to get this released properly within the next two weeks. Enjoy your time away :)

palewire commented 2 years ago

This has been merged and released. Thank you for your contribution.