Re: [PATCH] Add transforms feature - Mailing list pgsql-hackers

From Peter Eisentraut
Subject Re: [PATCH] Add transforms feature
Date
Msg-id 548E7B5C.8010000@gmx.net
Whole thread Raw
In response to Re: [PATCH] Add transforms feature  (Andres Freund <andres@2ndquadrant.com>)
Responses Re: [PATCH] Add transforms feature  (Tom Lane <tgl@sss.pgh.pa.us>)
Re: [PATCH] Add transforms feature  (Michael Paquier <michael.paquier@gmail.com>)
List pgsql-hackers
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


Attachment

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Commit fest 2014-12, let's begin!
Next
From: Tom Lane
Date:
Subject: Re: [PATCH] Add transforms feature