fix: remove foreign key, make trigger handle deletes #485
Conversation
Askir
force-pushed
the
jascha/ai-459-possible-bug-in-event-trigger
branch
2 times, most recently
from
2f3cf22 to
b4ccdc1
Compare
Askir
force-pushed
the
jascha/ai-459-possible-bug-in-event-trigger
branch
from
3e3b196 to
1293d44
Compare
| -- _build_vectorizer_trigger_definition | ||
| create or replace function ai._build_vectorizer_trigger_definition |
There was a problem hiding this comment.
I'd probably call this ai._vectorizer_build_trigger_definition so as to fit the pattern
| if (TG_OP = 'DELETE') then | ||
| execute '$DELETE_STATEMENT$' using old; | ||
| return old; | ||
| elsif (TG_OP = 'UPDATE') then | ||
| if $PK_CHANGE_CHECK$ then | ||
| execute '$DELETE_STATEMENT$' using old; | ||
| end if; |
There was a problem hiding this comment.
Is there a reason for using EXECUTE here instead of a "plain" DELETE statement like the INSERT statement below?
| quote_ident(attname) operator(pg_catalog.||) ' = $1.' operator(pg_catalog.||) quote_ident(attname), | ||
| ' and ' |
There was a problem hiding this comment.
Do you prefer using the concatenate operator instead of the format function? It's a matter of taste, but I find format a little easier to read and understand what the output is supposed to look like. But this works
| insert into $QUEUE_SCHEMA$.$QUEUE_TABLE$ ($PK_COLUMNS$) | ||
| values ($PK_VALUES$); | ||
| return new; | ||
| else |
There was a problem hiding this comment.
Let's go ahead and handle when TG_OP = 'TRUNCATE'. We can just truncate the queue and the target.
There was a problem hiding this comment.
hmm. Truncate may require a separate trigger. I'm not sure if you can do a for each row trigger on a truncate op. If it requires a separate trigger, I guess let's skip it.
There was a problem hiding this comment.
oh. i bet you could have two triggers with one trigger function.
| _func_def := replace(_func_def, '$DELETE_STATEMENT$', _delete_statement); | ||
| _func_def := replace(_func_def, '$PK_CHANGE_CHECK$', _pk_change_check); | ||
| _func_def := replace(_func_def, '$QUEUE_SCHEMA$', quote_ident(queue_schema)); | ||
| _func_def := replace(_func_def, '$QUEUE_TABLE$', quote_ident(queue_table)); | ||
| _func_def := replace(_func_def, '$PK_COLUMNS$', _pk_columns); | ||
| _func_def := replace(_func_def, '$PK_VALUES$', _pk_values); |
There was a problem hiding this comment.
I'm guessing you chose this over format because format doesn't have named fields? It does have positional fields, but that's still not ideal.
| update ai.vectorizer | ||
| set config = jsonb_set(config, '{version}', format('"%s"', _new_version)::jsonb) | ||
| where id = _vec.id; |
There was a problem hiding this comment.
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.
| do $upgrade_block$ | ||
| declare | ||
| _vec record; | ||
| _version text; |
| _owner name; | ||
| _acl aclitem[]; | ||
| _debug_acl aclitem[]; |
| execute format( | ||
| 'alter extension ai add function ai.%I()', | ||
| _vec.trigger_name | ||
| ); |
There was a problem hiding this comment.
Did you verify that adding it to the extension does not change the ownership of the function?
I probably would have altered the function to switch owner and then switch back, but this seems like it might be better or at least equally good.
No description provided.