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

To 0.5 #65

Merged
merged 8 commits into from
Sep 5, 2022
Merged

To 0.5 #65

merged 8 commits into from
Sep 5, 2022

Conversation

steve-chavez
Copy link
Member

Small improvements to make pg_net a bit more stable.

Closes #61 and #44.

Also reducing unnecessary work done by pg_net to address cases where there is high amount of requests - ref(slack link)

To remove the possibility of them being used as FKs

Fixes supabase#44
To reduce CPU usage, keep the check_worker_is_up function for debugging.
The index doesn't get used on the DELETE FROM net._http_response in
worker.c anyway because `clock_timestamp()` forces a Seq Scan.
Their contents shouldn't survive a crash, and UNLOGGED makes the
INSERTs/DELETEs faster and consume less CPU.
soedirgo
soedirgo previously approved these changes Sep 1, 2022
@soedirgo
Copy link
Member

soedirgo commented Sep 1, 2022

Btw supabase_functions.hooks has hooks_response_id_fkey which depends on net.http_request_queue_pkey. Can that be handled in a migration?

@soedirgo soedirgo dismissed their stale review September 1, 2022 05:03

issues w/ upgrade script

@soedirgo
Copy link
Member

soedirgo commented Sep 1, 2022

Some issues with the 0.4 -> 0.5 upgrade script:

  • dropping pkey constraint
ERROR:  cannot drop constraint http_request_queue_pkey on table net.http_request_queue because other objects depend on it
DETAIL:  constraint _http_response_id_fkey on table net._http_response depends on index net.http_request_queue_pkey
constraint hooks_request_id_fkey on table supabase_functions.hooks depends on index net.http_request_queue_pkey
  • _check_worker_is_up rename: return type should be void instead of trigger - might need to drop + create the function
  • drop index
ERROR:  index "_http_response_created_idx" does not exist
  • set unlogged
ERROR:  could not change table "http_request_queue" to unlogged because it references logged table "_http_response"
ERROR:  could not change table "http_request_queue" to unlogged because it references logged table "hooks"

@steve-chavez
Copy link
Member Author

steve-chavez commented Sep 1, 2022

Edit: Nevermind, I managed to reproduce it on the dashboard, I'll just add CASCADE to the script.

dropping pkey constraint

Current queue doesn't have a FK

pg_net/sql/pg_net.sql

Lines 42 to 43 in deed712

create table net._http_response(
id bigint primary key,

This was removed on 8538d83#diff-539e81249472b5ef91e28038ee011b645c8885f37ed6679e34d4483e23f4d390L46-R43

Already included on a previous script

alter table net._http_response drop constraint _http_response_id_fkey;

How are you testing the upgrade script? I can't reproduce the error

@steve-chavez
Copy link
Member Author

_check_worker_is_up rename: return type should be void instead of trigger - might need to drop + create the function

drop index

Added IF EXISTS

set unlogged

These now work.

@steve-chavez
Copy link
Member Author

steve-chavez commented Sep 2, 2022

I created a new supabase instance(has pg_net 0.2), enabled a webhook and did:

alter extension pg_net update to '0.3';
alter extension pg_net update to '0.4';
alter extension pg_net update to '0.5'; -- had to manually download pg_net--0.4--0.5.sql to /var/lib/postgresql/extension

And all of them ran successfully. Although I had to fix pg_net--0.2--0.3.sql(included on the latest commit).


It would have been good to have pgtap testing for migrations but doesn't seem worth it for now(not sure what pgx will change).

Copy link
Member

@soedirgo soedirgo left a comment

Choose a reason for hiding this comment

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

One more thing:

sql/pg_net--0.2--0.3.sql Outdated Show resolved Hide resolved
@steve-chavez steve-chavez merged commit 824b163 into supabase:master Sep 5, 2022
@steve-chavez steve-chavez mentioned this pull request Sep 6, 2022
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.

pg_net causes database to crash if url is null
2 participants