Skip to content

Fixed #470 -- Added support for database defaults on fields. #16092

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

Merged
merged 1 commit into from
May 12, 2023

Conversation

LilyAcorn
Copy link
Contributor

@LilyAcorn LilyAcorn force-pushed the ticket_470 branch 4 times, most recently from dd8ae3e to bc7a375 Compare September 25, 2022 08:35
Copy link
Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

In good shape! Some comments from pair review at the Djangocon Europe 2022 sprints...

@LilyAcorn
Copy link
Contributor Author

LilyAcorn commented Oct 2, 2022

This SQL is generated by Django for Oracle, but leads to the ORA-00936: missing expression error:

INSERT INTO "FIELD_DEFAULTS_DBARTICLE" ("HEADLINE", "PUB_DATE")
    SELECT * FROM (SELECT DEFAULT col_0, TO_TIMESTAMP(DEFAULT) col_1 FROM DUAL
        UNION ALL SELECT DEFAULT, TO_TIMESTAMP(DEFAULT) FROM DUAL)

The relevant python code:

class DBArticle(models.Model):
    headline = models.CharField(max_length=100, db_default="Default headline")
    pub_date = models.DateTimeField(db_default=Now())

    def __str__(self):
        return self.headline
        articles = [DBArticle(), DBArticle()]
        DBArticle.objects.bulk_create(articles)

What I'm not yet clear on is:
1: Which bit of the generated SQL is invalid
2: Why we use a subquery selecting from DUAL instead of a VALUES clause

@LilyAcorn
Copy link
Contributor Author

I can work around the above error somewhat by setting supports_default_keyword_in_insert to False for Oracle. This isn't ideal for a few reasons:

  1. Oracle supports DEFAULT just fine in a single insert, rather than in a bulk insert.
  2. I instead run into a new error: ORA-01830: date format picture ends before converting entire input string. This is because the Now() expression becomes CURRENT_TIMESTAMP which is then wrapped in TO_TIMESTAMP() by BulkInsertMapper in bulk_insert_sql.

@LilyAcorn
Copy link
Contributor Author

I currently suspect that we can't use the DEFAULT keyword in the SELECT from DUAL subquery.

I'm wondering if the best fix here is to change how we handle Oracle bulk inserts to use the INSERT ALL INTO syntax:

INSERT ALL
  INTO mytable (column1, column2, column_n) VALUES (expr1, expr2, expr_n)
  INTO mytable (column1, column2, column_n) VALUES (expr1, expr2, expr_n)
  INTO mytable (column1, column2, column_n) VALUES (expr1, expr2, expr_n)
SELECT * FROM dual;

which I hope parallels single inserts sufficiently to support DEFAULT properly and won't cause other issues.

@Viicos
Copy link
Contributor

Viicos commented Oct 3, 2022

This looks like a nice feature, but someone reading the documentation (me included) might be wondering what is the recommended argument to use (default or db_default) when defining model and attributes. Do you think adding a small section in the doc could be beneficial?

@LilyAcorn LilyAcorn force-pushed the ticket_470 branch 2 times, most recently from 927e510 to 026fb7d Compare December 26, 2022 12:56
@LilyAcorn
Copy link
Contributor Author

LilyAcorn commented Dec 26, 2022

@felixxm I've fixed the Oracle failures, but unfortunately this has left field_defaults.tests.DefaultTests.test_field_database_defaults_returning as a flaky test. I saw this failure occasionally when debugging the other tests, but I can't reliably trigger it:

Traceback (most recent call last):
  File "/home/jenkins/workspace/pull-requests-oracle/database/oracle19/label/oracle/python/python3.11/django/test/testcases.py", line 1603, in skip_wrapper
    return test_func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jenkins/workspace/pull-requests-oracle/database/oracle19/label/oracle/python/python3.11/tests/field_defaults/tests.py", line 51, in test_field_database_defaults_returning
    self.assertLess((a.pub_date - now).seconds, 5)
AssertionError: 86399 not less than 5

For some reason, occasionally the now value is later than the a.pub_date value, despite being calculated earlier. Unfortunately this doesn't appear to be as simple as a millisecond/microsecond precision difference, as it was for sqlite on Windows. I'm wondering if Oracle and Python are using different clocks and therefore there is always a random chance of this failure on Oracle.

@LilyAcorn

This comment was marked as outdated.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@LilyFoote Thanks 👍 I left initial comments.

@LilyAcorn LilyAcorn force-pushed the ticket_470 branch 3 times, most recently from 4354606 to f713b11 Compare February 21, 2023 21:37
@felixxm felixxm force-pushed the ticket_470 branch 2 times, most recently from 26c57e1 to 9643d8e Compare May 10, 2023 07:22
@felixxm
Copy link
Member

felixxm commented May 10, 2023

Pushed set of small changes, 64% reviewed 😅 Tomorrow I want to start working on tests and docs.

@charettes
Copy link
Member

charettes commented May 10, 2023

In the light of ticket-34553 and whole saga of % escaping in literal values usage in schema alterations I think it would be worth adding tests for db_default="%", db_default="'", and db_default='"'.

@felixxm felixxm changed the title Fixed #470 -- Support database defaults for fields Fixed #470 -- Added support for database defaults on fields. May 11, 2023
@felixxm
Copy link
Member

felixxm commented May 11, 2023

In the light of ticket-34553 and whole saga of % escaping in literal values usage in schema alterations I think it would be worth adding tests for db_default="%", db_default="'", and db_default='"'.

Added tests. It's funny because they work in almost all cases 😄 The only exception is the single quotation mark ' on Oracle, it's properly quoted and is set for new rows, but .... 🥁 is not set on existing rows 🤯 I believe it's a bug in Oracle itself.

@felixxm felixxm force-pushed the ticket_470 branch 2 times, most recently from 212ae5c to 9cb8c3d Compare May 11, 2023 17:46
@felixxm
Copy link
Member

felixxm commented May 11, 2023

buildbot, test on oracle.

@felixxm felixxm force-pushed the ticket_470 branch 2 times, most recently from 22e1089 to 9c9706b Compare May 12, 2023 07:20
@felixxm
Copy link
Member

felixxm commented May 12, 2023

I noticed that inserting primary keys with db_default crashes, and fixed it.

@felixxm felixxm force-pushed the ticket_470 branch 2 times, most recently from 1dc0ef4 to 1c99e09 Compare May 12, 2023 07:23
@felixxm
Copy link
Member

felixxm commented May 12, 2023

I pushed final edits 🚀

@LilyFoote Thanks for your humongous efforts 🥇 🌟
We implemented database defaults 18 years 🥲 after the ticket was created 💥

@adamchainz
Copy link
Member

“Festina lente”, or “more haste, less speed” 🐢

🏅 Thank you @LilyFoote and @felixxm ! 🤘

Special thanks to Hannes Ljungberg for finding multiple implementation
gaps.

Thanks also to Simon Charette, Adam Johnson, and Mariusz Felisiak for
reviews.
@felixxm felixxm merged commit 7414704 into django:main May 12, 2023
@LilyAcorn LilyAcorn deleted the ticket_470 branch May 12, 2023 17:54
@felixxm felixxm temporarily deployed to schedules May 13, 2023 02:44 — with GitHub Actions Inactive
@felixxm felixxm temporarily deployed to schedules May 14, 2023 02:46 — with GitHub Actions Inactive
@felixxm felixxm temporarily deployed to schedules May 15, 2023 02:46 — with GitHub Actions Inactive
@felixxm felixxm temporarily deployed to schedules May 16, 2023 02:46 — with GitHub Actions Inactive
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.

6 participants