Re: [PATCH 4/4] Add tests to dblink covering use of COPY TO FUNCTION

On Tue, Dec 29, 2009 at 11:47 PM, Daniel Farina <drfarina(at)acm(dot)org> wrote:
> On Tue, Dec 29, 2009 at 8:11 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> It might be possible to do this without introducing a notion of an
>> explicit object, but there are a couple of problems with that.  First,
>> it requires the user to specify a lot of details on every COPY
>> invocation, which is awkward.  Second, there's a security issue to
>> think about here.  If we were just copying to a UDF that took a string
>> as an argument, that would be fine, but it's not safe to let
>> unprivileged users to directly invoke functions that take a
>> type-internal argument.  Introducing an explicit object type would
>> allow the creation of copy targets to be restricted to super-users but
>> then granted out to whom the super-user chooses.
>>
>> Thoughts?
>
> Agree on the type internal and superuser access -- indeed, if one were
> to refactor the two existing COPY modes into external functions, the
> stdout behavior would be marked with SECURITY DEFINER and the to-file
> functions would only be superuser-accessible.  (Interesting note: that
> means copy.c could theoretically lose the special check for superuser
> privilege for this mode, relying on standard function permissions...).
>
> I was mostly satisfied with a byzantine but otherwise semantically
> simple interface until the idea matures some more -- perhaps in
> practice -- to inform the most useful kind of convenience to support
> it.  I don't see a strong reason personally to rush into defining such
> an interface just yet, although an important interface we would have
> to define is contract a user would have to follow to enable their very
> own fully featured COPY output mode.

I think we should try to put an interface layer in place in the first
iteration. I don't really see this as having much value without that.
If we implement this strictly as a series of COPY options, we're
going to be committed to supporting that interface forever whether it
turns out to be good for anything or not. Since any such interface
would pretty much have to be superuser-only, I'm inclined to think
that there is not enough bang for the buck to justify the ongoing
maintenance effort.

One way to figure out whether we've define the COPY TARGET interface
correctly is to put together a few sample implementations and see
whether they seem functional and useful, without too many lacunas. I
think it should be possible to assess from a few such implementations
whether we have basically the right design.

> Still, the patch as-submitted is quite far from achieving one of my
> main litmus test goals: subsumption of existing COPY behavior.
> Particularly thorny was how to support the copying-to-a-file
> semantics, but I believe that the copy-options patch provide a nice
> avenue to solve this problem, as one can call a function in the
> options list and somehow pass the return value of that initializer --
> which may include a file handle -- to the byte-handling function.

It may be useful to consider whether the current behavior could be
re-implemented using some proposed extension mechanism, but I would
not put much emphasis on trying to really subsume the current behavior
into such an implementation. Certainly, we will need to continue
support the existing syntax forever, so the existing functionality
will need to be special-cased at least to that extent.

> Finally, I think a valid point was made that the patch is much more
> powerful to end users if it supports byte arrays, and there are some
> open questions as to whether this should be the only/primary supported
> mode.  I personally like the INTERNAL-type interface, as dealing with
> the StringInfo buffer used by current COPY code is very convenient
> from C and the current implementation is not very complicated yet
> avoids unnecessary copying/value coercions, but I agree there is
> definitely value in enabling the use of more normal data types.

I agree that needs some further thought. Again, a few sample
implementations might help provide clarity here. I agree that the
StringInfo representation is a bit random, but OTOH I mostly see
someone installing a few COPY TARGET implementations into their DB and
then using them over and over again. Creating a new COPY TARGET
should be relatively rare, so if they have to be written in C, we may
not care that much. On the flip side we should be looking for some
concrete gain from putting that restriction into the mix.

...Robert