search

FirebirdSQL / firebird

Firebird server, client and tools
https://www.firebirdsql.org/
1.19k stars 204 forks source link

Non-enforced constraints #8076

Open aafemt opened 1 month ago

aafemt commented 1 month ago

Partial implementation of #2358 using standard-compliant clause ALTER CONSTRAINT [NOT] ENFORCED.

asfernandes commented 1 month ago

What will happen with isql -x for not enforced constraints?

hvlad commented 1 month ago

Why not enforced constaint deactivates corresponding index ? It is not necessary at all and requires costly operation (build index) when constraint is enforced again.

aafemt commented 1 month ago

What will happen with isql -x for not enforced constraints?

Nothing. CREATE TABLE statement does not support creation of inactive constraints.

aafemt commented 1 month ago

Why not enforced constaint deactivates corresponding index ?

Because active unique index won't allow insertion of duplicates by itself.

It is not necessary at all and requires costly operation (build index) when constraint is enforced again.

AFAIK deactivate index + bulk insert + activate index is less costly than bulk insert with active index.

hvlad commented 1 month ago

Why not enforced constaint deactivates corresponding index ?

Because active unique index won't allow insertion of duplicates by itself.

It is could be changed

It is not necessary at all and requires costly operation (build index) when constraint is enforced again.

AFAIK deactivate index + bulk insert + activate index is less costly than bulk insert with active index.

It is about constraints in general, not about singe use-case.

aafemt commented 1 month ago

It is could be changed

Ok, if it is ever changed, this feature can be changed too.

It is about constraints in general, not about singe use-case.

I know no other use case for this feature.

hvlad commented 1 month ago

It is could be changed

Ok, if it is ever changed, this feature can be changed too.

It would be much better to do it at once, not leaving big piece of work for someone else, IMHO

It is about constraints in general, not about singe use-case.

I know no other use case for this feature.

It doesn't means they not exists. Such kind of "arguments" never works.

hvlad commented 1 month ago

BTW, it will be good to show here excerpt from standard that describes the feature.

aafemt commented 1 month ago

It would be much better to do it at once, not leaving big piece of work for someone else, IMHO

I do what I can. Sorry, I have no idea how to implement duplicates in unique indices and doubt that it is anyhow useful.

From the standard I have access to BNF only:

11.25 <alter table constraint definition> Function

Change the definition of a table constraint. Format

<alter table constraint definition> ::=
  ALTER CONSTRAINT <constraint name> <constraint enforcement>

PS: Oops, header for 11.25 was lost because of parentheses...

aafemt commented 1 month ago

It doesn't means they not exists. Such kind of "arguments" never works.

Let's solve problems as they appear. In this case - wait while someone come with different use case. In this PR I only solve my own problem which I know to exist.

hvlad commented 1 month ago

It would be much better to do it at once, not leaving big piece of work for someone else, IMHO

I do what I can. Sorry, I have no idea how to implement duplicates in unique indices and doubt that it is anyhow useful.

If unique constraint is not enforced one should not wonder duplicates in "unique" index

From the standard I have access to BNF only:

11.25 Function Change the definition of a table constraint. Format

<alter table constraint definition> ::=
  ALTER CONSTRAINT <constraint name> <constraint enforcement>

Do you have BNF that allows to change ENFORCED state ? So far the only definition of [NOT] ENFORCED constraints I found is completely different from what you want here:

https://www.ibm.com/docs/en/db2/11.5?topic=constraints-creating-modifying

@mrotteveel: could you shed a light on this, please ?

aafemt commented 1 month ago

Do you have BNF that allows to change ENFORCED state ?

It is exactly quoted. <alter table constraint definition> is a clause of ALTER TABLE statement:

11.10 <alter table statement> Function

Change the definition of a table. Format

<alter table statement> ::=
  ALTER TABLE <table name> <alter table action>

<alter table action> ::=
    <add column definition>
  | <alter column definition>
  | <drop column definition>
  | <add table constraint definition>
  | <alter table constraint definition>
  | <drop table constraint definition>
  | <add table period definition>
  | <drop table period definition>
  | <add system versioning clause>
  | <drop system versioning clause>

And even if I get it wrong, I'll just remove "ANSI standard-compliant" from the doc. This PR is still a valid solution for #2358 even without standard compliance.

mrotteveel commented 1 month ago

From SQ:2023-2:

11.25 \

Function Change the definition of a table constraint.

Format

<alter table constraint definition> ::=
    ALTER CONSTRAINT <constraint name> <constraint enforcement>

Syntax Rules 1) Let T be the table identified by the \

in the containing \. 2) The \ shall identify a table constraint TC of T. 3) TC shall not identify a unique constraint.

Access Rules None.

General Rules 1) The table constraint descriptor of TC is modified as follows.    Case:       a) If NOT ENFORCED is specified, then the indication of whether the constraint is enforced or not enforced is replaced with an indication that the constraint is not enforced.       b) Otherwise, the indication of whether the constraint is enforced or not enforced is replaced with an indication that the constraint is enforced.

Conformance Rules 1) Without Feature F492, “Optional table constraint enforcement”, conforming SQL language shall not contain an \ that contains a \.

And as a reminder, the SQL standard defines "unique constraint" as (4.25.3.2 Unique constraints):

An indication of whether it was defined with PRIMARY KEY or UNIQUE.

In other words, by syntax rule 3, [NOT] ENFORCED is not allowed for unique constraints (i.e. both PRIMARY KEY and UNIQUE constraints), only for referential and check constraints.

Section 4.25.3.1 Introduction to table constraints also says:

NOTE 52 — A unique constraint is always enforced. A table check constraint or a referential constraint can either be enforced or not enforced.

mrotteveel commented 1 month ago

In other words, for standard conformance, the support for NOT ENFORCED for PRIMARY/UNIQUE KEY should be removed, and support for CHECK constraints should be added, because not enforcing PRIMARY/UNIQUE KEYS is explicitly not allowed by the standard.

aafemt commented 1 month ago
  1. TC shall not identify a unique constraint.

Ah, ok, this really is going to be a non-standard feature.

hvlad commented 1 month ago

@mrotteveel: thanks !

IMHO, we could live with [NOT] ENFORCED PRIMARY/UNIQUE KEY constraints. I'm not insisting, just show my current opinion.

And I'm still very concerned about needs of [de]activation of underlying indices in-sync with constraint enforcement. Proposed implementation should be almost completely changed to avoid it, it can't be just "fixed a bit".

aafemt commented 1 month ago

Proposed implementation should be almost completely changed to avoid it, it can't be just "fixed a bit".

If ALTER INDEX is going to be still supported for PK/UK/FK indices I don't see more changes than a couple of lines in ALTER CONSTRAINT implementation: just update a field in RDB$RELATION_CONSTRAINT (and some flags in cached structures depending on implementation) instead of call to modifyIndex(). And one-liner for ISQL. What do I miss?

hvlad commented 1 month ago

Proposed implementation should be almost completely changed to avoid it, it can't be just "fixed a bit".

If ALTER INDEX is going to be still supported for PK/UK/FK indices I don't see more changes than a couple of lines in ALTER CONSTRAINT implementation: just update a field in RDB$RELATION_CONSTRAINT (and some flags in cached structures depending on implementation) instead of call to modifyIndex(). And one-liner for ISQL. What do I miss?

  1. If it is so easy - why not implement it in this PR ?
  2. You missed a lot of details of how engine checks indexed constraints and how it manages its internal metadata caches.

PS I could be wrong and there is a less work to do actually :)

aafemt commented 1 month ago

Exactly because I don't know how engine manage constraints and metadata cache, I've chose the simplest solution: to reuse index deactivation code. For my purposes it is enough.

If you insist I can remove ALTER CONSTRAINT part of this PR and limit it to allow deactivation of constraint indices. Then whoever is eager to implement ALTER CONSTRAINT can do it from scratch.

aafemt commented 1 month ago

What will happen with isql -x for not enforced constraints?

BTW, isql -x for inactive indexes also extract them as active. Should it be considered as a separate issue?

aafemt commented 4 weeks ago

Now this PR is a complete solution for #2358. Shall you review it in current state or I must make it much bigger implementing creation of not enforced constraints?

livius2 commented 3 weeks ago

Nothing. CREATE TABLE statement does not support creation of inactive constraints.

So after Create Table stetement should be additional statement fo deactivation.

aafemt commented 3 weeks ago

after Create Table stetement should be additional statement fo deactivation.

No, just creation of a not enforced constraint should be added. The only question is if it will be in this PR or a separate one.

aafemt commented 3 weeks ago

Now it is complete.

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.