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: remove foreign key, make trigger handle deletes #485

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Askir
Copy link
Contributor

@Askir Askir commented Feb 14, 2025

No description provided.

@Askir Askir force-pushed the jascha/ai-459-possible-bug-in-event-trigger branch 2 times, most recently from 2f3cf22 to b4ccdc1 Compare February 15, 2025 01:34
@Askir Askir force-pushed the jascha/ai-459-possible-bug-in-event-trigger branch from 3e3b196 to 1293d44 Compare February 20, 2025 21:07
@Askir Askir marked this pull request as ready for review February 20, 2025 22:27
@Askir Askir requested a review from a team as a code owner February 20, 2025 22:27
Copy link
Collaborator

@jgpruitt jgpruitt left a comment

Choose a reason for hiding this comment

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

nice!

Comment on lines +651 to +653
update ai.vectorizer
set config = jsonb_set(config, '{version}', format('"%s"', _new_version)::jsonb)
where id = _vec.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to make sure we don't have any other config upgrades in this release or this may cause the other(s) to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only if they rely on the version being pre 0.8.1 right? Or is there another concern, I don't understand?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i just mean if we have a second upgrade like this in the same release that this one goes out in, then we may have an issue because the other upgrade would also be looking for vectorizers with version < 0.8.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@Askir Askir requested a review from jgpruitt February 26, 2025 10:13
@@ -126,7 +126,6 @@ begin
, chunk text not null
, embedding @extschema:[email protected](%L) storage main not null
, unique (%s, chunk_seq)
, foreign key (%s) references %I.%I (%s) on delete cascade
Copy link
Collaborator

Choose a reason for hiding this comment

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

source_schema and source_table parameters are no longer required/used.


elsif (TG_LEVEL = 'STATEMENT') then
if (TG_OP = 'TRUNCATE') then
execute format('truncate table %I.%I', '$TARGET_SCHEMA$', '$TARGET_TABLE$');
Copy link
Collaborator

Choose a reason for hiding this comment

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

truncate the queue table too


return _func_def;
end;
$func$ language plpgsql stable security invoker
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$func$ language plpgsql stable security invoker
$func$ language plpgsql immutable security invoker

);

execute format(
'create trigger %I after insert or update or delete on %I.%I for each row execute function ai.%I()',
Copy link
Collaborator

Choose a reason for hiding this comment

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

ai -> queue_schema

Comment on lines +659 to +662
execute format(
'create trigger %I after truncate on %I.%I for each statement execute function ai.%I()',
format('%s_truncate',_vec.trigger_name) , _vec.source_schema, _vec.source_table, _vec.trigger_name
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ai -> queue_schema

Comment on lines +664 to +667
execute format(
'alter extension ai drop function ai.%I()',
_vec.trigger_name
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ai -> queue_schema

Comment on lines +651 to +653
update ai.vectorizer
set config = jsonb_set(config, '{version}', format('"%s"', _new_version)::jsonb)
where id = _vec.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i just mean if we have a second upgrade like this in the same release that this one goes out in, then we may have an issue because the other upgrade would also be looking for vectorizers with version < 0.8.1

FROM ai.vectorizer v
LOOP
-- Find the foreign key constraint for this vectorizer's store table
SELECT conname INTO _constraint_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

in theory there could be multiple foreign key constraints here, but this would only happen if the users did so manually, and that would be very weird. so, i'm okay with the assumption

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.

2 participants