Re: [PATCH] Add transforms feature

I have an updated patch for this. At the end of the 2014-01 commit
fest, it seems that the design was generally considered OK.

This patch is rebased, has some updates and some bug fixes.

Responses to the last review below.

On 4/4/14 6:21 PM, Andres Freund wrote:
>> index 4e476c3..5ac9f05 100644
>> --- a/src/Makefile.shlib
>> +++ b/src/Makefile.shlib
>> @@ -133,7 +133,7 @@ ifeq ($(PORTNAME), darwin)
>> else
>> # loadable module
>> DLSUFFIX = .so
>> - LINK.shared = $(COMPILER) -bundle -multiply_defined suppress
>> + LINK.shared = $(COMPILER) -bundle -multiply_defined suppress -Wl,-undefined,dynamic_lookup
>> endif
>> BUILD.exports = $(AWK) '/^[^\#]/ {printf "_%s\n",$$1}' $< >$@
>> exports_file = $(SHLIB_EXPORTS:%.txt=%.list)
>
> Hm. Do the linkers on other platforms support this behaviour? Linux
> does, by default, but I have zap clue about the rest.

I think all other platforms do this by default, or can be made to do so.

> Why do we need this? I guess because a transform from e.g. hstore to $pl
> needs symbols out of hstore.so and the $pl?
>
> I wonder if it woudln't be better to rely on explicit symbol lookups for
> this. That'd avoid the need for the order dependency and linker changes
> like this.

That seems quite difficult. For example, hstore has things like
HS_COUNT() and HS_VAL(), which aren't even symbols.

>> + case OBJECT_TRANSFORM:
>> + {
>> + TypeName *typename = (TypeName *) linitial(objname);
>> + char *langname = (char *) linitial(objargs);
>> + Oid type_id = typenameTypeId(NULL, typename);
>> + Oid lang_id = get_language_oid(langname, false);
>> +
>> + address.classId = TransformRelationId;
>> + address.objectId =
>> + get_transform_oid(type_id, lang_id, missing_ok);
>> + address.objectSubId = 0;
>> + }
>> + break;
>
> Hm. I wonder if missing_ok should be forwarded to get_language_oid() and
> (by changing the way things are done atm) to typenameTypeId?

done

>> + case OCLASS_TRANSFORM:
>> + {
>> + HeapTuple trfTup;
>> + Form_pg_transform trfForm;
>> +
>> + trfTup = SearchSysCache1(TRFOID,
>> + ObjectIdGetDatum(object->objectId));
>> + if (!HeapTupleIsValid(trfTup))
>> + elog(ERROR, "could not find tuple for transform %u",
>> + object->objectId);
>> +
>> + trfForm = (Form_pg_transform) GETSTRUCT(trfTup);
>> +
>> + appendStringInfo(&buffer, _("transform for %s language %s"),
>> + format_type_be(trfForm->trftype),
>> + get_language_name(trfForm->trflang, false));
>> +
>> + ReleaseSysCache(trfTup);
>> + break;
>> + }
>> +
>
> Why deviate from the usual 'cache lookup failed for ..'? elog doesn't
> translate so it's not particular relevant, but ...

That's how the surrounding code does it.

>> referenced.objectSubId = 0;
>> recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>>
>> + /* dependency on transform used by return type, if any */
>> + if ((trfid = get_transform_oid(returnType, languageObjectId, true)))
>> + {
>> + referenced.classId = TransformRelationId;
>> + referenced.objectId = trfid;
>> + referenced.objectSubId = 0;
>> + recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>> + }
>> +
>
> Should be compared to InvalidOid imo, rather than implicitly assuming
> that InvalidOid evaluates to false.

I think it's widely assumed that InvalidOid is false.

>> +/*
>> + * CREATE TRANSFORM
>> + */
>> +Oid
>> +CreateTransform(CreateTransformStmt *stmt)
>> +{
> ...
>> + if (!pg_type_ownercheck(typeid, GetUserId()))
>> + aclcheck_error_type(ACLCHECK_NOT_OWNER, typeid);
>> +
>> + aclresult = pg_type_aclcheck(typeid, GetUserId(), ACL_USAGE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error_type(aclresult, typeid);
>> +
>> + /*
>> + * Get the language
>> + */
>> + langid = get_language_oid(stmt->lang, false);
>> +
>> + aclresult = pg_language_aclcheck(langid, GetUserId(), ACL_USAGE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error(aclresult, ACL_KIND_LANGUAGE, stmt->lang);
>
> Hm. Is USAGE really sufficient here? Should we possibly make it
> dependant on lanpltrusted like CreateFunction() does?

It could be done, but I don't see why it's necessary. If the language
isn't trusted, why grant USAGE on it?

>> + if (stmt->fromsql)
>> + {
>> + fromsqlfuncid = LookupFuncNameTypeNames(stmt->fromsql->funcname, stmt->fromsql->funcargs, false);
>> +
>> + if (!pg_proc_ownercheck(fromsqlfuncid, GetUserId()))
>> + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));
>
> Why isn't EXECUTE sufficient here?

because of the below

>> + aclresult = pg_proc_aclcheck(fromsqlfuncid, GetUserId(), ACL_EXECUTE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error(aclresult, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));
>> +
>> + tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(fromsqlfuncid));
>> + if (!HeapTupleIsValid(tuple))
>> + elog(ERROR, "cache lookup failed for function %u", fromsqlfuncid);
>> + procstruct = (Form_pg_proc) GETSTRUCT(tuple);
>> + if (procstruct->prorettype != INTERNALOID)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("return data type of FROM SQL function must be \"internal\"")));
>> + check_transform_function(procstruct);
>> + ReleaseSysCache(tuple);
>
> So, this can be used to call a function that takes INTERNAL, and returns
> INTERNAL. Isn't that normally reserved for superusers? I think this at
> the very least needs to be an ownercheck on the function?

exactly, see above

>> @@ -66,6 +66,7 @@ CATALOG(pg_proc,1255) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81) BKI_SCHEMA_MACRO
>> text proargnames[1]; /* parameter names (NULL if no names) */
>> pg_node_tree proargdefaults;/* list of expression trees for argument
>> * defaults (NULL if none) */
>> + Oid protrftypes[1] /* types for which to apply transforms */
>> text prosrc; /* procedure source text */
>> text probin; /* secondary procedure info (can be NULL) */
>> text proconfig[1]; /* procedure-local GUC settings */
>
> I wonder if this shouldn't rather be a array that lists the transform to
> be used for every single column akin to proargtypes or such. That's
> going to make life easier for pl developers.

That would allow using different transforms for different arguments of
the same type, which I don't want to allow, and PLs might not be able to
handle.

I understand where you are coming from, but the alternative seems worse.

>> /**********************************************************************
>> @@ -1260,6 +1264,7 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
>> bool *isnull)
>> {
>> FmgrInfo tmp;
>> + Oid funcid;
>>
>> /* we might recurse */
>> check_stack_depth();
>> @@ -1283,6 +1288,8 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
>> /* must call typinput in case it wants to reject NULL */
>> return InputFunctionCall(finfo, NULL, typioparam, typmod);
>> }
>> + else if ((funcid = get_transform_tosql(typid, current_call_data->prodesc->lang_oid)))
>> + return OidFunctionCall1(funcid, PointerGetDatum(sv));
>> else if (SvROK(sv))
>> {
>> /* handle references */
>
> Am I missing something here? You're not looking at the proc's
> transforms, but just lookup the general ones? Same for output and such.
>
> Looks like you forgot to update this.

fixed