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

"alter table" command can add a field which has "not null" definition [CORE2696] #3096

Closed
firebird-automations opened this issue Oct 19, 2009 · 19 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: Jacob Lee (letsgolee)

Is related to CORE1518
Is related to CORE1748
Is related to CORE1355
Is duplicated by CORE3298
Relate to CORE1404
Is duplicated by CORE4013
Is related to QA546

"alter table" command can add a field which has "not null" definition but does not have "default" definition.

example:

SQL> select * from equip_tbl;

EQUIPNAME EQUIPID
==================== =======
carrier a
svc b
charger
feeding roll c

SQL> alter table equip_tbl add equip_id char(2) not null;
SQL> select * from equip_tbl;

EQUIPNAME EQUIPID EQUIP_ID
==================== ======= ========
carrier a <null>
svc b <null>
charger <null>
feeding roll c <null>

In my knowledge, a table should add a field which has "not null" and "default" both like:
alter table equip_tbl add equip_id char(2) default '' not null. in this case the result is like:

SQL> alter table equip_tbl add equip_id char(2) default '' not null;
SQL> select * from equip_tbl;

EQUIPNAME EQUIPID EQUIP_ID
==================== ======= ========
carrier a
svc b
charger
feeding roll c

Commits: 218f419

====== Test Details ======

See tests that manipulates with NULL fields/domains and check results:
* CORE1518 Adding a non-null restricted column to a populated table renders the table inconsistent
* CORE4453 (Regression: NOT NULL constraint, declared in domain, does not work)
* CORE4725 (Inconsistencies with ALTER DOMAIN and ALTER TABLE with DROP NOT NULL and PRIMARY KEYs)
* CORE4733 (Command "Alter table <T> alter TYPE <C> <DOMAIN_WITH_NOT_NULL" does not verifies data in column <C> and makes incorrect assignments in <C> to ZERO / JULIAN_DATE / ASCII(0) for types INT, TIMESTAMP and VARCHAR)

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

FWIW, this is an originally intended behavior, so it actually belongs to a "design pitfall" category. But I do agree that this legacy behavior is very questionable, to say at least. I hope to have it changed in future versions, hopefully in v3.0.

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue is related to CORE1518 [ CORE1518 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue is related to CORE1748 [ CORE1748 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: Sean Leyne (seanleyne)

Dmitry,

I agree with your "design pitfall" characterization for this issue. I do not, however, believe that any fix is required or desirable.

As I see it, there is no way of knowing what value should be assigned to the existing table rows, the default value can not be infered to applicable.

It is for the developer to realize that they need to "fix" the data for these types of schema changes, not for the engine to "guess".

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

I strongly believe that *any* execution of the SQL statement *must not* make the data inconsistent (in this case -- violating the just created constraint). If this rule would be followed from the beginning, we'd virtually never hear about "unrestorable backups".

So I see only two ways to proceed with the issue: prohibit such a schema change at all or populate the new column with a default value (if specified, otherwise also prohibit). The third one might be to introduce some special syntax for the desired ability, although I don't like it much.

As for clever developers knowing what they're doing, they're free to add the new column as nullable, then populate it with data and finally change the nullability flag (and the engine must validate the last change against the stored data).

@firebird-automations
Copy link
Collaborator Author

Commented by: Sean Leyne (seanleyne)

Dmitry,

You are trying to solve the "unrecoverable backup" by adding schema requirements which are not part of the SQL standard and not imposed by other engines.

The solution to the backup problem seems straight-forward to me:

Have the backup treat the application of the column constraints ("NOT NULL" flag, constraints) as a separate step in the backup/restore cycle to the effect that when performing a restore the order of operation would be:
1. Restore basic column definition
2. Restore table data
3. Restore column constraints (i.e. full column definition)

By following this order of operation the existing data can be kept untouched while the database is fully restorable.

As for the assignment of the default value to existing rows, MS SQL addresses the assignment of values through the use of a "WITH VALUES" syntax to provide values for each existing row in the table. If "WITH VALUES" is not used, each row has the value NULL in the new column.

In the end, the inconsistency in historical data should be supported without any additional requirement. It is up to the developer to resolve the inconsistency, not the engine.

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

Sean, the "unrestorable backup" is just a consequence of the issue and yes, it can be addressed separately. But anyway, I'll be considering this ticket a bug requiring fixing as long as you'll be able to read NULLs from the NOT NULL column, period.

Now regarding the standards and other engines. From the SQL specification:

The column defined by the <column definition\> is added to T\. Let C be the column added to T\.
Case:
    a\) If C is a generated column, then <\.\.\.\>
    b\) Otherwise, C is a base column\.
   Case:
       i\) If C is an identity column, then <\.\.\.\>
      ii\) Otherwise, every value in C is the default value for C\.
      NOTE 292 — The default value of a column is defined in Subclause 11\.5, "<default clause\>"\.

From the Oracle documentation:

You cannot add a column with a NOT NULL constraint if table has any rows unless you also specify the DEFAULT clause\.

Regarding MS SQL and WITH VALUES clause:

If the new column does not allow nulls, the default value is stored in new rows regardless of whether WITH VALUES is specified\.

From the PostgreSQL documentation:

The new column is initially filled with whatever default value is given\.

From the DB2 documentation:

ADD column\-definition
    NOT NULL
        Prevents the column from containing null values\. The default\-clause must also be specified\.

@firebird-automations
Copy link
Collaborator Author

Commented by: @pmakowski

Agree Dmitry
and it would solved also CORE1355

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue is related to CORE1355 [ CORE1355 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Fix Version: 3.0 Alpha 1 [ 10331 ]

Component: Engine [ 10000 ]

assignee: Adriano dos Santos Fernandes [ asfernandes ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @asfernandes

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue is duplicated by CORE3298 [ CORE3298 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue relate to CORE1404 [ CORE1404 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Link: This issue is duplicated by CORE4013 [ CORE4013 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pcisar

Link: This issue is related to QA546 [ QA546 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

QA Status: No test

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Covered by another test(s)

Test Details: See tests that manipulates with NULL fields/domains and check results:
* CORE1518 Adding a non-null restricted column to a populated table renders the table inconsistent
* CORE4453 (Regression: NOT NULL constraint, declared in domain, does not work)
* CORE4725 (Inconsistencies with ALTER DOMAIN and ALTER TABLE with DROP NOT NULL and PRIMARY KEYs)
* CORE4733 (Command "Alter table <T> alter TYPE <C> <DOMAIN_WITH_NOT_NULL" does not verifies data in column <C> and makes incorrect assignments in <C> to ZERO / JULIAN_DATE / ASCII(0) for types INT, TIMESTAMP and VARCHAR)

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Closed [ 6 ]

Fix Version: 3.0.1 [ 10730 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

Fix Version: 3.0.0 [ 10740 ]

Fix Version: 3.0.1 [ 10730 ] =>

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

No branches or pull requests

2 participants