Re: [PATCH] postgres_fdw extension support

On Mon, Sep 28, 2015 at 10:47 PM, Paul Ramsey <pramsey(at)cleverelephant(dot)ca> wrote:
> On Sat, Sep 26, 2015 at 5:41 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Sat, Sep 26, 2015 at 4:29 AM, Paul Ramsey wrote:
>>> On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier wrote:
>>> src/backend/utils/adt/format_type.c
>>> +/*
>>> + * This version allows a nondefault typemod to be specified and fully
>>> qualified.
>>> + */
>>> +char *
>>> +format_type_with_typemod_qualified(Oid type_oid, int32 typemod)
>>> +{
>>> + return format_type_internal(type_oid, typemod, true, false, true);
>>> +}
>>
>> Patch 0001 in this email has a routine called format_type_detailed
>> that I think is basically what we need for this stuff:
>> http://www.postgresql.org/message-id/CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE=mXvRhEfjJ51aCfwQ@mail.gmail.com
>> Could you look at it?
>
> I'm not sure it helps much. I'd still need a function to turn the
> output into a formatted string, but more importantly the big question
> for me to avoid breaking regression is: if it's built-in, don't schema
> qualitfy it; if it's extended do so. I'm not seeing why ignoring the
> typmod in the case of deparsing extended type constants is going to be
> a problem? All the old behaviour is untouched in the current patch.

Yeah, let's do it so then.

>> +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?

>> +-- ===================================================================
>> +-- 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
matter. Some contrib/ modules do this cleanup, others not. I would
have cleaned up the room if I would have coded this set of tests, but
as you're putting your hands in it I am fine with your decision and
that's really no big deal.

>> +CREATE SCHEMA "SH 1";
>> Is there a reason to use double-quoted relations? There is no real
>> reason to not use them, this is just to point out that this is not a
>> common practice in the other regression tests.
>
> Just following the pattern from postgres_fdw. And since I found things
> to be sensitive to full qualification of function names, etc, it
> seemed like a good idea to try out odd named tables/schemas since the
> pattern was there to follow.

OK. I have no arguments against that.

> I also changed the extension being tested from 'seg' to 'cube', since
> cube had some functions I could test as well as operators.

This change looks fine to me.
--
Michael