I haven't had the time to finish all the issues with this, but I want to keep the discussion going in the meantime and provide an updated patch.
On mån, 2012-06-18 at 17:33 +0200, Andres Freund wrote: > Cursory code review: > * pstrndup already exists as pnstrdup (hstore_plperl.c) Fixed. > * PyString_FromStringAndSize return value not decreffed? PyDict_SetItem > doesn't steal references Fixed. > * In plpython_to_hstore I would move the 'pairs' and some related variables > in > the PG_TRY block, so the reader doesn't need to check whether it should be > volatile > * In the same function items needs to be volatile to fit into longjmp > semantics I'll recheck that later. > * I don't think recording dependencies on transforms used when creating > functions is a good idea as the transform might get created after the > functions already exists. That seems to be a pretty confusing behaviour. We need the dependencies, because otherwise dropping a transform would break or silently alter the behavior of functions that depend on it. That sounds like my worst nightmare, thinking of some applications that would be affected by that. But your point is a good one. I think this could be addressed by prohibiting the creation of a transform that affects functions that already exist. Because the legacy behavior of PL implementations of defaulting to a string representation conversion, we would technically need a dependency on the absence of a transform object to make this airtight. In the far future, I could imagine removing this default behavior, meaning you couldn't create the function if no suitable transforms exist for all argument and return types. > * I forsee the need for multiple transforms for the same type/language pair > to > coexist. The user would need to manually "choose"/"call" the transform in > that > case. This currently isn't easily possible... I thought about this briefly at the beginning, but see under "worst nightmare" above. Also, having a configuration setting for this or something would prevent any PL functions from being immutable. We don't allow multiple casts or multiple in/out functions either, which are related concepts. If you want different behavior, you should define a different type or different language. > > *) No psql backslash commands yet. Could be added. > Doesn't really seem necessary to me. Not many people will need to look at > this > and the list of commands already is rather long. I'm going to leave this out for now. > > *) Permissions: Transforms don't have owners, a bit like casts. > > Currently, you are allowed to drop a transform if you own both the type > > and the language. That might be too strict, maybe own the type and have > > privileges on the language would be enough. > Seems sensible enough to me. I have made this change. > > *) There is currently some syntax schizophrenia. The grammar accepts > > > > CREATE TRANSFORM FOR hstore LANGUAGE plperl ( > > FROM SQL WITH hstore_to_plperl, > > TO SQL WITH plperl_to_hstore > > ); > > > > but pg_dump produces > > > > CREATE TRANSFORM FOR hstore LANGUAGE plperl ( > > FROM SQL WITH hstore_to_plperl(hstore), > > TO SQL WITH plperl_to_hstore(internal) > > ); > > > > The SQL standard allows both. (In the same way that it allows 'DROP > > FUNCTION foo' without arguments, if it is not ambigious.) Precedent is > > that CREATE CAST requires arguments, but CREATE LANGUAGE does not. > I don't find that problematic personally. I have fixed the syntax to include argument types, so the dump output and the input grammar is consistent. Other changes: - Fixed ecpg grammar to work again with this. - Changed extension naming to be more consistent. - Build additional contrib modules conditionally depending on whether --with-perl or --with-python were configured. (complaint from Jeff Janes) - Fixed Python 3. Things I still want to do: - Refactor the regression test framework for Python 3 so that contrib modules or external extensions don't have to repeat the magic in src/pl/plpython/Makefile. (Python 3 with hstore_plpython and ltree_plpython works, but the tests don't run.) - Refactor pyobject_to_string(), which is currently kind of copied and pasted from plpython, but should instead be exported by plpython in some suitable way. - Refactor shared library building so that I can have, say, hstore, hstore_plperl, and hstore_plpython in one directory, rather than in three. The reason being, if someone has a new type in a repository on github or something, I don't want them to have to make three separate projects or some crazy subdirectory structure in order to add some PL support for their type. This will require some deep Makefile.shlib hacking, but I think it's worth trying to make this simple(r). So, it's quite likely that this patch won't get finished in this commit fest.
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers