Thread: PATCH: Include all columns in default names for foreign key constraints.
Hello!
I have written a small patch to modify the default names for foreign key
constraints. Currently if the foreign key is composed of multiple columns we
only use the first one in the constraint name. This can lead to similar
constraint names when two foreign keys start with the same column. This sort
of situation may commonly occur in a multi-tenant environment.
> CREATE TABLE users (tenant_id int, id int, PRIMARY KEY (tenant_id, id));
> CREATE TABLE posts (tenant_id int, id int, PRIMARY KEY (tenant_id, id));
> CREATE TABLE comments (
tenant_id int,
id int,
post_id int,
commenter_id int,
FOREIGN KEY (tenant_id, post_id) REFERENCES posts,
FOREIGN KEY (tenant_id, commenter_id) REFERENCES users
)
> \d comments
Table "public.comments"
Foreign-key constraints:
"comments_tenant_id_fkey" FOREIGN KEY (tenant_id, commenter_id)
REFERENCES users(tenant_id, id)
"comments_tenant_id_fkey1" FOREIGN KEY (tenant_id, post_id)
REFERENCES posts(tenant_id, id)
The two constraints have nearly identical names. With my patch the default names
will include both column names, so we have we will instead have this output:
Foreign-key constraints:
"comments_tenant_id_commenter_id_fkey" FOREIGN KEY (tenant_id,
commenter_id) REFERENCES users(tenant_id, id)
"comments_tenant_id_post_id_fkey" FOREIGN KEY (tenant_id, post_id)
REFERENCES posts(tenant_id, id)
This makes the default names for foreign keys in line with the default names
for indexes. Hopefully an uncontroversial change!
The logic for creating index names is in the function ChooseIndexNameAddition
in src/backend/commands/indexcmds.c. There is also similar logic fore creating
names for statistics in ChooseExtendedStatisticNameAddition in
src/backend/commands/statscmds.c.
I pretty much just copied and pasted the implementation from
ChooseIndexNameAddition and placed it in src/backend/commands/tablecmds.c.
The new function is called ChooseForeignKeyConstraintNameAddition. I updated
the comments in indexcmds.c and statscmds.c to also reference this new function.
Each of the three versions takes in the columns in slightly different forms, so
I don't think creating a single implementation of this small bit of logic is
desirable, and I have no idea where such a util function would go.
Regression tests are in src/test/regress/sql/foreign_key.sql. I create two
composite foreign keys on table, one via the CREATE TABLE statement, and the
other in a ALTER TABLE statement. The generated names of the constraints are
then queried from the pg_constraint table.
This is my first submission to Postgres, so I'm not entirely sure what the
protocol is here to get this merged; should I add this patch to the 2019-03
Commitfest?
Happy to hear any feedback!
- Paul Martinez
Attachment
On 13/01/2019 01:55, Paul Martinez wrote: > This is my first submission to Postgres, so I'm not entirely sure what the > protocol is here to get this merged; should I add this patch to the 2019-03 > Commitfest? I haven't looked at the patch yet, but I think it's a good idea and anyway yes, please add it to the next commitfest. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Re: PATCH: Include all columns in default names for foreign keyconstraints.
From
Peter Eisentraut
Date:
On 13/01/2019 01:55, Paul Martinez wrote: > The two constraints have nearly identical names. With my patch the default names > will include both column names, so we have we will instead have this output: > > Foreign-key constraints: > "comments_tenant_id_commenter_id_fkey" FOREIGN KEY (tenant_id, > commenter_id) REFERENCES users(tenant_id, id) > "comments_tenant_id_post_id_fkey" FOREIGN KEY (tenant_id, post_id) > REFERENCES posts(tenant_id, id) > > This makes the default names for foreign keys in line with the default names > for indexes. Hopefully an uncontroversial change! I think this is a good change. > I pretty much just copied and pasted the implementation from > ChooseIndexNameAddition and placed it in src/backend/commands/tablecmds.c. The use of "name2" in the comment doesn't make sense outside the context of indexcmds.c. Maybe rewrite that a bit. > Regression tests are in src/test/regress/sql/foreign_key.sql. I create two > composite foreign keys on table, one via the CREATE TABLE statement, and the > other in a ALTER TABLE statement. The generated names of the constraints are > then queried from the pg_constraint table. Existing regression tests already exercise this, and they are failing all over the place because of the changes of the generated names. That is to be expected. You should investigate those failures and adjust the "expected" files. Then you probably don't need your additional tests. It might be worth having a test that runs into the 63-character length limit somehow. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: PATCH: Include all columns in default names for foreign key constraints.
From
Paul Martinez
Date:
Thanks for the comments! On Fri, Feb 8, 2019 at 2:11 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 13/01/2019 01:55, Paul Martinez wrote: > > I pretty much just copied and pasted the implementation from > > ChooseIndexNameAddition and placed it in src/backend/commands/tablecmds.c. > > The use of "name2" in the comment doesn't make sense outside the context > of indexcmds.c. Maybe rewrite that a bit. Updated. > > Regression tests are in src/test/regress/sql/foreign_key.sql. I create two > > composite foreign keys on table, one via the CREATE TABLE statement, and the > > other in a ALTER TABLE statement. The generated names of the constraints are > > then queried from the pg_constraint table. > > Existing regression tests already exercise this, and they are failing > all over the place because of the changes of the generated names. That > is to be expected. You should investigate those failures and adjust the > "expected" files. Then you probably don't need your additional tests. > > It might be worth having a test that runs into the 63-character length > limit somehow. Yikes, sorry about that. Some tests are failing on my machine because of dynamic linking issues and I totally missed all the foreign key failures. I think I've fixed all the tests. I changed the test I added to test the 63-character limit. Attached is an updated patch. - Paul
Attachment
Re: PATCH: Include all columns in default names for foreign keyconstraints.
From
Peter Eisentraut
Date:
On 2019-03-09 22:27, Paul Martinez wrote: > Yikes, sorry about that. Some tests are failing on my machine because of dynamic > linking issues and I totally missed all the foreign key failures. I think I've > fixed all the tests. I changed the test I added to test the 63-character limit. > > Attached is an updated patch. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services