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

Non-enforced constraints #8076

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

aafemt
Copy link
Contributor

@aafemt aafemt commented Apr 9, 2024

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

@asfernandes
Copy link
Member

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

@hvlad
Copy link
Member

hvlad commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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

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

@aafemt
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Member

hvlad commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Member

hvlad commented Apr 10, 2024

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
Copy link
Member

hvlad commented Apr 10, 2024

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

@aafemt
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Member

hvlad commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Member

mrotteveel commented Apr 10, 2024

From SQ:2023-2:

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>

Syntax Rules

  1. Let T be the table identified by the <table name> in the containing <alter table statement>.
  2. The <constraint name> 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 <alter table constraint definition> that contains a <constraint enforcement>.

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
Copy link
Member

mrotteveel commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

  1. TC shall not identify a unique constraint.

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

@hvlad
Copy link
Member

hvlad commented Apr 10, 2024

@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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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
Copy link
Member

hvlad commented Apr 10, 2024

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
Copy link
Contributor Author

aafemt commented Apr 10, 2024

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 aafemt marked this pull request as ready for review April 13, 2024 15:53
@aafemt
Copy link
Contributor Author

aafemt commented Apr 18, 2024

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
Copy link
Contributor Author

aafemt commented Apr 20, 2024

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
Copy link

livius2 commented Apr 21, 2024

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

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

@aafemt
Copy link
Contributor Author

aafemt commented Apr 21, 2024

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
Copy link
Contributor Author

aafemt commented Apr 25, 2024

Now it is complete.

@aafemt aafemt changed the title ALTER CONSTRAINT clause for PK/UK/FK Non-enforced constraints Apr 26, 2024
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.

None yet

5 participants