Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@konskov
konskov committed Aug 24, 2022
1 parent dac0de8 commit d815056
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 44 deletions.
9 changes: 8 additions & 1 deletion src/bgw/job.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,15 @@ ts_bgw_job_run_config_check(Oid check, int32 job_id, Jsonb *config)
if (!OidIsValid(check))
return;

/* NULL config may be valid */
Const *arg;
if (config == NULL)
arg = makeNullConst(JSONBOID, -1, InvalidOid);
else
arg = makeConst(JSONBOID, -1, InvalidOid, -1, JsonbPGetDatum(config), false, false);

List *args =
list_make1(makeConst(JSONBOID, -1, InvalidOid, -1, JsonbPGetDatum(config), false, false));
list_make1(arg);
FuncExpr *funcexpr =
makeFuncExpr(check, VOIDOID, args, InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL);

Expand Down
36 changes: 15 additions & 21 deletions tsl/src/bgw_policy/job_api.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
* This function ensures that the check function has the required signature
* @param check A valid Oid
*/
static inline bool
check_signature_is_valid(Oid check)
static inline void
validate_check_signature(Oid check)
{
Oid proc = InvalidOid;
ObjectWithArgs *object;
Expand All @@ -51,7 +51,13 @@ check_signature_is_valid(Oid check)
object->objargs = list_make1(SystemTypeName("jsonb"));
proc = LookupFuncWithArgs(OBJECT_ROUTINE, object, true);

return OidIsValid(proc);
if (!OidIsValid(proc))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("function or procedure %s.%s(config jsonb) not found",
NameStr(check_schema),
NameStr(check_name)),
errhint("The check function's signature must be (config jsonb).")));
}

/*
Expand Down Expand Up @@ -137,17 +143,11 @@ job_add(PG_FUNCTION_ARGS)
namestrcpy(&proc_name, func_name);
namestrcpy(&owner_name, GetUserNameFromId(owner, false));

/* The check exists but does not have the expected signature: (config jsonb) */
if (OidIsValid(check) && !check_signature_is_valid(check))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("function or procedure %s.%s(config jsonb) not found",
NameStr(check_schema),
NameStr(check_name)),
errhint("The check function's signature must be (config jsonb).")));
/* The check exists but may not have the expected signature: (config jsonb) */
if (OidIsValid(check))
validate_check_signature(check);

if (config)
ts_bgw_job_run_config_check(check, 0, config);
ts_bgw_job_run_config_check(check, 0, config);

job_id = ts_bgw_job_insert_relation(&application_name,
schedule_interval,
Expand Down Expand Up @@ -323,14 +323,8 @@ job_alter(PG_FUNCTION_ARGS)
namestrcpy(&check_schema, get_namespace_name(get_func_namespace(check)));
namestrcpy(&check_name, check_name_str);

/* The check exists but does not have the expected signature: (config jsonb) */
if (OidIsValid(check) && !check_signature_is_valid(check))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("function or procedure %s.%s(config jsonb) not found",
NameStr(check_schema),
NameStr(check_name)),
errhint("The check function's signature must be (config jsonb).")));
/* The check exists but may not have the expected signature: (config jsonb) */
validate_check_signature(check);

namestrcpy(&job->fd.check_schema, NameStr(check_schema));
namestrcpy(&job->fd.check_name, NameStr(check_name));
Expand Down
52 changes: 36 additions & 16 deletions tsl/test/expected/bgw_custom.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ DECLARE
BEGIN
SELECT jsonb_object_field_text (config, 'drop_after')::interval INTO STRICT drop_after;
IF drop_after IS NULL THEN
RAISE EXCEPTION 'Config must have drop_after';
RAISE EXCEPTION 'Config must be not NULL and have drop_after';
END IF ;
END
$$;
Expand All @@ -600,9 +600,12 @@ AS $$
DECLARE
drop_after interval;
BEGIN
IF config IS NULL THEN
RETURN;
END IF;
SELECT jsonb_object_field_text (config, 'drop_after')::interval INTO STRICT drop_after;
IF drop_after IS NULL THEN
RAISE EXCEPTION 'Config must have drop_after';
RAISE EXCEPTION 'Config can be NULL but must have drop_after if not';
END IF ;
END
$$ LANGUAGE PLPGSQL;
Expand All @@ -617,24 +620,22 @@ $$;
-- step 3, add the job with the config check function passed as argument
-- test procedures
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_proc'::regproc);
ERROR: Config must have drop_after
select add_job('test_proc_with_check', '5 secs', config => '{"one": "two"}', check_config => 'test_config_check_proc'::regproc);
ERROR: Config must have drop_after
ERROR: Config must be not NULL and have drop_after
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_proc'::regproc);
ERROR: Config must be not NULL and have drop_after
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "chicken"}', check_config => 'test_config_check_proc'::regproc);
ERROR: invalid input syntax for type interval: "chicken"
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "2 weeks"}', check_config => 'test_config_check_proc'::regproc)
as job_with_proc_check_id \gset
-- test functions
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_func'::regproc);
ERROR: Config must have drop_after
ERROR: Config can be NULL but must have drop_after if not
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_func'::regproc);
add_job
---------
1006
(1 row)

select add_job('test_proc_with_check', '5 secs', config => '{"one": "two"}', check_config => 'test_config_check_func'::regproc);
ERROR: Config must have drop_after
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "chicken"}', check_config => 'test_config_check_func'::regproc);
ERROR: invalid input syntax for type interval: "chicken"
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "2 weeks"}', check_config => 'test_config_check_func'::regproc)
Expand Down Expand Up @@ -690,20 +691,21 @@ ERROR: function "test_nonexistent_check_func" does not exist at character 82
CREATE OR REPLACE FUNCTION test_config_check_func(config jsonb) RETURNS VOID
AS $$
BEGIN
RAISE NOTICE 'This message will only get printed if config is not NULL';
RAISE NOTICE 'This message will get printed for both NULL and not NULL config';
END
$$ LANGUAGE PLPGSQL;
SET client_min_messages = NOTICE;
-- no check done
-- check done for both NULL and non-NULL config
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_func'::regproc);
NOTICE: This message will get printed for both NULL and not NULL config
add_job
---------
1008
(1 row)

-- check done
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_func'::regproc) as job_id \gset
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
-- check function not returning void
CREATE OR REPLACE FUNCTION test_config_check_func_returns_int(config jsonb) RETURNS INT
AS $$
Expand Down Expand Up @@ -742,7 +744,7 @@ WARNING: function or procedure public.test_config_check_func(config jsonb) not

-- run alter again, should get a config check
select alter_job(:job_id, config => '{}');
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
alter_job
--------------------------------------------------------------------------
(1009,"@ 1 hour","@ 0",-1,"@ 5 mins",t,{},-infinity,public.renamed_func)
Expand All @@ -757,7 +759,7 @@ END
$$ LANGUAGE PLPGSQL;
-- register the new check
select alter_job(:job_id, check_config => 'substitute_check_func');
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
alter_job
-----------------------------------------------------------------------------------
(1009,"@ 1 hour","@ 0",-1,"@ 5 mins",t,{},-infinity,public.substitute_check_func)
Expand Down Expand Up @@ -800,19 +802,19 @@ ERROR: permission denied for function "test_config_check_func_privileges"
\c :TEST_DBNAME :ROLE_SUPERUSER
-- check that alter_job rejects a check function with invalid signature
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'renamed_func') as job_id_alter \gset
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
select alter_job(:job_id_alter, check_config => 'test_config_check_func_0args');
ERROR: function or procedure public.test_config_check_func_0args(config jsonb) not found
select alter_job(:job_id_alter);
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
alter_job
--------------------------------------------------------------------------
(1011,"@ 5 secs","@ 0",-1,"@ 5 mins",t,{},-infinity,public.renamed_func)
(1 row)

-- test that we can unregister the check function
select alter_job(:job_id_alter, check_config => 0);
NOTICE: This message will only get printed if config is not NULL
NOTICE: This message will get printed for both NULL and not NULL config
alter_job
-------------------------------------------------------
(1011,"@ 5 secs","@ 0",-1,"@ 5 mins",t,{},-infinity,)
Expand Down Expand Up @@ -880,3 +882,21 @@ ERROR: permission denied for function "test_config_check_func_privileges"
DROP SCHEMA test_schema CASCADE;
NOTICE: drop cascades to function test_schema.test_config_check_func_privileges(jsonb)
DROP ROLE user_noexec;
-- test with aggregate check proc
create function jsonb_add (j1 jsonb, j2 jsonb) returns jsonb
AS $$
BEGIN
RETURN j1 || j2;
END
$$ LANGUAGE PLPGSQL;
create table jsonb_values (j jsonb, i int);
insert into jsonb_values values ('{"refresh_after":"2 weeks"}', 1), ('{"compress_after":"2 weeks"}', 2), ('{"drop_after":"2 weeks"}', 3);
CREATE AGGREGATE sum_jsb (jsonb)
(
sfunc = jsonb_add,
stype = jsonb,
initcond = '{}'
);
-- for test coverage, check unsupported aggregate type
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'sum_jsb'::regproc);
ERROR: unsupported function type
37 changes: 31 additions & 6 deletions tsl/test/sql/bgw_custom.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ DECLARE
BEGIN
SELECT jsonb_object_field_text (config, 'drop_after')::interval INTO STRICT drop_after;
IF drop_after IS NULL THEN
RAISE EXCEPTION 'Config must have drop_after';
RAISE EXCEPTION 'Config must be not NULL and have drop_after';
END IF ;
END
$$;
Expand All @@ -341,9 +341,12 @@ AS $$
DECLARE
drop_after interval;
BEGIN
IF config IS NULL THEN
RETURN;
END IF;
SELECT jsonb_object_field_text (config, 'drop_after')::interval INTO STRICT drop_after;
IF drop_after IS NULL THEN
RAISE EXCEPTION 'Config must have drop_after';
RAISE EXCEPTION 'Config can be NULL but must have drop_after if not';
END IF ;
END
$$ LANGUAGE PLPGSQL;
Expand All @@ -360,15 +363,14 @@ $$;
-- step 3, add the job with the config check function passed as argument
-- test procedures
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_proc'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"one": "two"}', check_config => 'test_config_check_proc'::regproc);
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_proc'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "chicken"}', check_config => 'test_config_check_proc'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "2 weeks"}', check_config => 'test_config_check_proc'::regproc)
as job_with_proc_check_id \gset

-- test functions
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_func'::regproc);
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_func'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"one": "two"}', check_config => 'test_config_check_func'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "chicken"}', check_config => 'test_config_check_func'::regproc);
select add_job('test_proc_with_check', '5 secs', config => '{"drop_after": "2 weeks"}', check_config => 'test_config_check_func'::regproc)
as job_with_func_check_id \gset
Expand Down Expand Up @@ -418,12 +420,12 @@ select add_job('test_proc_with_check', '5 secs', config => '{}', check_config =>
CREATE OR REPLACE FUNCTION test_config_check_func(config jsonb) RETURNS VOID
AS $$
BEGIN
RAISE NOTICE 'This message will only get printed if config is not NULL';
RAISE NOTICE 'This message will get printed for both NULL and not NULL config';
END
$$ LANGUAGE PLPGSQL;

SET client_min_messages = NOTICE;
-- no check done
-- check done for both NULL and non-NULL config
select add_job('test_proc_with_check', '5 secs', config => NULL, check_config => 'test_config_check_func'::regproc);
-- check done
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'test_config_check_func'::regproc) as job_id \gset
Expand Down Expand Up @@ -537,3 +539,26 @@ select * from alter_job(:job_id_owner, check_config => 'test_schema.test_config_
\c :TEST_DBNAME :ROLE_SUPERUSER
DROP SCHEMA test_schema CASCADE;
DROP ROLE user_noexec;

-- test with aggregate check proc
create function jsonb_add (j1 jsonb, j2 jsonb) returns jsonb
AS $$
BEGIN
RETURN j1 || j2;
END
$$ LANGUAGE PLPGSQL;

create table jsonb_values (j jsonb, i int);
insert into jsonb_values values ('{"refresh_after":"2 weeks"}', 1), ('{"compress_after":"2 weeks"}', 2), ('{"drop_after":"2 weeks"}', 3);

CREATE AGGREGATE sum_jsb (jsonb)
(
sfunc = jsonb_add,
stype = jsonb,
initcond = '{}'
);

-- for test coverage, check unsupported aggregate type
select add_job('test_proc_with_check', '5 secs', config => '{}', check_config => 'sum_jsb'::regproc);


0 comments on commit d815056

Please sign in to comment.