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