Non-enforced constraints #8076

aafemt · 2024-04-09T15:45:39Z

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

asfernandes · 2024-04-10T00:03:41Z

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

hvlad · 2024-04-10T06:57:18Z

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 · 2024-04-10T09:09:43Z

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

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

aafemt · 2024-04-10T09:12:10Z

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 · 2024-04-10T09:20:28Z

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 · 2024-04-10T09:28:03Z

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 · 2024-04-10T10:23:16Z

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 · 2024-04-10T10:24:45Z

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

aafemt · 2024-04-10T10:30:10Z

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 · 2024-04-10T10:34:10Z

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 · 2024-04-10T11:16:18Z

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 · 2024-04-10T11:24:20Z

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 · 2024-04-10T11:58:30Z

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

Let T be the table identified by the <table name> in the containing <alter table statement>.

The <constraint name> shall identify a table constraint TC of T.

TC shall not identify a unique constraint.

Access Rules
None.

General Rules

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

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 · 2024-04-10T12:02:23Z

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 · 2024-04-10T12:03:28Z

TC shall not identify a unique constraint.

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

hvlad · 2024-04-10T12:26:50Z

@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 · 2024-04-10T12:37:47Z

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 · 2024-04-10T12:50:20Z

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?

If it is so easy - why not implement it in this PR ?
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 · 2024-04-10T12:57:14Z

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 · 2024-04-18T15:30:25Z

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 · 2024-04-20T11:30:13Z

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 · 2024-04-21T16:15:27Z

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

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

aafemt · 2024-04-21T19:17:26Z

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 · 2024-04-25T16:16:53Z

Now it is complete.

hvlad

It was a quick look, so I can miss something.
Also, I didn't check extract.epp, show.epp and SystemTriggers.epp

hvlad · 2025-07-09T12:42:36Z

src/dsql/DdlNodes.epp

@@ -3482,7 +3525,6 @@ bool TriggerDefinition::modify(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch
 		{
 			switch (TRG.RDB$SYSTEM_FLAG)
 			{
-				case fb_sysflag_check_constraint:


hvlad · 2025-07-09T12:42:51Z

src/dsql/DdlNodes.epp

 					constraint.name = clause->name;
+					constraint.create->enforced = clause->enforced;
+					*notNull = clause->enforced;


This is wrong

hvlad · 2025-07-09T12:43:10Z

src/dsql/DdlNodes.epp

 				break;
+			}


Not needed braces, remove please

hvlad · 2025-07-09T12:45:44Z

src/dsql/DdlNodes.epp

+						if (strcmp(RC.RDB$CONSTRAINT_TYPE, PRIMARY_KEY) == 0 ||
+							strcmp(RC.RDB$CONSTRAINT_TYPE, UNIQUE_CNSTRT) == 0)
+						{
+							// Deactivation of primary/unique key requires check for active foreign keys


Is this code works for deactivaiton only ?

hvlad · 2025-07-09T13:09:55Z

src/dsql/DdlNodes.epp

+						}
+						else if (strcmp(RC.RDB$CONSTRAINT_TYPE, NOT_NULL_CNSTRT) == 0)
+						{
+							AutoRequest requestHandle;


Should this and below requests be cached ?

hvlad · 2025-07-09T13:10:35Z

src/dsql/DdlNodes.epp

@@ -10117,6 +10282,8 @@ void AlterIndexNode::execute(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch,
 		executeDdlTrigger(tdbb, dsqlScratch, transaction, DTW_BEFORE, DDL_TRIGGER_ALTER_INDEX,
 			name, NULL);

+		checkIndexReferenced(tdbb, transaction, name.c_str());


Is it supposed to run when index is activated ?

hvlad · 2025-07-09T13:11:53Z

doc/sql.extensions/README.ddl.txt

+
+Primary and unique keys cannot be deactivated if they are referenced by any active foreign key.
+
+The corresponding ALTER INDEX and ALTER TRIGGER statements are allowed as well.


Not clear at all.

hvlad · 2025-07-09T13:27:37Z

src/jrd/ini.epp

@@ -411,7 +411,7 @@ namespace
 			const char* users[] = {userName, "PUBLIC"};
 			int grantOptions[] = {WITH_GRANT_OPTION, 0};

-			for (unsigned i = 0; i < FB_NELEM(users); i++)
+			for (int i = 0; i < FB_NELEM(users); i++)


FB_NELEM is signed. It fixes a warning.

It is FB_SIZE_T in master and better to fix if to be at least unsigned in v5 too, there is no sence to use signed type, imho.

Are you going to change this place in master ?

Anyway, there is a lot of such misuse, why fix just one of them and at the unrelated PR without any need ?

https://groups.google.com/g/firebird-devel/c/D4eu6KGUlRU/m/s-GnU8FxAAAJ

This PR is for master.
I fix annoying warnings only in files I touch frequently during work to keep the PR small. Fortunately Mark is going to do this job so may be I'll remove this piece from patch.

This PR is for master.

So why you argue ?

I fix annoying warnings only in files I touch frequently during work to keep the PR small

This file makes NO relation with this PR. Don't do it or you will wait for reviews forever as nobody wants to waste a time.

This PR waited more than a year. Forever is just a little longer so why should I care?..

You never care. Go wait another year.

aafemt marked this pull request as ready for review April 13, 2024 15:53

aafemt changed the title ~~ALTER CONSTRAINT clause for PK/UK/FK~~ Non-enforced constraints Apr 26, 2024

ALTER CONSTRAINT clause for constraints

9abe46e

aafemt force-pushed the alter_constraint branch from bfb319e to 9abe46e Compare August 7, 2024 16:08

aafemt mentioned this pull request Aug 7, 2024

Tests for Firebird pull request 8076 FirebirdSQL/firebird-qa#30

Open

Merge branch 'master' into alter_constraint

f3c3ae7

aafemt added 2 commits November 8, 2024 13:01

Merge branch 'master' into alter_constraint

e3bb525

Merge branch 'master' into alter_constraint

2a1d755

hvlad reviewed Jul 9, 2025

View reviewed changes

aafemt marked this pull request as draft July 9, 2025 16:30


		Primary and unique keys cannot be deactivated if they are referenced by any active foreign key.

		The corresponding ALTER INDEX and ALTER TRIGGER statements are allowed as well.

Uh oh!

Non-enforced constraints #8076

Are you sure you want to change the base?

Non-enforced constraints #8076

Uh oh!

Conversation

aafemt commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfernandes commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrotteveel commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrotteveel commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

hvlad commented Apr 10, 2024

Uh oh!

aafemt commented Apr 10, 2024

Uh oh!

aafemt commented Apr 18, 2024

Uh oh!

aafemt commented Apr 20, 2024

Uh oh!

livius2 commented Apr 21, 2024

Uh oh!

aafemt commented Apr 21, 2024

Uh oh!

aafemt commented Apr 25, 2024

Uh oh!

hvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aafemt commented Apr 9, 2024 •

edited

Loading

aafemt commented Apr 10, 2024 •

edited

Loading

aafemt commented Apr 10, 2024 •

edited

Loading

mrotteveel commented Apr 10, 2024 •

edited

Loading

mrotteveel commented Apr 10, 2024 •

edited

Loading