Implement event trigger to remove jobs from vectorize.job upon table deletion

[varshith257]

/claim #148
Closes #148

[varshith257]

@ChuckHend Can have a review on this PR?

[varshith257]

It seems test fails are not related of changes in this PR?

Any changes needed or can be merged @ChuckHend ?

[ChuckHend]

@varshith257 yes these tests will fail for any branch that isnt part of the org (due to security policy on the repo). It is because there is an API key and it is in a github secret. We can ignore those tests.

[varshith257]

@ChuckHend Can it be merged?

[ChuckHend]

Sorry we haven't had a chance to do a thorough review yet. Can you add a test that asserts the records are indeed dropped?

[varshith257]

Sure!

[varshith257]

@ChuckHend Added tests! Can you review this PR?

[varshith257]

@ChuckHend Can it be merged? Or any changes needed?

[ChuckHend]

Left some more specific comments in-line.

This needs to work when the job name is != to the table name, and also needs to consider the schema of the table.

[varshith257]

@ChuckHend Done!

[ChuckHend]

@varshith257, the test that was added in this PR does not compile.

error[E0282]: type annotations needed
   --> tests/integration_tests.rs:96:9
    |
96  |     let job_exists = sqlx::query_scalar(&format!(
    |         ^^^^^^^^^^
...
104 |         !job_exists,
    |         ----------- type must be known at this point
    |
help: consider giving `job_exists` an explicit type
    |
96  |     let job_exists: /* Type */ = sqlx::query_scalar(&format!(
    |                   ++++++++++++

[varshith257]

@ChuckHend Sorry! I have modified it locally and I didn't pushed here lol 🤦

[varshith257]

@ChuckHend Can you approve workflows again?

[varshith257]

@ChuckHend Are any changes needed? Let me know, I am ready to incorporate them and get this PR merged soon :)

[ChuckHend]

This should be a select vectorize.table(...) instead, so that the test stays current with the APIs around that table.

[ChuckHend]

@varshith257 - i think replace this direct insert with vectorize.table(). It should help the test move along as it is currently failing because the search_alg column no longer exists (it was removed from the project).

[ChuckHend]

The test_drop_table_triggers_job_deletion fails when I run locally.

make test-integration TEST_NAME=test_drop_table_triggers_job_deletion


test test_drop_table_triggers_job_deletion ... URL: postgres://user:pass@localhost:28817/postgres
thread 'test_drop_table_triggers_job_deletion' panicked at tests/integration_tests.rs:89:5:
assertion `left == right` failed: Job was not deleted after table drop
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
FAILED

I'm going to push a change to the CI workflow so that the failing ./core tests dont block the extension tests from running.

[varshith257]

@ChuckHend I am debugging if the trigger event is triggered or something is blocking in test for deletion

[varshith257]

@ChuckHend Done! It's just my initial wrong approach to accessing the table and schema. Now it's working fine. It's up for you review

test test_drop_table_triggers_job_deletion ... URL: ***localhost:28817/postgres
ok

[akhilender-bongirwar]

@varshith257 your recent commit "fix tests" contains changes that are very similar to what I implemented in the recent commits in my PR #178 😂 .

[varshith257]

Hi @akhilender-bongirwar, I’d like to clarify that this implementation was developed independently to address the issue requirements. My earlier approach used joins with pg_class and pg_namespace but after further testing and debugging, I found that directly iterating over pg_event_trigger_dropped_objects() and parsing object_identity was the most reliable and efficient approach. You can check with the discussions between me and @ChuckHend

In scenarios like this, where we work with PostgreSQL’s built-in functions and constraints, it’s natural for solutions to align and appear similar.

[akhilender-bongirwar]

In scenarios like this, where we work with PostgreSQL’s built-in functions and constraints, it’s natural for solutions to align and appear similar.

However, as we both understand, this process operates on a first-come, first-served basis, as @ChuckHend previously mentioned. Since my PR was completed first and is currently under review, you can refer to the comments under my PR for further context.

[varshith257]

this process operates on a first-come, first-served basis, as @ChuckHend previously mentioned.

FYI, that mean the PR first opened and if it goes inactive completely or closed then the second contributor gets a chance

[akhilender-bongirwar]

FYI, that mean the PR first opened and if it goes inactive completely or closed then the second contributor gets a chance

FYI, this PR was inactive for a month, and it's quite surprising that similar changes appeared just a day after I submitted mine. @varshith257, let’s be fair here. I trust @ChuckHend to make the final call, and I’m confident he recognizes the efforts put in and the order in which they were made.

[varshith257]

See for a review of @ChuckHend of my PR. And the PR is inactive because of my exams and I have clearly communicated to the reviewer in Slack. Just I am triggering you that inactivity of raising for next PR not the one which is open. I don't wanna argue anymore it's final decision of @ChuckHend and this PR is opened 2 months ago and have been active last 23 days ago

[ChuckHend]

There are two commits that look similar to me.

@akhilender-bongirwar 5ba8c03 made on 09-Dec-2024
@varshith257 f211b32 made on 10-Dec-2024

Both of these look like good solutions to me. Is there anything I am missing here, it seems like @akhilender-bongirwar has the solution first.

[akhilender-bongirwar]

There are two commits that look similar to me.

@akhilender-bongirwar 5ba8c03 made on 09-Dec-2024 @varshith257 f211b32 made on 10-Dec-2024

Both of these look like good solutions to me. Is there anything I am missing here, it seems like @akhilender-bongirwar has the solution first.

Thank you for reviewing the commits, @ChuckHend. I appreciate the acknowledgment that my solution was implemented first. I believe both commits aim to address the issue effectively, but as per the first come, first serve guideline, I trust your judgment to finalize the PR based on the timeline and contributions. Happy to make any adjustments if needed!

[varshith257]

Closing as fixed by #178