On September 30, 2015 at 12:54:44 AM, Michael Paquier (michael.paqu...@gmail.com) wrote:
>> +extern bool extractExtensionList(char *extensionString, >> + List **extensionOids); >> What's the point of the boolean status in this new routine? The return >> value of extractExtensionList is never checked, and it would be more >> simple to just return the parsed list as return value, no? > > I started changing it, then found out why it is the way it is. During > the options parsing, the list of current extensionOids is passed in, > so that extra ones can be added, since both the wrapper and the server > can be declared with extensionOids. It's also doubling as a flag on > whether the function should bother to try and populate the list, or > just do a sanity check on the options string. I can change the > signature to > > extern List* extractExtensionList(char *extensionString, List > *currentExtensionOids, bool populateList); > to be more explicit if necessary. Hm. Wouldn't it be just fine if only the server is able to define a list of extensions then? It seems to me that all the use-cases of this feature require to have a list of extensions defined per server, and not per fdw type. This would remove a level of complexity in your patch without impacting the feature usability as well. I would personally go without it but I am fine to let a committer (Tom?) put a final judgement stamp on this matter. Thoughts? Simon wanted to be able to define extensions at the wrapper level as well as the server level. It’s a nice enough little feature to warrant a small amount of added complexity, IMO. I’m going to change the signature, since verbose clarity > terse obscurity for my wee brain (only took me a couple weeks to forget why that signature was what it wa s). >> +-- =================================================================== >> +-- clean up >> +-- =================================================================== >> Perhaps here you meant dropping the schema and the foreign tables >> created previously? > > I did, but since postgres_fdw doesn't clean up after itself, perhaps > just leaving the room messy is the appropriate behaviour? That's appropriate when keeping around objects that are aimed to be tested with for example pg_upgrade, some regression tests of src/test/regress do that on purpose for example. Now it is true that an extension regression database will be normally reused by another extension just after, and that there is no general rule on this I’ll clean up then, I was just being lazy. P. -- http://postgis.net http://cleverelephant.ca
