Use regrole for job owner

Instead of using a user name to register the owner of a job, we use regrole. This allows renames to work properly since the underlying OID does not change when the owner name changes. We add a check when calling `DROP ROLE` that there is no job with that owner and generate an error if there is.
timescale · Apr 18, 2023 · 9a64385 · 9a64385
1 parent 20db884
commit 9a64385
Show file tree

Hide file tree

Showing 23 changed files with 334 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -39,6 +39,7 @@ accidentally triggering the load of a previous DB version.**
 * #5433 Fix join rte in CAggs with joins
 * #5543 Copy scheduled_jobs list before sorting it
 * #5570 Improve interpolate error message on datatype mismatch
+* #5558 Use regrole for job owner
 
 **Thanks**
 * @nikolaps for reporting an issue with the COPY fetcher

diff --git a/sql/job_error_log_retention.sql b/sql/job_error_log_retention.sql
@@ -65,7 +65,7 @@ VALUES
     INTERVAL '1h',
     '_timescaledb_internal',
     'policy_job_error_retention',
-    CURRENT_ROLE,
+    current_role::regrole,
     true,
     '{"drop_after":"1 month"}',
     '_timescaledb_internal',

diff --git a/sql/pre_install/tables.sql b/sql/pre_install/tables.sql
@@ -280,7 +280,7 @@ CREATE TABLE _timescaledb_config.bgw_job (
   retry_period interval NOT NULL,
   proc_schema name NOT NULL,
   proc_name name NOT NULL,
-  owner name NOT NULL DEFAULT CURRENT_ROLE,
+  owner regrole NOT NULL DEFAULT current_role::regrole,
   scheduled bool NOT NULL DEFAULT TRUE,
   fixed_schedule bool not null default true,
   initial_start timestamptz,

diff --git a/sql/updates/latest-dev.sql b/sql/updates/latest-dev.sql
@@ -57,3 +57,74 @@ ALTER FUNCTION _timescaledb_internal.compressed_data_recv(internal) SET SCHEMA _
 
 ALTER FUNCTION _timescaledb_internal.rxid_in(cstring) SET SCHEMA _timescaledb_functions;
 ALTER FUNCTION _timescaledb_internal.rxid_out(@extschema@.rxid) SET SCHEMA _timescaledb_functions;
+
+-- drop dependent views
+DROP VIEW IF EXISTS timescaledb_information.job_errors;
+DROP VIEW IF EXISTS timescaledb_information.job_stats;
+DROP VIEW IF EXISTS timescaledb_information.jobs;
+DROP VIEW IF EXISTS timescaledb_experimental.policies;
+
+ALTER TABLE _timescaledb_config.bgw_job
+    ALTER COLUMN owner DROP DEFAULT,
+    ALTER COLUMN owner TYPE regrole USING owner::regrole,
+    ALTER COLUMN owner SET DEFAULT current_role::regrole;
+CREATE TABLE _timescaledb_config.bgw_job_tmp AS SELECT * FROM _timescaledb_config.bgw_job;
+
+ALTER EXTENSION timescaledb DROP TABLE _timescaledb_config.bgw_job;
+ALTER EXTENSION timescaledb DROP SEQUENCE _timescaledb_config.bgw_job_id_seq;
+ALTER TABLE _timescaledb_internal.bgw_job_stat
+      DROP CONSTRAINT IF EXISTS bgw_job_stat_job_id_fkey;
+ALTER TABLE _timescaledb_internal.bgw_policy_chunk_stats
+      DROP CONSTRAINT IF EXISTS bgw_policy_chunk_stats_job_id_fkey;
+CREATE TABLE _timescaledb_internal.tmp_bgw_job_seq_value AS
+       SELECT last_value, is_called FROM _timescaledb_config.bgw_job_id_seq;
+DROP TABLE _timescaledb_config.bgw_job;
+
+CREATE SEQUENCE _timescaledb_config.bgw_job_id_seq MINVALUE 1000;
+SELECT pg_catalog.pg_extension_config_dump('_timescaledb_config.bgw_job_id_seq', '');
+SELECT pg_catalog.setval('_timescaledb_config.bgw_job_id_seq', last_value, is_called)
+  FROM _timescaledb_internal.tmp_bgw_job_seq_value;
+DROP TABLE _timescaledb_internal.tmp_bgw_job_seq_value;
+
+CREATE TABLE _timescaledb_config.bgw_job (
+  id integer NOT NULL DEFAULT nextval('_timescaledb_config.bgw_job_id_seq'),
+  application_name name NOT NULL,
+  schedule_interval interval NOT NULL,
+  max_runtime interval NOT NULL,
+  max_retries integer NOT NULL,
+  retry_period interval NOT NULL,
+  proc_schema name NOT NULL,
+  proc_name name NOT NULL,
+  owner regrole NOT NULL DEFAULT current_role::regrole,
+  scheduled bool NOT NULL DEFAULT TRUE,
+  fixed_schedule bool not null default true,
+  initial_start timestamptz,
+  hypertable_id integer,
+  config jsonb,
+  check_schema name,
+  check_name name,
+  timezone text,
+  CONSTRAINT bgw_job_pkey PRIMARY KEY (id),
+  CONSTRAINT bgw_job_hypertable_id_fkey FOREIGN KEY (hypertable_id) REFERENCES _timescaledb_catalog.hypertable (id) ON DELETE CASCADE
+);
+
+ALTER SEQUENCE _timescaledb_config.bgw_job_id_seq OWNED BY _timescaledb_config.bgw_job.id;
+CREATE INDEX bgw_job_proc_hypertable_id_idx
+       ON _timescaledb_config.bgw_job(proc_schema,proc_name,hypertable_id);
+INSERT INTO _timescaledb_config.bgw_job
+       SELECT * FROM _timescaledb_config.bgw_job_tmp ORDER BY id;
+DROP TABLE _timescaledb_config.bgw_job_tmp;
+ALTER TABLE _timescaledb_internal.bgw_job_stat
+      ADD CONSTRAINT bgw_job_stat_job_id_fkey
+      	  FOREIGN KEY(job_id)
+	  REFERENCES _timescaledb_config.bgw_job(id)
+	  ON DELETE CASCADE;
+ALTER TABLE _timescaledb_internal.bgw_policy_chunk_stats
+      ADD CONSTRAINT bgw_policy_chunk_stats_job_id_fkey
+      	  FOREIGN KEY(job_id)
+	  REFERENCES _timescaledb_config.bgw_job(id)
+	  ON DELETE CASCADE;
+
+SELECT pg_catalog.pg_extension_config_dump('_timescaledb_config.bgw_job', 'WHERE id >= 1000');
+GRANT SELECT ON _timescaledb_config.bgw_job TO PUBLIC;
+GRANT SELECT ON _timescaledb_config.bgw_job_id_seq TO PUBLIC;
diff --git a/sql/updates/reverse-dev.sql b/sql/updates/reverse-dev.sql
@@ -202,3 +202,73 @@ BEGIN
 END
 $BODY$ SET search_path TO pg_catalog, pg_temp;
 
+-- drop dependent views
+DROP VIEW IF EXISTS timescaledb_information.job_errors;
+DROP VIEW IF EXISTS timescaledb_information.job_stats;
+DROP VIEW IF EXISTS timescaledb_information.jobs;
+DROP VIEW IF EXISTS timescaledb_experimental.policies;
+
+ALTER TABLE _timescaledb_config.bgw_job
+    ALTER COLUMN owner DROP DEFAULT,
+    ALTER COLUMN owner TYPE name USING owner::name,
+    ALTER COLUMN owner SET DEFAULT current_role;
+CREATE TABLE _timescaledb_config.bgw_job_tmp AS SELECT * FROM _timescaledb_config.bgw_job;
+
+ALTER EXTENSION timescaledb DROP TABLE _timescaledb_config.bgw_job;
+ALTER EXTENSION timescaledb DROP SEQUENCE _timescaledb_config.bgw_job_id_seq;
+ALTER TABLE _timescaledb_internal.bgw_job_stat
+      DROP CONSTRAINT IF EXISTS bgw_job_stat_job_id_fkey;
+ALTER TABLE _timescaledb_internal.bgw_policy_chunk_stats
+      DROP CONSTRAINT IF EXISTS bgw_policy_chunk_stats_job_id_fkey;
+CREATE TABLE _timescaledb_internal.tmp_bgw_job_seq_value AS
+       SELECT last_value, is_called FROM _timescaledb_config.bgw_job_id_seq;
+DROP TABLE _timescaledb_config.bgw_job;
+
+CREATE SEQUENCE _timescaledb_config.bgw_job_id_seq MINVALUE 1000;
+SELECT pg_catalog.pg_extension_config_dump('_timescaledb_config.bgw_job_id_seq', '');
+SELECT pg_catalog.setval('_timescaledb_config.bgw_job_id_seq', last_value, is_called)
+  FROM _timescaledb_internal.tmp_bgw_job_seq_value;
+DROP TABLE _timescaledb_internal.tmp_bgw_job_seq_value;
+
+CREATE TABLE _timescaledb_config.bgw_job (
+  id integer NOT NULL DEFAULT nextval('_timescaledb_config.bgw_job_id_seq'),
+  application_name name NOT NULL,
+  schedule_interval interval NOT NULL,
+  max_runtime interval NOT NULL,
+  max_retries integer NOT NULL,
+  retry_period interval NOT NULL,
+  proc_schema name NOT NULL,
+  proc_name name NOT NULL,
+  owner name NOT NULL DEFAULT current_role,
+  scheduled bool NOT NULL DEFAULT TRUE,
+  fixed_schedule bool not null default true,
+  initial_start timestamptz,
+  hypertable_id integer,
+  config jsonb,
+  check_schema name,
+  check_name name,
+  timezone text,
+  CONSTRAINT bgw_job_pkey PRIMARY KEY (id),
+  CONSTRAINT bgw_job_hypertable_id_fkey FOREIGN KEY (hypertable_id) REFERENCES _timescaledb_catalog.hypertable (id) ON DELETE CASCADE
+);
+
+ALTER SEQUENCE _timescaledb_config.bgw_job_id_seq OWNED BY _timescaledb_config.bgw_job.id;
+CREATE INDEX bgw_job_proc_hypertable_id_idx
+       ON _timescaledb_config.bgw_job(proc_schema,proc_name,hypertable_id);
+INSERT INTO _timescaledb_config.bgw_job
+       SELECT * FROM _timescaledb_config.bgw_job_tmp ORDER BY id;
+DROP TABLE _timescaledb_config.bgw_job_tmp;
+ALTER TABLE _timescaledb_internal.bgw_job_stat
+      ADD CONSTRAINT bgw_job_stat_job_id_fkey
+      	  FOREIGN KEY(job_id)
+	  REFERENCES _timescaledb_config.bgw_job(id)
+	  ON DELETE CASCADE;
+ALTER TABLE _timescaledb_internal.bgw_policy_chunk_stats
+      ADD CONSTRAINT bgw_policy_chunk_stats_job_id_fkey
+      	  FOREIGN KEY(job_id)
+	  REFERENCES _timescaledb_config.bgw_job(id)
+	  ON DELETE CASCADE;
+
+SELECT pg_catalog.pg_extension_config_dump('_timescaledb_config.bgw_job', 'WHERE id >= 1000');
+GRANT SELECT ON _timescaledb_config.bgw_job TO PUBLIC;
+GRANT SELECT ON _timescaledb_config.bgw_job_id_seq TO PUBLIC;
diff --git a/sql/with_telemetry.sql b/sql/with_telemetry.sql
@@ -7,5 +7,5 @@ CREATE OR REPLACE FUNCTION @extschema@.get_telemetry_report() RETURNS jsonb
 	LANGUAGE C STABLE PARALLEL SAFE;
 
 INSERT INTO _timescaledb_config.bgw_job (id, application_name, schedule_interval, max_runtime, max_retries, retry_period, proc_schema, proc_name, owner, scheduled, fixed_schedule) VALUES
-(1, 'Telemetry Reporter [1]', INTERVAL '24h', INTERVAL '100s', -1, INTERVAL '1h', '_timescaledb_internal', 'policy_telemetry', CURRENT_ROLE, true, false)
+(1, 'Telemetry Reporter [1]', INTERVAL '24h', INTERVAL '100s', -1, INTERVAL '1h', '_timescaledb_internal', 'policy_telemetry', current_role::regrole, true, false)
 ON CONFLICT (id) DO NOTHING;
diff --git a/src/bgw/job.c b/src/bgw/job.c
@@ -291,8 +291,7 @@ bgw_job_from_tupleinfo(TupleInfo *ti, size_t alloc_size)
 				   NameStr(
 					   *DatumGetName(values[AttrNumberGetAttrOffset(Anum_bgw_job_check_name)])));
 	if (!nulls[AttrNumberGetAttrOffset(Anum_bgw_job_owner)])
-		namestrcpy(&job->fd.owner,
-				   NameStr(*DatumGetName(values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)])));
+		job->fd.owner = DatumGetObjectId(values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)]);
 
 	if (!nulls[AttrNumberGetAttrOffset(Anum_bgw_job_scheduled)])
 		job->fd.scheduled = DatumGetBool(values[AttrNumberGetAttrOffset(Anum_bgw_job_scheduled)]);
@@ -977,9 +976,7 @@ ts_bgw_job_check_max_retries(BgwJob *job)
 void
 ts_bgw_job_permission_check(BgwJob *job)
 {
-	Oid owner_oid = get_role_oid(NameStr(job->fd.owner), false);
-
-	if (!has_privs_of_role(GetUserId(), owner_oid))
+	if (!has_privs_of_role(GetUserId(), job->fd.owner))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("insufficient permissions to alter job %d", job->fd.id)));
@@ -1329,7 +1326,7 @@ int
 ts_bgw_job_insert_relation(Name application_name, Interval *schedule_interval,
 						   Interval *max_runtime, int32 max_retries, Interval *retry_period,
 						   Name proc_schema, Name proc_name, Name check_schema, Name check_name,
-						   Name owner, bool scheduled, bool fixed_schedule, int32 hypertable_id,
+						   Oid owner, bool scheduled, bool fixed_schedule, int32 hypertable_id,
 						   Jsonb *config, TimestampTz initial_start, const char *timezone)
 {
 	Catalog *catalog = ts_catalog_get();
@@ -1363,7 +1360,7 @@ ts_bgw_job_insert_relation(Name application_name, Interval *schedule_interval,
 	else
 		nulls[AttrNumberGetAttrOffset(Anum_bgw_job_check_name)] = true;
 
-	values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)] = NameGetDatum(owner);
+	values[AttrNumberGetAttrOffset(Anum_bgw_job_owner)] = ObjectIdGetDatum(owner);
 	values[AttrNumberGetAttrOffset(Anum_bgw_job_scheduled)] = BoolGetDatum(scheduled);
 	values[AttrNumberGetAttrOffset(Anum_bgw_job_fixed_schedule)] = BoolGetDatum(fixed_schedule);
 	/* initial_start must have a value if the schedule is fixed */
@@ -1407,6 +1404,13 @@ ts_bgw_job_insert_relation(Name application_name, Interval *schedule_interval,
 	ts_catalog_insert_values(rel, desc, values, nulls);
 	ts_catalog_restore_user(&sec_ctx);
 
+	/* This is where we would add a call to recordDependencyOnOwner, but it
+	 * cannot support dependencies on anything but built-in classes since
+	 * getObjectClass() have a lot of hard-coded checks in place.
+	 *
+	 * Instead we have a check in process_utility.c that prevents dropping the
+	 * user if there is a dependent job. */
+
 	table_close(rel, NoLock);
 	return values[AttrNumberGetAttrOffset(Anum_bgw_job_id)];
 }

diff --git a/src/bgw/job.h b/src/bgw/job.h
@@ -45,7 +45,7 @@ extern TSDLLEXPORT bool ts_bgw_job_update_by_id(int32 job_id, BgwJob *job);
 extern TSDLLEXPORT int32 ts_bgw_job_insert_relation(
 	Name application_name, Interval *schedule_interval, Interval *max_runtime, int32 max_retries,
 	Interval *retry_period, Name proc_schema, Name proc_name, Name check_schema, Name check_name,
-	Name owner, bool scheduled, bool fixed_schedule, int32 hypertable_id, Jsonb *config,
+	Oid owner, bool scheduled, bool fixed_schedule, int32 hypertable_id, Jsonb *config,
 	TimestampTz initial_start, const char *timezone);
 extern TSDLLEXPORT void ts_bgw_job_permission_check(BgwJob *job);
 

diff --git a/src/bgw/scheduler.c b/src/bgw/scheduler.c
@@ -251,7 +251,6 @@ scheduled_bgw_job_transition_state_to(ScheduledBgwJob *sjob, JobState new_state)
 #endif
 
 	BgwJobStat *job_stat;
-	Oid owner_uid;
 
 	switch (new_state)
 	{
@@ -314,7 +313,6 @@ scheduled_bgw_job_transition_state_to(ScheduledBgwJob *sjob, JobState new_state)
 			else
 				sjob->timeout_at = DT_NOEND;
 
-			owner_uid = get_role_oid(NameStr(sjob->job.fd.owner), false);
 			CommitTransactionCommand();
 			MemoryContextSwitchTo(scratch_mctx);
 
@@ -323,7 +321,7 @@ scheduled_bgw_job_transition_state_to(ScheduledBgwJob *sjob, JobState new_state)
 				 sjob->job.fd.id,
 				 NameStr(sjob->job.fd.application_name));
 
-			sjob->handle = ts_bgw_job_start(&sjob->job, owner_uid);
+			sjob->handle = ts_bgw_job_start(&sjob->job, sjob->job.fd.owner);
 			if (sjob->handle == NULL)
 			{
 				elog(WARNING,

diff --git a/src/process_utility.c b/src/process_utility.c
@@ -13,6 +13,7 @@
 #include <catalog/index.h>
 #include <catalog/objectaddress.h>
 #include <catalog/pg_trigger.h>
+#include <catalog/pg_authid.h>
 #include <commands/copy.h>
 #include <commands/vacuum.h>
 #include <commands/defrem.h>
@@ -71,6 +72,7 @@
 #include "compression_with_clause.h"
 #include "partitioning.h"
 #include "debug_point.h"
+#include "debug_assert.h"
 
 #ifdef USE_TELEMETRY
 #include "telemetry/functions.h"
@@ -1698,6 +1700,62 @@ process_drop_continuous_aggregates(ProcessUtilityArgs *args, DropStmt *stmt)
 				 errhint("Drop continuous aggregates and other objects in separate statements.")));
 }
 
+static bool
+fetch_role_info(RoleSpec *rolespec, Oid *roleid)
+{
+	/* Special role specifiers should not be present when dropping a role,
+	 * but if they are, we just ignore them */
+	if (rolespec->roletype != ROLESPEC_CSTRING)
+		return false;
+
+	/* Fetch the heap tuple from system table. If heaptuple is not valid it
+	 * means we did not find a role. We ignore it since the real execution
+	 * will handle this. */
+	HeapTuple tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(rolespec->rolename));
+	if (!HeapTupleIsValid(tuple))
+		return false;
+
+	Form_pg_authid roleform = (Form_pg_authid) GETSTRUCT(tuple);
+	*roleid = roleform->oid;
+	ReleaseSysCache(tuple);
+	return true;
+}
+
+static DDLResult
+process_drop_role(ProcessUtilityArgs *args)
+{
+	DropRoleStmt *stmt = (DropRoleStmt *) args->parsetree;
+	ListCell *cell;
+	foreach (cell, stmt->roles)
+	{
+		RoleSpec *rolespec = lfirst(cell);
+		Oid roleid;
+
+		if (!fetch_role_info(rolespec, &roleid))
+			continue;
+
+		ScanIterator iterator =
+			ts_scan_iterator_create(BGW_JOB, AccessShareLock, CurrentMemoryContext);
+		ts_scanner_foreach(&iterator)
+		{
+			bool isnull;
+			TupleInfo *ti = ts_scan_iterator_tuple_info(&iterator);
+			Datum value = slot_getattr(ti->slot, Anum_bgw_job_owner, &isnull);
+			if (!isnull && DatumGetObjectId(value) == roleid)
+			{
+				Datum value = slot_getattr(ti->slot, Anum_bgw_job_id, &isnull);
+				Ensure(!isnull, "job id was null");
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+						 errmsg("role \"%s\" cannot be dropped because some objects depend on it",
+								rolespec->rolename),
+						 errdetail("owner of job %d", DatumGetInt32(value))));
+			}
+		}
+	}
+	return DDL_CONTINUE;
+}
+
 static DDLResult
 process_drop_start(ProcessUtilityArgs *args)
 {
@@ -4098,6 +4156,9 @@ process_ddl_command_start(ProcessUtilityArgs *args)
 			 */
 			handler = process_drop_start;
 			break;
+		case T_DropRoleStmt:
+			handler = process_drop_role;
+			break;
 		case T_DropTableSpaceStmt:
 			handler = process_drop_tablespace;
 			break;

diff --git a/src/ts_catalog/catalog.h b/src/ts_catalog/catalog.h
@@ -742,7 +742,7 @@ typedef struct FormData_bgw_job
 	Interval retry_period;
 	NameData proc_schema;
 	NameData proc_name;
-	NameData owner;
+	Oid owner;
 	bool scheduled;
 	bool fixed_schedule;
 	TimestampTz initial_start;