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.



Reply via email to