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