fix: bound request timeouts - #285
utkarash2991 wants to merge 2 commits into
Conversation
6e47ab2 to
b465078
Compare
| assert execinfo.value.orig.diag.message_primary == CHECK_VIOLATION | ||
|
|
||
|
|
||
| def test_worker_clamps_timeout_of_rows_queued_before_the_bound(sess): |
There was a problem hiding this comment.
In future we could also think of having CI tests in which we run upgrades and assert on properties we want upgrades to uphold.
There was a problem hiding this comment.
I agree with this. I would much rather the tests do something like:
- install older version of extension
- setup (insert rows, whatever)
- upgrade to latest or target version
- run tests
as opposed to cherry pick aspects of the migration script in the pytest code.
|
|
||
|
|
||
| @pytest.mark.parametrize("timeout", [0, -1, 600001]) | ||
| def test_direct_insert_rejects_timeout_out_of_range(sess, timeout): |
There was a problem hiding this comment.
Is it necessary to test inserting into table and using the convenience function? Seems to me like they test the same code paths and we can just test the function to test the entire execution.
There was a problem hiding this comment.
YEs they are testing the same code path. I just added it for sake of completeness purpose.
There was a problem hiding this comment.
IMO if they test the same code path it's not buying us anything other than slower testing times, especially since we can't run these tests in parallel (or at least its not documented).
| handle->ez_handle = curl_easy_init(); | ||
|
|
||
| handle->timeout_milliseconds = row.timeout_milliseconds; | ||
| if (handle->timeout_milliseconds <= 0 || handle->timeout_milliseconds > MAX_TIMEOUT_MS) { |
There was a problem hiding this comment.
I'm a bit confused about this. The upgrade scripts seems to make this impossible via the below statement, does it not? Personally I think I would prefer to handle this at the SQL level. Are we concerned about a mismatch between the c .so and the customer not upgrading the in database extension? IMO we should try to not expose the customer to breaking changes without them running upgrade on the extension.
update net.http_request_queue
set timeout_milliseconds = net._max_timeout_ms()
where timeout_milliseconds <= 0 or timeout_milliseconds > net._max_timeout_ms();
There was a problem hiding this comment.
@AndrewJackson2020 we need this change as because the binary and the extension SQL don't move together. The .so is replaced whenever the image or package is upgraded but the constraint only appears when we run ALTER EXTENSION UPDATE . So there's a window where the constraint doesn't exist yet and the new worker still has to deal with out-of-range rows.
It also matters for the hung projects this PR is about. After a worker restart the 0-timeout row is back in the queue and the worker picks it up again, so the upgrade script can't even take its lock until that batch finishes. With this line the batch finishes, and then the upgrade can run.
Regarding the breaking changes, the condition here just tries to ensure that the batch finishes, which otherwise can go to hung state forever.
There was a problem hiding this comment.
That's fine. I did not realize upgrading the extension was not possible under those circumstances. I would just put a comment in. This logic presumably can and should be removed in the future (after we have migrated instances off of the old version).
There was a problem hiding this comment.
The .so is replaced whenever the image or package is upgraded but the constraint only appears when we run ALTER EXTENSION UPDATE
The above would be a non-issue if we go ahead with a GUC as mentioned above.
There was a problem hiding this comment.
Added a GUC in the latest commit.
|
@utkarash2991 can you make sure this is rebased so that it is picking up the new CI checks? |
11737f7 to
c13d386
Compare
| @pytest.mark.parametrize("timeout", [0, -1, 2147483647]) | ||
| def test_worker_bounds_out_of_range_timeouts(conn, timeout): | ||
| """A 0, negative or oversized timeout is clamped to pg_net.max_timeout_ms instead of hanging the worker""" | ||
|
|
||
| request_id = pg_http_request( | ||
| conn, | ||
| "select net.http_get(url := 'http://localhost:8080/pathological?status=200', timeout_milliseconds := %s)", | ||
| (timeout,), | ||
| ) | ||
|
|
||
| response = pg_collect_response(conn, request_id) | ||
|
|
||
| assert response["status"] == "SUCCESS" |
There was a problem hiding this comment.
I think we should refine this. Two options:
- Just fail when the timeout surpasses the GUC one. IMO the best.
- Silently clamp it as it's done now, but log a warning.
4 participants
We pass
timeout_millisecondsstraight toCURLOPT_TIMEOUT_MSand libcurl treats 0 as no timeout. So a request with 0 timeout to a server which accepts the connection but never responds blocks the whole batch loop and the worker never comes out of it.net.worker_restart()also doesn't help here as the restart flag is only checked between batches.What this PR does:
pg_net.max_timeout_ms(default 600000,PGC_SUSET).1..pg_net.max_timeout_ms: it is not sent and gets anERRORresponse with the messagetimeout_milliseconds must be between 1 and <max> (pg_net.max_timeout_ms), got <value>. Same shape as any other per-request failure (bad URL, curl error). Done in C only, no SQL changes and no migration, and it covers requests inserted directly into the queue as well.Behaviour change: a 0 or negative or oversized timeout used to hang the worker (negative ones made
curl_easy_setoptfail and crash it). Now the request fails with an error response and the batch carries on. Everything in range is untouched.Tests (psycopg): 0, negative and oversized timeouts get the error response with the exact message; lowering
pg_net.max_timeout_msviaalter system+ reload changes the bound in the message; non-superusers can't set it.