Re: [PATCH] Add transforms feature

On 2014-01-15 21:13:18 -0500, Peter Eisentraut wrote:
> The attached patch will probably fail to apply because of the pg_proc
> changes.  So if you want to try it out, look into the header for the Git
> hash it was based off.  I'll produce a properly merged version when this
> approach is validated.  Also, some implementation details could probably
> take some revising after a couple of nights of sleep. ;-)

You've slept since? ;)

So, I am only doign a look through the patch, to see where it has gone
in the past year.

> 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.

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.

> +            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?

> +        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 ...

>      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.

> +/*
> + * 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?

> +    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?

> +        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?

> @@ -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.
>  /**********************************************************************
> @@ -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.


>      for (i = 0; i < desc->nargs; i++)
>      {
>          if (fcinfo->argnull[i])
> @@ -2055,9 +2075,16 @@ static SV  *plperl_call_perl_func(plperl_proc_desc *desc,
>          else
>          {
>              SV           *sv;
> +            Oid            funcid;
>  
>              if (OidIsValid(desc->arg_arraytype[i]))
>                  sv = plperl_ref_from_pg_array(fcinfo->arg[i], desc->arg_arraytype[i]);
> +            else if (list_member_oid(current_call_data->prodesc->trftypes, argtypes[i]))
> +            {
> +                funcid = get_transform_fromsql(argtypes[i], current_call_data->prodesc->lang_oid);
> +                Assert(funcid); // TODO
> +                sv = (SV *) DatumGetPointer(OidFunctionCall1(funcid, fcinfo->arg[i]));
> +            }

This is the behaviour I'd really would like to avoid. Searching an array
for every parameter sucks. I think this really should be a vector with
one element per argument. Yes, that's going to require special handling
of the return type, but whatever.


So, I lost my motiviation here. I like this version *much* more than the
state of a year ago. I think there's a fair amount of work required
here, but it seems to be dilligence that's required, not redesign. But I
still don't think that's doable within the next couple of days?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services