Implement create_operator_func API. #210

lyupan · 2023-07-03T17:54:48Z

pgtle_admin can run create_operator_func to create operator funcs for base types created in pg_tle (by create_base_type).

User-defined operator function must accept one or two arguments of BYTEA, we make an C-version operator function that accepts base type as the argument; inside the C-version operator function, we find the user-defined operator function and execute it.

Notes:

create_operator_func is not strictly necessary to define an operator function on the base type; however, it will be required for languages such as plrust, because the custom base type is not available in plrust;
CREATE OPERATOR CLASS requires superuser privileges in pg_tle, but should be already supported in AWS RDS.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pgtle_admin can run create_operator_func to create operator funcs for base types created in pg_tle (by create_base_type). User-defined operator function must accept one or two arguments of BYTEA, we make an C-version operator function that accepts base type as the argument; inside the C-version operator function, we find the user-defined operator function and execute it. Notes: 1. create_operator_func is not strictly necessary to define an operator function on the base type; however, it will be required for languages such as plrust, because the custom base type is not available in plrust; 2. `CREATE OPERATOR CLASS` requires superuser privileges in pg_tle, but should be already supported in AWS RDS. Co-authored-by: Jim Mlodgenski <jimmy76@gmail.com>

jkatz

Just a few comments to get started with. Need to dive deeper into this.

jkatz · 2023-07-03T18:11:14Z

src/datatype.c

+	if (!((proc->pronargs == 1 && proc->proargtypes.values[0] == BYTEAOID) ||
+		  (proc->pronargs == 2 && proc->proargtypes.values[0] == BYTEAOID && proc->proargtypes.values[1] == BYTEAOID)))
+	{
+		ReleaseSysCache(tuple);
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+				 errmsg("type opeartor function must accept one or two arguments of type bytea")));
+	}
+
+	if (proc->pronamespace != expectedNamespace)
+	{
+		ReleaseSysCache(tuple);
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+				 errmsg("type operator functions must exist in the same namespace as the type")));
+	}
+
+	argTypes = (Oid *) palloc(proc->pronargs * sizeof(Oid));
+	argTypes[0] = typeOid;
+	if (proc->pronargs == 2)
+		argTypes[1] = typeOid;


Suggested change

if (!((proc->pronargs == 1 && proc->proargtypes.values[0] == BYTEAOID) ||

(proc->pronargs == 2 && proc->proargtypes.values[0] == BYTEAOID && proc->proargtypes.values[1] == BYTEAOID)))

{

ReleaseSysCache(tuple);

ereport(ERROR,

(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),

errmsg("type opeartor function must accept one or two arguments of type bytea")));

}

if (proc->pronamespace != expectedNamespace)

{

ReleaseSysCache(tuple);

ereport(ERROR,

(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),

errmsg("type operator functions must exist in the same namespace as the type")));

}

argTypes = (Oid *) palloc(proc->pronargs * sizeof(Oid));

argTypes[0] = typeOid;

if (proc->pronargs == 2)

argTypes[1] = typeOid;

if (proc->pronamespace != expectedNamespace)

{

ReleaseSysCache(tuple);

ereport(ERROR,

(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),

errmsg("type operator functions must exist in the same namespace as the type")));

}

if (proc->pronargs > 2)

{

ReleaseSysCache(tuple);

ereport(ERROR,

(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),

errmsg("type opeartor function must accept one or two arguments bytea")));

}

argTypes = (Oid *) palloc(proc->pronargs * sizeof(Oid));

/* FIXME: declare int i at the top of the function */

for (i = 0; i < proc->pronargs; i++)

{

if (proc->proargtypes.values[i] != BYTEAOID)

{

ReleaseSysCache(tuple);

ereport(ERROR,

(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),

errmsg("type operator function must accept arguments of bytea")));

}

argTypes[i] = typeOid;

}

Updated. Thanks for the suggestion!

jkatz · 2023-07-03T18:12:54Z

src/datatype.c

+	if (OidIsValid(LookupFuncName(funcNameList, proc->pronargs, argTypes, true)))
+		ereport(ERROR,
+				(errcode(ERRCODE_DUPLICATE_OBJECT),
+				 errmsg("function \"%s\" already exists", NameListToString(funcNameList))));


This doesn't release the tuple.

Also -- are we able to release the tuple after we copy the data into the structure? I believe we'd have to do a full copy of the tuples.

This doesn't release the tuple

Good catch! Fixed in a new commit.

Also -- are we able to release the tuple after we copy the data into the structure? I believe we'd have to do a full copy of the tuples.

Do you refer to this code makeString(NameStr(proc->proname))? That's probably an issue, I've added a pstrdup to do a string copy.

@lyupan I was referring to seeing if we could make a copy of the entire struct so we could immediately release the tuple, or (as you suggest), copy the values we need and release it. I don't think it's a long running operation, but I'd generally think it'd be better to not have to access the tuple longer than we need to.

…es in postgres 11.

jkatz

I think this is just about ready. Most of my comments were minor.

jkatz · 2023-07-10T20:06:14Z

src/datatype.c

 	inputNames = list_make2(makeString(get_namespace_name(proc->pronamespace)),
-							makeString(NameStr(proc->proname)));
+							makeString(pstrdup(NameStr(proc->proname))));


This is pretty hard to read. Likewise, if we need to make a copy of proc->proname, we'd also have to do that for proc->pronamespace (unless get_namespace_name is doing that).

jkatz · 2023-07-10T20:25:42Z

src/datatype.c

+	argTypes[0] = typeOid;
+	if (nargs == 2)
+		argTypes[1] = typeOid;


Suggested change

argTypes[0] = typeOid;

if (nargs == 2)

argTypes[1] = typeOid;

for (i = 0; i < nargs; i++)

argTypes[i] = typeOid;

Done. Thanks for the suggestion

jkatz · 2023-07-10T20:27:01Z

src/datatype.c

+
+	CHECK_CAN_SET_ROLE(typeOwner, tleadminoid);
+
+	if (!is_pgtle_io_func(inputOid, true) || !is_pgtle_io_func(outputOid, false))


Suggested change

if (!is_pgtle_io_func(inputOid, true) || !is_pgtle_io_func(outputOid, false))

if (!(is_pgtle_io_func(inputOid, true) && is_pgtle_io_func(outputOid, false)))

jkatz · 2023-07-10T20:32:14Z

src/datatype.c

+
+	prosrcstring = TextDatumGetCString(prosrcattr);
+	ReleaseSysCache(tuple);
+	return strcmp(prosrcstring, typeInput ? TLE_BASE_TYPE_IN : TLE_BASE_TYPE_OUT) == 0;


We've been using strncmp

jkatz · 2023-07-10T20:33:13Z

src/datatype.c

+	}
+
+	nargs = proc->pronargs;
+	if (nargs != 1 && nargs != 2)


Suggested change

if (nargs != 1 && nargs != 2)

if (nargs < 1 || nargs > 2)

jkatz · 2023-07-10T20:33:46Z

src/datatype.c

+
+	procname = get_qualified_funcname(fcinfo->flinfo->fn_oid);
+	get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
+	if (nargs != 1 && nargs != 2)


Suggested change

if (nargs != 1 && nargs != 2)

if (nargs < 1 || nargs > 2)

jkatz · 2023-07-10T20:36:04Z

src/datatype.c

+	argtypes[0] = BYTEAOID;
+	if (nargs == 2)
+		argtypes[1] = BYTEAOID;


Suggested change

argtypes[0] = BYTEAOID;

if (nargs == 2)

argtypes[1] = BYTEAOID;

for (i = 0; i < 2; i++)

argtypes[i] = BYTEAOID;

jkatz · 2023-07-10T20:37:18Z

src/datatype.c

+	if (nargs == 1)
+		result = OidFunctionCall1Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0));
+	else
+		result = OidFunctionCall2Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0), PG_GETARG_DATUM(1));
+
+	return result;


Suggested change

if (nargs == 1)

result = OidFunctionCall1Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0));

else

result = OidFunctionCall2Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0), PG_GETARG_DATUM(1));

return result;

if (nargs == 1)

return OidFunctionCall1Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0));

return OidFunctionCall2Coll(userFunc, InvalidOid, PG_GETARG_DATUM(0), PG_GETARG_DATUM(1));

formalLogicGirl · 2023-07-10T21:12:16Z

src/datatype.c

+	 * pgtle_admin can run this function, let's check and make sure there is
+	 * not a way to bypass that
+	 */
+	check_is_pgtle_admin();


Can we add some regression tests for the various checks we exercise in this function? Such as

a non pgtle_admin role attempts to call the function -- fails

role without CREATE permissions in namespace attempts to call the function -- fails

role is not owner of base type -- fails

role is not owner of operator func -- fails

Done. Added a few more regression tests cases.

formalLogicGirl · 2023-07-11T17:24:39Z

src/datatype.c

+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for function %u", funcid);
+	proc = (Form_pg_proc) GETSTRUCT(tuple);
+


Can we move the ReleaseSysCache(tuple) call way up here, and call it once only, after copying all the values we need in the subsequent code? My understanding is that right now, we only have ref copies so we can't release until we're done, but there are a fixed number of values we reference later so we could make copies of all those upfront.

The reason for this suggestion is to make it easier to read and reason about it. The way its structured now, there are many code paths to check to ensure we've released the lock, so it would be a little easier to read and maintain in the future if we change the code later, to have one point of releasing the lock.

It can also reduce the period of time PROCOID is locked for the happy path, although I do recognize that for the error cases instead of failing early like now, we will do more work before release because of all the copying we will never need.

Done. Reduced a few ReleaseSysCache calls by copying the values upfront. Because we want check nargs before allocation a new array (to avoid too many arguments or zero/negative nargs values), still do two ReleaseSysCache calls here.

formalLogicGirl · 2023-07-11T17:27:59Z

src/datatype.c

+	proc = (Form_pg_proc) GETSTRUCT(tuple);
+	if (proc->prolang != ClanguageId)
+	{
+		ReleaseSysCache(tuple);


Similar comment as previous one, about copying the data out, and once only ReleaseSysCache(tuple)

SysCacheGetAttr is somewhat more costly than a simply copy, and there are only two ReleaseSysCache, I guess we can keep it as it is.

formalLogicGirl

Looks good to me!

jkatz reviewed Jul 3, 2023

View reviewed changes

Lyu Pan and others added 3 commits July 3, 2023 18:41

Fix sys cache leak in check_user_operator_func and regress test failr…

70be303

…es in postgres 11.

Refactor check_user_operator_func code for better readability.

e308776

Merge branch 'aws:main' into main

04be35d

jkatz reviewed Jul 10, 2023

View reviewed changes

formalLogicGirl reviewed Jul 10, 2023

View reviewed changes

Minor code refactor in datatype.c and more regression test cases.

c913681

lyupan requested review from jkatz and formalLogicGirl July 10, 2023 22:27

formalLogicGirl reviewed Jul 11, 2023

View reviewed changes

Reduce ReleaseSysCache calls.

d94445d

lyupan requested a review from formalLogicGirl July 11, 2023 18:55

formalLogicGirl approved these changes Jul 11, 2023

View reviewed changes

formalLogicGirl merged commit 101d19b into aws:main Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement create_operator_func API. #210

Implement create_operator_func API. #210

lyupan commented Jul 3, 2023

jkatz left a comment

jkatz Jul 3, 2023

lyupan Jul 3, 2023

jkatz Jul 3, 2023

lyupan Jul 3, 2023

jkatz Jul 10, 2023

jkatz left a comment

jkatz Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

jkatz Jul 10, 2023

lyupan Jul 10, 2023

formalLogicGirl Jul 10, 2023

lyupan Jul 10, 2023

formalLogicGirl Jul 11, 2023

lyupan Jul 11, 2023 •

edited

Loading

formalLogicGirl Jul 11, 2023

lyupan Jul 11, 2023

formalLogicGirl left a comment


		CHECK_CAN_SET_ROLE(typeOwner, tleadminoid);

		if (!is_pgtle_io_func(inputOid, true) \|\| !is_pgtle_io_func(outputOid, false))

	if (!is_pgtle_io_func(inputOid, true) \|\| !is_pgtle_io_func(outputOid, false))
	if (!(is_pgtle_io_func(inputOid, true) && is_pgtle_io_func(outputOid, false)))

Implement create_operator_func API. #210

Implement create_operator_func API. #210

Conversation

lyupan commented Jul 3, 2023

jkatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyupan Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formalLogicGirl left a comment

Choose a reason for hiding this comment

lyupan Jul 11, 2023 •

edited

Loading