Skip to content
This repository has been archived by the owner on Apr 2, 2024. It is now read-only.

Stop all background workers for the duration of the upgrade #290

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

cevian
Copy link
Contributor

@cevian cevian commented May 23, 2022

This commit stops bgws while upgrading the Promscale extension.
This prevents lock contention between jobs and upgrades and thus
seems safer.

@cevian cevian requested a review from a team as a code owner May 23, 2022 20:37
@cevian
Copy link
Contributor Author

cevian commented May 23, 2022

This will address some of the potential slowness seen in #274

pgnickb
pgnickb previously approved these changes May 24, 2022
Copy link
Member

@JamesGuthrie JamesGuthrie left a comment

Choose a reason for hiding this comment

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

The background workers should also be stopped at the beginning of promscale--0.0.0.sql.


-- TimescaleDB itself does the same thing:
-- https://github.com/timescale/timescaledb/blob/72d03e6f7d30cc4794c9263445f14199241e2eb5/sql/updates/pre-update.sql#L34
SELECT _timescaledb_internal.restart_background_workers();
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work without timescaledb. I know that we're planning on not supporting installations without timescaledb, but that will also require improving our test infra at least a little. I would suggest the following:

+-- stop all background workers in the current db so they don't interfere
+-- with the upgrade (by e.g. holding locks).
+--
+-- Note: this stops the scheduler (and thus all jobs)
+-- and waits for the current txn (the one doing the upgrade) to finish
+-- before starting it up again. Thus, no jobs will be running during
+-- the upgrade.
+
+-- TimescaleDB itself does the same thing:
+-- https://github.com/timescale/timescaledb/blob/72d03e6f7d30cc4794c9263445f14199241e2eb5/sql/updates/pre-update.sql#L34
+DO
+$stop_bgw$
+DECLARE
+    _is_timescaledb_installed boolean = false;
+BEGIN
+    SELECT count(*) > 0
+    INTO STRICT _is_timescaledb_installed
+    FROM pg_extension
+    WHERE extname='timescaledb';
+
+    IF _is_timescaledb_installed THEN
+        SELECT _timescaledb_internal.restart_background_workers();
+    END IF;
+END;
+$stop_bgw$;

@pgnickb pgnickb dismissed their stale review May 24, 2022 10:14

Missed some important points

@JamesGuthrie
Copy link
Member

Implementation of #291

@pgnickb
Copy link
Contributor

pgnickb commented May 25, 2022

Sorry for a stupid question, but I failed to get the answer by just glancing through the code of ts_bgw_db_workers_restart.. Is it blocking or does it always return immediately upon sending the message? And more relevantly, do we want to set a statement_timeout for this operation in case it takes too long?

@JamesGuthrie
Copy link
Member

Sorry for a stupid question, but I failed to get the answer by just glancing through the code of ts_bgw_db_workers_restart.. Is it blocking or does it always return immediately upon sending the message? And more relevantly, do we want to set a statement_timeout for this operation in case it takes too long?

It looks like it can be blocking, if the message queue to the background worker process is full. The comment on ts_bgw_message_send_and_wait (called by ts_bgw_db_workers_restart) says:

/*
 * Write element to queue, wait/error if queue is full
 * consumes message and deallocates
 */
extern bool
ts_bgw_message_send_and_wait(BgwMessageType message_type, Oid db_oid)

@cevian On another note, I tested this change out in my test environment, by adding the "stop bgw" snippet to the promscale--0.0.0.sql script. It stopped the background workers, and they were run again when the timeout was reached.

@JamesGuthrie
Copy link
Member

I also added a commit to make CI pass. Feel free to squash it down.

@JamesGuthrie
Copy link
Member

@cevian are you waiting on anything to merge this?

This commit stops bgws while upgrading the Promscale extension.
This prevents lock contention between jobs and upgrades and thus
seems safer.
@cevian
Copy link
Contributor Author

cevian commented Jun 7, 2022

@JamesGuthrie JamesGuthrie merged commit 83ec2e9 into master Jun 7, 2022
@JamesGuthrie JamesGuthrie deleted the cevian/stop_bgw branch June 7, 2022 22:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants