Re: [PATCH] postgres_fdw extension support

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.

> + * shippable.c
> + * Non-built-in objects cache management and utilities.
> + *
> + * Is a non-built-in shippable to the remote server? Only if
> + * the object is in an extension declared by the user in the
> + * OPTIONS of the wrapper or the server.
> This is rather unclear. It would be more simple to describe that as
> "Facility to track database objects shippable to a foreign server".

Done

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

> -REGRESS = postgres_fdw
> +REGRESS = postgres_fdw shippable
> +EXTRA_INSTALL = contrib/seg
> The order of the tests is important and should be mentioned,
> shippable.sql using the loopback server created by postgres_fdw.

Done

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

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

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

P.

> --
> Michael