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: