Fix REASSIGN OWNED BY for background jobs

Using `REASSIGN OWNED BY` for background jobs do not work because it does not change the owner of the job. This commit fixes this by capturing the utility command and makes the necessary changes to the `bgw_job` table. It also factors out background jobs DDL tests into a separate file.
timescale · Jul 29, 2024 · b443f46 · b443f46
1 parent a4a023e
commit b443f46
Show file tree

Hide file tree

Showing 8 changed files with 233 additions and 62 deletions.
diff --git a/.unreleased/pr_6987 b/.unreleased/pr_6987
@@ -0,0 +1 @@
+Fixes: #6987 Fix REASSIGN OWNED BY for background jobs
diff --git a/src/bgw/job.h b/src/bgw/job.h
@@ -73,3 +73,4 @@ extern TSDLLEXPORT void ts_bgw_job_validate_schedule_interval(Interval *schedule
 extern TSDLLEXPORT char *ts_bgw_job_validate_timezone(Datum timezone);
 
 extern TSDLLEXPORT bool ts_is_telemetry_job(BgwJob *job);
+ScanTupleResult ts_bgw_job_change_owner(TupleInfo *ti, void *data);
diff --git a/src/process_utility.c b/src/process_utility.c
@@ -24,6 +24,7 @@
 #include <commands/tablecmds.h>
 #include <commands/tablespace.h>
 #include <commands/trigger.h>
+#include <commands/user.h>
 #include <commands/vacuum.h>
 #include <miscadmin.h>
 #include <nodes/makefuncs.h>
@@ -4061,6 +4062,60 @@ process_create_rule_start(ProcessUtilityArgs *args)
 	return DDL_CONTINUE;
 }
 
+/*
+ * Update the owner of a background job given by a heap tuple.
+ *
+ * Note that there is no check for correct privileges here and this is the
+ * responsibility of the caller.
+ */
+static void
+ts_bgw_job_update_owner(Relation rel, HeapTuple tuple, TupleDesc tupledesc, Oid newrole_oid)
+{
+	bool isnull[Natts_bgw_job];
+	Datum values[Natts_bgw_job];
+	bool replace[Natts_bgw_job] = { false };
+	HeapTuple new_tuple;
+
+	heap_deform_tuple(tuple, tupledesc, values, isnull);
+
+	if (DatumGetObjectId(values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)]) != newrole_oid)
+	{
+		values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)] = Int32GetDatum(newrole_oid);
+		replace[AttrNumberGetAttrOffset(Anum_bgw_job_owner)] = true;
+		new_tuple = heap_modify_tuple(tuple, tupledesc, values, isnull, replace);
+		ts_catalog_update(rel, new_tuple);
+		heap_freetuple(new_tuple);
+	}
+}
+
+static DDLResult
+process_reassign_owned_start(ProcessUtilityArgs *args)
+{
+	ReassignOwnedStmt *stmt = (ReassignOwnedStmt *) args->parsetree;
+	List *role_ids = roleSpecsToIds(stmt->roles);
+	ScanIterator iterator =
+		ts_scan_iterator_create(BGW_JOB, RowExclusiveLock, CurrentMemoryContext);
+	ts_scanner_foreach(&iterator)
+	{
+		bool should_free, isnull;
+		TupleInfo *ti = ts_scan_iterator_tuple_info(&iterator);
+		Datum value = slot_getattr(ti->slot, Anum_bgw_job_owner, &isnull);
+		if (!isnull && list_member_oid(role_ids, DatumGetObjectId(value)))
+		{
+			Oid newrole_oid = get_rolespec_oid(stmt->newrole, false);
+			HeapTuple tuple = ts_scanner_fetch_heap_tuple(ti, false, &should_free);
+
+			/* We do not need to check privileges here since ReassignOwnedObjects() will check the
+			 * privileges and error out if they are not correct. */
+			ts_bgw_job_update_owner(ti->scanrel, tuple, ts_scanner_get_tupledesc(ti), newrole_oid);
+
+			if (should_free)
+				heap_freetuple(tuple);
+		}
+	}
+	return DDL_CONTINUE;
+}
+
 /* ALTER TABLE <name> SET ( timescaledb.compress, ...) */
 static DDLResult
 process_altertable_set_options(AlterTableCmd *cmd, Hypertable *ht)
@@ -4241,6 +4296,9 @@ process_ddl_command_start(ProcessUtilityArgs *args)
 		case T_RuleStmt:
 			handler = process_create_rule_start;
 			break;
+		case T_ReassignOwnedStmt:
+			handler = process_reassign_owned_start;
+			break;
 		case T_DropStmt:
 			/*
 			 * Drop associated metadata/chunks but also continue on to drop

diff --git a/tsl/test/expected/bgw_db_scheduler.out b/tsl/test/expected/bgw_db_scheduler.out
@@ -1619,35 +1619,6 @@ consecutive_crashes     | 0
 flags                   | 0
 
 \x off
--- Test renaming a user and see that the owner of the job changes.
-\c :TEST_DBNAME :ROLE_SUPERUSER
-CREATE USER another_user;
-SET ROLE another_user;
-SELECT insert_job('another_one', 'bgw_test_job_1', INTERVAL '100ms', INTERVAL '100s', INTERVAL '1s') AS job_id \gset
-SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
-   proc_name    |    owner     
-----------------+--------------
- bgw_test_job_1 | another_user
-(1 row)
-
-RESET ROLE;
-ALTER USER another_user RENAME TO renamed_user;
-SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
-   proc_name    |    owner     
-----------------+--------------
- bgw_test_job_1 | renamed_user
-(1 row)
-
--- This should fail since the job is dependent on the owner
-\set VERBOSITY default
-\set ON_ERROR_STOP 0
-DROP USER renamed_user;
-ERROR:  role "renamed_user" cannot be dropped because some objects depend on it
-DETAIL:  owner of job 1026
-\set ON_ERROR_STOP 1
-DELETE FROM _timescaledb_config.bgw_job WHERE id = :job_id;
--- This should succeed
-DROP USER renamed_user;
 --
 -- Test without retry
 --
@@ -1670,7 +1641,7 @@ DELETE FROM _timescaledb_config.bgw_job;
 INSERT INTO _timescaledb_config.bgw_job(application_name, schedule_interval, max_runtime, max_retries, retry_period, proc_schema, proc_name) VALUES('bgw_test_job_2_error', INTERVAL '5000ms', INTERVAL '20ms', 0, INTERVAL '20ms', 'public', 'bgw_test_job_2_error') RETURNING id;
   id  
 ------
- 1027
+ 1026
 (1 row)
 
 \c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER
@@ -1684,16 +1655,16 @@ SELECT ts_bgw_db_scheduler_test_run_and_wait_for_scheduler_finish(25);
 SELECT job_id, last_run_success, total_runs, total_successes, total_failures, total_crashes FROM _timescaledb_internal.bgw_job_stat;
  job_id | last_run_success | total_runs | total_successes | total_failures | total_crashes 
 --------+------------------+------------+-----------------+----------------+---------------
-   1027 | f                |          1 |               0 |              1 |             0
+   1026 | f                |          1 |               0 |              1 |             0
 (1 row)
 
 SELECT * FROM sorted_bgw_log;
  msg_no |   application_name   |                            msg                            
 --------+----------------------+-----------------------------------------------------------
       0 | DB Scheduler         | [TESTING] Registered new background worker
       1 | DB Scheduler         | [TESTING] Wait until (RANDOM), started at (RANDOM)
-      1 | bgw_test_job_2_error | job 1027 reached max_retries after 1 consecutive failures
-      2 | bgw_test_job_2_error | job 1027 threw an error
+      1 | bgw_test_job_2_error | job 1026 reached max_retries after 1 consecutive failures
+      2 | bgw_test_job_2_error | job 1026 threw an error
       3 | bgw_test_job_2_error | Error job 2
       2 | DB Scheduler         | [TESTING] Wait until (RANDOM), started at (RANDOM)
 (6 rows)
@@ -1714,7 +1685,7 @@ SELECT ts_bgw_db_scheduler_test_run_and_wait_for_scheduler_finish(100, 50);
 SELECT job_id, last_run_success, total_runs, total_successes, total_failures, total_crashes FROM _timescaledb_internal.bgw_job_stat;
  job_id | last_run_success | total_runs | total_successes | total_failures | total_crashes 
 --------+------------------+------------+-----------------+----------------+---------------
-   1027 | f                |          1 |               0 |              1 |             0
+   1026 | f                |          1 |               0 |              1 |             0
 (1 row)
 
 -- We increase the mock time a lot to ensure the job does not get restarted. However, the amount of scheduler sleep/wakeup cycles
@@ -1723,8 +1694,8 @@ SELECT * FROM sorted_bgw_log WHERE msg NOT LIKE '[TESTING] Wait until%';
  msg_no |   application_name   |                            msg                            
 --------+----------------------+-----------------------------------------------------------
       0 | DB Scheduler         | [TESTING] Registered new background worker
-      1 | bgw_test_job_2_error | job 1027 reached max_retries after 1 consecutive failures
-      2 | bgw_test_job_2_error | job 1027 threw an error
+      1 | bgw_test_job_2_error | job 1026 reached max_retries after 1 consecutive failures
+      2 | bgw_test_job_2_error | job 1026 threw an error
       3 | bgw_test_job_2_error | Error job 2
 (4 rows)
 

diff --git a/tsl/test/expected/bgw_job_ddl.out b/tsl/test/expected/bgw_job_ddl.out
@@ -0,0 +1,91 @@
+-- This file and its contents are licensed under the Timescale License.
+-- Please see the included NOTICE for copyright information and
+-- LICENSE-TIMESCALE for a copy of the license.
+-- Test for DDL-like functionality
+\c :TEST_DBNAME :ROLE_SUPERUSER
+CREATE OR REPLACE FUNCTION insert_job(
+       application_name NAME,
+       job_type NAME,
+       schedule_interval INTERVAL,
+       max_runtime INTERVAL,
+       retry_period INTERVAL,
+       owner regrole DEFAULT CURRENT_ROLE::regrole,
+       scheduled BOOL DEFAULT true,
+       fixed_schedule BOOL DEFAULT false
+) RETURNS INT LANGUAGE SQL SECURITY DEFINER AS
+$$
+  INSERT INTO _timescaledb_config.bgw_job(application_name,schedule_interval,max_runtime,max_retries,
+  retry_period,proc_name,proc_schema,owner,scheduled,fixed_schedule)
+  VALUES($1,$3,$4,5,$5,$2,'public',$6,$7,$8) RETURNING id;
+$$;
+CREATE USER another_user;
+SET ROLE another_user;
+SELECT insert_job('another_one', 'bgw_test_job_1', INTERVAL '100ms', INTERVAL '100s', INTERVAL '1s') AS job_id \gset
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |    owner     
+----------------+--------------
+ bgw_test_job_1 | another_user
+(1 row)
+
+-- Test that reassigning to another user privileges does not work for
+-- a normal user. We test both users with superuser privileges and
+-- default permissions.
+\set ON_ERROR_STOP 0
+REASSIGN OWNED BY another_user TO :ROLE_CLUSTER_SUPERUSER;
+ERROR:  permission denied to reassign objects
+REASSIGN OWNED BY another_user TO :ROLE_DEFAULT_PERM_USER;
+ERROR:  permission denied to reassign objects
+\set ON_ERROR_STOP 1
+RESET ROLE;
+-- Test that renaming a user changes keeps the job assigned to that user.
+ALTER USER another_user RENAME TO renamed_user;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |    owner     
+----------------+--------------
+ bgw_test_job_1 | renamed_user
+(1 row)
+
+\set VERBOSITY default
+\set ON_ERROR_STOP 0
+-- Test that dropping a user owning a job fails.
+DROP USER renamed_user;
+ERROR:  role "renamed_user" cannot be dropped because some objects depend on it
+DETAIL:  owner of job 1000
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |    owner     
+----------------+--------------
+ bgw_test_job_1 | renamed_user
+(1 row)
+
+-- Test that re-assigning objects owned by an unknown user still fails
+REASSIGN OWNED BY renamed_user, unknown_user TO :ROLE_DEFAULT_PERM_USER;
+ERROR:  role "unknown_user" does not exist
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |    owner     
+----------------+--------------
+ bgw_test_job_1 | renamed_user
+(1 row)
+
+\set ON_ERROR_STOP 1
+-- Test that reassigning the owned job actually changes the owner of
+-- the job.
+START TRANSACTION;
+REASSIGN OWNED BY renamed_user TO :ROLE_DEFAULT_PERM_USER;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |       owner       
+----------------+-------------------
+ bgw_test_job_1 | default_perm_user
+(1 row)
+
+ROLLBACK;
+-- Test that reassigning to postgres works
+REASSIGN OWNED BY renamed_user TO :ROLE_CLUSTER_SUPERUSER;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+   proc_name    |       owner        
+----------------+--------------------
+ bgw_test_job_1 | cluster_super_user
+(1 row)
+
+-- Dropping the user now should work.
+DROP USER renamed_user;
+DELETE FROM _timescaledb_config.bgw_job WHERE id = :job_id;
diff --git a/tsl/test/sql/CMakeLists.txt b/tsl/test/sql/CMakeLists.txt
@@ -6,6 +6,7 @@ set(TEST_FILES
     agg_partials_pushdown.sql
     bgw_security.sql
     bgw_policy.sql
+    bgw_job_ddl.sql
     cagg_deprecated_bucket_ng.sql
     cagg_errors.sql
     cagg_invalidation.sql

diff --git a/tsl/test/sql/bgw_db_scheduler.sql b/tsl/test/sql/bgw_db_scheduler.sql
@@ -674,32 +674,6 @@ SELECT * FROM sorted_bgw_log;
 SELECT * FROM _timescaledb_internal.bgw_job_stat;
 \x off
 
--- Test renaming a user and see that the owner of the job changes.
-\c :TEST_DBNAME :ROLE_SUPERUSER
-CREATE USER another_user;
-
-SET ROLE another_user;
-SELECT insert_job('another_one', 'bgw_test_job_1', INTERVAL '100ms', INTERVAL '100s', INTERVAL '1s') AS job_id \gset
-
-SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
-
-RESET ROLE;
-ALTER USER another_user RENAME TO renamed_user;
-
-SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
-
--- This should fail since the job is dependent on the owner
-\set VERBOSITY default
-\set ON_ERROR_STOP 0
-DROP USER renamed_user;
-\set ON_ERROR_STOP 1
-
-DELETE FROM _timescaledb_config.bgw_job WHERE id = :job_id;
-
--- This should succeed
-DROP USER renamed_user;
-
-
 --
 -- Test without retry
 --

diff --git a/tsl/test/sql/bgw_job_ddl.sql b/tsl/test/sql/bgw_job_ddl.sql
@@ -0,0 +1,74 @@
+-- This file and its contents are licensed under the Timescale License.
+-- Please see the included NOTICE for copyright information and
+-- LICENSE-TIMESCALE for a copy of the license.
+
+-- Test for DDL-like functionality
+
+\c :TEST_DBNAME :ROLE_SUPERUSER
+CREATE OR REPLACE FUNCTION insert_job(
+       application_name NAME,
+       job_type NAME,
+       schedule_interval INTERVAL,
+       max_runtime INTERVAL,
+       retry_period INTERVAL,
+       owner regrole DEFAULT CURRENT_ROLE::regrole,
+       scheduled BOOL DEFAULT true,
+       fixed_schedule BOOL DEFAULT false
+) RETURNS INT LANGUAGE SQL SECURITY DEFINER AS
+$$
+  INSERT INTO _timescaledb_config.bgw_job(application_name,schedule_interval,max_runtime,max_retries,
+  retry_period,proc_name,proc_schema,owner,scheduled,fixed_schedule)
+  VALUES($1,$3,$4,5,$5,$2,'public',$6,$7,$8) RETURNING id;
+$$;
+
+CREATE USER another_user;
+
+SET ROLE another_user;
+SELECT insert_job('another_one', 'bgw_test_job_1', INTERVAL '100ms', INTERVAL '100s', INTERVAL '1s') AS job_id \gset
+
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+-- Test that reassigning to another user privileges does not work for
+-- a normal user. We test both users with superuser privileges and
+-- default permissions.
+\set ON_ERROR_STOP 0
+REASSIGN OWNED BY another_user TO :ROLE_CLUSTER_SUPERUSER;
+REASSIGN OWNED BY another_user TO :ROLE_DEFAULT_PERM_USER;
+\set ON_ERROR_STOP 1
+
+RESET ROLE;
+
+-- Test that renaming a user changes keeps the job assigned to that user.
+ALTER USER another_user RENAME TO renamed_user;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+\set VERBOSITY default
+\set ON_ERROR_STOP 0
+
+-- Test that dropping a user owning a job fails.
+DROP USER renamed_user;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+-- Test that re-assigning objects owned by an unknown user still fails
+REASSIGN OWNED BY renamed_user, unknown_user TO :ROLE_DEFAULT_PERM_USER;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+\set ON_ERROR_STOP 1
+
+-- Test that reassigning the owned job actually changes the owner of
+-- the job.
+START TRANSACTION;
+REASSIGN OWNED BY renamed_user TO :ROLE_DEFAULT_PERM_USER;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+ROLLBACK;
+
+-- Test that reassigning to postgres works
+REASSIGN OWNED BY renamed_user TO :ROLE_CLUSTER_SUPERUSER;
+SELECT proc_name, owner FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+-- Dropping the user now should work.
+DROP USER renamed_user;
+
+DELETE FROM _timescaledb_config.bgw_job WHERE id = :job_id;
+
+