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

Fix infinite recursion for schema_list_extensions() catalog query #752

Merged

Conversation

kbarber
Copy link
Contributor

@kbarber kbarber commented Apr 19, 2024

Fixes: #749

This PR: #702 introduced important ordering for extension installation that took into account relationships between extension configuration tables.

The new query executed by schema_list_extensions() seems to have an infinite loop. You can reproduce this by running a pgcopydb clone with the follow tables:

create table one (id int, two_id int);
create table two (id int, one_id int, three_id int);
create table three (id int);
alter table one add constraint one_pk primary key (id);
alter table two add constraint two_pk primary key (id);
alter table three add constraint three_pk primary key (id);
alter table one add constraint one_two_fk foreign key (two_id) references two(id);
alter table two add constraint two_one_fk foreign key (one_id) references one(id);
alter table two add constraint two_three_fk foreign key (three_id) references three(id);

This patch attempts to remove the loop by adding cycle detection as documented as a pattern here: https://www.postgresql.org/docs/current/queries-with.html#QUERIES-WITH-CYCLE it also adds the above tables & FK creations to extensions/copydb.sh to test for regression in the future.

@kbarber
Copy link
Contributor Author

kbarber commented Apr 19, 2024

@JamesGuthrie if you have any strong feelings about this fix, or a better idea please let me know :-). I'm worried the cyclic detection might affect the depth, but I think the new change probably corrects the depth now its not infinite.

I was thinking of adding the tests that James described here: #701 to avoid regression since its a complex query. Looks like it needs the TimescaleDB extension.

dimitri
dimitri previously approved these changes Apr 19, 2024
Copy link
Owner

@dimitri dimitri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I think we could maybe improve on the spelling of the test case, so while I could merge as-is, if you think my proposal is an improvement would you be up for another revision before merge?

Comment on lines 39 to 47
create table one (id int, two_id int);
create table two (id int, one_id int, three_id int);
create table three (id int);
alter table one add constraint one_pk primary key (id);
alter table two add constraint two_pk primary key (id);
alter table three add constraint three_pk primary key (id);
alter table one add constraint one_two_fk foreign key (two_id) references two(id);
alter table two add constraint two_one_fk foreign key (one_id) references one(id);
alter table two add constraint two_three_fk foreign key (three_id) references three(id);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we inline the pkey and fkey definitions into the CREATE TABLE statements? I think it's easier to read. It needs be, let's just add another SQL file to that test unit (such as ddl.sql as found in other units) to have more flexibility in the SQL syntax/layout.

@dimitri dimitri added the bug Something isn't working label Apr 19, 2024
@dimitri dimitri added this to the v0.16 milestone Apr 19, 2024
@kbarber kbarber force-pushed the fix_inf_recursion_in_schema_list_extensions branch from 852ccb2 to b0a74c4 Compare April 19, 2024 14:50
@kbarber kbarber marked this pull request as ready for review April 19, 2024 14:53
@kbarber
Copy link
Contributor Author

kbarber commented Apr 19, 2024

@dimitri 'twas a fun problem, let me know if you need anything else on this one.

tests/extensions/ddl.sql Show resolved Hide resolved
@dimitri dimitri merged commit 9a151a6 into dimitri:main Apr 20, 2024
18 checks passed
arajkumar pushed a commit to arajkumar/pgcopydb that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERROR: out of memory when calling schema_list_extensions()
2 participants