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

Upsert #156

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Upsert #156

wants to merge 15 commits into from

Conversation

geoffrey-eisenbarth
Copy link
Contributor

I'm willing to write tests and update documentation if this approach seems sufficient.

Having some trouble getting the test suite to run (getting django.db.utils.OperationalError: fe_sendauth: no password supplied when I run pipenv run python setup.py test); do I need to create a (postgres) database called test?

@palewire
Copy link
Owner

palewire commented Mar 8, 2023

Thanks for taking a run at it. Tests and documentation will def. be something we want. Yes I think you'll need a database to test against.

Here is how the test suite configures the database https://github.com/palewire/django-postgres-copy/blob/main/.github/workflows/continuous-deployment.yaml#L11-L21

@geoffrey-eisenbarth
Copy link
Contributor Author

geoffrey-eisenbarth commented Mar 8, 2023

@palewire Thanks for giving the go-ahead. I've got some new tests in progress, but will wait to commit them until I can run them locally.

EDIT: Got testing up and running, had to edit my pg_hba.conf to trust, I think my postgres db user had a password set.

@geoffrey-eisenbarth
Copy link
Contributor Author

@palewire While writing tests I discovered a weird nuance about Django's "Model Constraints," it's now been addressed (and covered in testing).

The basic issue was that if a constraint is specified as

class Meta:
  constraints = [
    models.UniqueConstraint(
      fields=['field1', 'field2'],
      include=['field3'],  # This prevents a real psql "constraint" from being created
      name='my_constraint',
    )
  ],

then Django doesn't actually create a PSQL constrained named "my_constraint", it creates a UNIQUE INDEX, so doing something like ON CONFLICT ON CONSTRAINT "my_constraint" DO... will fail because there is no PSQL constraint named "my_constraint."

However, it seems in PSQL documentation for INSERT they recommend not naming constraints directly, but rather to let PSQL figure it out by just naming the relevant columns, so I've done that and things look good, all tests passing.

Going to look into how to update the documentation next. Is there anything obvious next steps I'm missing? Thanks!

@geoffrey-eisenbarth
Copy link
Contributor Author

@palewire Is it possible for me to re-run the GitHub actions? The PR is passing tests and flake8 locally. Not sure how to progress.

@geoffrey-eisenbarth
Copy link
Contributor Author

@palewire Can I get an update on this? It's been working well on my machine for awhile now, hoping to use it in production soon. Is there anything else I can do to move this forward?

Thanks!

@geoffrey-eisenbarth
Copy link
Contributor Author

After a discussion awhile back on the postgresql Slack, it was recommended I add the IS DISTINCT FROM clause to avoid "updating" rows that didn't actually change.

@geoffrey-eisenbarth geoffrey-eisenbarth mentioned this pull request Dec 30, 2023
@geoffrey-eisenbarth
Copy link
Contributor Author

@palewire Any movement for this? I'm willing to fix any conflicts with the current version. If it's your preference not to support upsert, or do it a different way, could you let me know?

Thanks!

@geoffrey-eisenbarth
Copy link
Contributor Author

@palewire Just giving you another ping. If you want me to let this go, just let me know. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants