Re: pg_restore multiple --function options

Hi,

While looking at the pg_restore code, I noticed that while it supports
specifying multiple --table options to restore several tables, it does
not support multiple --function options. Or --index, --schema, or --trigger.

The support for multiple --table options was added in 9.3, in January.
See
http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
Was there a particular reason for only doing it for --table, or was it
just an oversight or lack of interest? No doubt that --table is the most
interesting one, but IMHO the other options should behave the same, for
the sake of consistency.

- Heikki

On Mon, Aug 26, 2013 at 10:29:06PM +0300, Heikki Linnakangas wrote:
> Hi,
>
> While looking at the pg_restore code, I noticed that while it
> supports specifying multiple --table options to restore several
> tables, it does not support multiple --function options. Or --index,
> --schema, or --trigger.
>
> The support for multiple --table options was added in 9.3, in
> January. See http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
> Was there a particular reason for only doing it for --table, or was
> it just an oversight or lack of interest? No doubt that --table is
> the most interesting one, but IMHO the other options should behave
> the same, for the sake of consistency.

+1 for making them consistent. There will also be an improvement in
usability.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Aug 27, 2013 at 5:17 AM, David Fetter <david(at)fetter(dot)org> wrote:
> On Mon, Aug 26, 2013 at 10:29:06PM +0300, Heikki Linnakangas wrote:
>> Hi,
>>
>> While looking at the pg_restore code, I noticed that while it
>> supports specifying multiple --table options to restore several
>> tables, it does not support multiple --function options. Or --index,
>> --schema, or --trigger.
>>
>> The support for multiple --table options was added in 9.3, in
>> January. See http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
>> Was there a particular reason for only doing it for --table, or was
>> it just an oversight or lack of interest? No doubt that --table is
>> the most interesting one, but IMHO the other options should behave
>> the same, for the sake of consistency.
>
> +1 for making them consistent. There will also be an improvement in
> usability.
+1. It would be good to have consistency there for all the objects.
--
Michael

On 27.08.2013 03:26, Michael Paquier wrote:
> On Tue, Aug 27, 2013 at 5:17 AM, David Fetter<david(at)fetter(dot)org> wrote:
>> On Mon, Aug 26, 2013 at 10:29:06PM +0300, Heikki Linnakangas wrote:
>>> While looking at the pg_restore code, I noticed that while it
>>> supports specifying multiple --table options to restore several
>>> tables, it does not support multiple --function options. Or --index,
>>> --schema, or --trigger.
>>>
>>> The support for multiple --table options was added in 9.3, in
>>> January. See http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
>>> Was there a particular reason for only doing it for --table, or was
>>> it just an oversight or lack of interest? No doubt that --table is
>>> the most interesting one, but IMHO the other options should behave
>>> the same, for the sake of consistency.
>>
>> +1 for making them consistent. There will also be an improvement in
>> usability.
> +1. It would be good to have consistency there for all the objects.

Would anyone object to backpatching that change to 9.3 ? The risk seems
very small, and it would be good to do the other options in the same
release as --table. It was an oversight to only do it for --table in 9.3.

Assuming no objections, I'll apply the attached patch to 9.3 and master
later tonight.

- Heikki

On Tue, Aug 27, 2013 at 08:14:32PM +0300, Heikki Linnakangas wrote:
> On 27.08.2013 03:26, Michael Paquier wrote:
> >On Tue, Aug 27, 2013 at 5:17 AM, David Fetter<david(at)fetter(dot)org> wrote:
> >>On Mon, Aug 26, 2013 at 10:29:06PM +0300, Heikki Linnakangas wrote:
> >>>While looking at the pg_restore code, I noticed that while it
> >>>supports specifying multiple --table options to restore several
> >>>tables, it does not support multiple --function options. Or --index,
> >>>--schema, or --trigger.
> >>>
> >>>The support for multiple --table options was added in 9.3, in
> >>>January. See http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
> >>>Was there a particular reason for only doing it for --table, or was
> >>>it just an oversight or lack of interest? No doubt that --table is
> >>>the most interesting one, but IMHO the other options should behave
> >>>the same, for the sake of consistency.
> >>
> >>+1 for making them consistent. There will also be an improvement in
> >>usability.
> >+1. It would be good to have consistency there for all the objects.
>
> Would anyone object to backpatching that change to 9.3 ?

I'm sure someone will, but I'm not that someone. It's not as though
the added syntax could have produced anything but a runtime error if
someone had it deployed, and the improvements both in usefulness and
consistency are compelling.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 27.08.2013 03:26, Michael Paquier wrote:
>> On Tue, Aug 27, 2013 at 5:17 AM, David Fetter<david(at)fetter(dot)org> wrote:
>>> On Mon, Aug 26, 2013 at 10:29:06PM +0300, Heikki Linnakangas wrote:
>>>> While looking at the pg_restore code, I noticed that while it
>>>> supports specifying multiple --table options to restore several
>>>> tables, it does not support multiple --function options. Or --index,
>>>> --schema, or --trigger.
>>>>
>>>> The support for multiple --table options was added in 9.3, in
>>>> January. See http://www.postgresql.org/message-id/CAK3UJRG+yV1mK5twLfKVMCwXH4f6PnJou6Rn=ecabyfQH1vVHg@mail.gmail.com.
>>>> Was there a particular reason for only doing it for --table, or was
>>>> it just an oversight or lack of interest? No doubt that --table is
>>>> the most interesting one, but IMHO the other options should behave
>>>> the same, for the sake of consistency.

>>> +1 for making them consistent. There will also be an improvement in
>>> usability.
>> +1. It would be good to have consistency there for all the objects.

> Would anyone object to backpatching that change to 9.3 ? The risk seems
> very small, and it would be good to do the other options in the same
> release as --table. It was an oversight to only do it for --table in 9.3.

> Assuming no objections, I'll apply the attached patch to 9.3 and master
> later tonight.

I object, strongly. This is a feature addition, and has no business going
in post-rc1, especially with no time for review.

As far as the function case goes, I'm not really thrilled about layering
more functionality on that until we've come to some understanding about
how to select from a group of overloaded functions. It may be that this
is orthogonal to that issue ... or maybe not. I don't have any objection
to fixing the non-function cases, as long as it's only in HEAD.

regards, tom lane

On 27.08.2013 21:56, Tom Lane wrote:
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> writes:
>> Would anyone object to backpatching that change to 9.3 ? The risk seems
>> very small, and it would be good to do the other options in the same
>> release as --table. It was an oversight to only do it for --table in 9.3.
>
>> Assuming no objections, I'll apply the attached patch to 9.3 and master
>> later tonight.
>
> I object, strongly. This is a feature addition, and has no business going
> in post-rc1, especially with no time for review.

Ok.

> As far as the function case goes, I'm not really thrilled about layering
> more functionality on that until we've come to some understanding about
> how to select from a group of overloaded functions. It may be that this
> is orthogonal to that issue ... or maybe not. I don't have any objection
> to fixing the non-function cases, as long as it's only in HEAD.

Huh, what's that issue?

As the code stands, you have to pass the argument types to the
--function flag, ie. --function="myfunc(integer)". It's annoyingly picky
about the spelling, as the it has to match exactly what pg_dump prints,
but it does handle selecting one function from a group of overloaded
ones. And that really is orthogonal to whether or not you can give
multiple --function arguments.

- Heikki

On 08/27/2013 10:14 AM, Heikki Linnakangas wrote:
> Would anyone object to backpatching that change to 9.3 ? The risk seems
> very small, and it would be good to do the other options in the same
> release as --table. It was an oversight to only do it for --table in 9.3.
>
> Assuming no objections, I'll apply the attached patch to 9.3 and master
> later tonight.

That will pretty much force us to do an RC2, and therefore push back
final release date.

Just for consideration.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> As the code stands, you have to pass the argument types to the
> --function flag, ie. --function="myfunc(integer)". It's annoyingly picky
> about the spelling, as the it has to match exactly what pg_dump prints,
> but it does handle selecting one function from a group of overloaded
> ones.

Oh --- OK, I was misremembering. I recalled that people weren't happy
with the handling of --function, but had the details wrong.

[pokes around]

I think really the issues are (1) it only works in pg_restore, not
pg_dump, and (2) there's no wildcard matching (the pickiness about
argument type name spelling being perhaps a subset of that).

It's probably true that accepting multiple patterns doesn't preclude
solving either of those, and indeed might help users work around (2).
So nevermind that objection. But I still say this is all too late
for 9.3.

regards, tom lane

On Tue, Aug 27, 2013 at 10:35:40PM +0300, Heikki Linnakangas wrote:
> On 27.08.2013 21:56, Tom Lane wrote:
> >Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> writes:
> >>Would anyone object to backpatching that change to 9.3 ? The risk seems
> >>very small, and it would be good to do the other options in the same
> >>release as --table. It was an oversight to only do it for --table in 9.3.
> >
> >>Assuming no objections, I'll apply the attached patch to 9.3 and master
> >>later tonight.
> >
> >I object, strongly. This is a feature addition, and has no business going
> >in post-rc1, especially with no time for review.
>
> Ok.
>
> >As far as the function case goes, I'm not really thrilled about layering
> >more functionality on that until we've come to some understanding about
> >how to select from a group of overloaded functions. It may be that this
> >is orthogonal to that issue ... or maybe not. I don't have any objection
> >to fixing the non-function cases, as long as it's only in HEAD.
>
> Huh, what's that issue?
>
> As the code stands, you have to pass the argument types to the
> --function flag, ie. --function="myfunc(integer)". It's annoyingly
> picky about the spelling, as the it has to match exactly what
> pg_dump prints, but it does handle selecting one function from a
> group of overloaded ones. And that really is orthogonal to whether
> or not you can give multiple --function arguments.

Come to think of it, some kind of recognition that functions can come
in several flavors would be awesome, e.g.

--function=myfunc\*

which would capture all variants of myfunc.

Let the bikeshedding begin!

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Aug 27, 2013 at 1:14 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Assuming no objections, I'll apply the attached patch to 9.3 and master
> later tonight.

Just a little stylistic nitpick: could we pluralize the --help outputs
for the modified options so that they make clear that multiple
specifications are supported. E.g. "restore named index(es)" instead
of just "restore named index". The last round of such changes tried to
make such --help tweaks, it would be nice to keep 'em consistent.

Josh

On Thu, Sep 5, 2013 at 09:15:09PM -0400, Josh Kupershmidt wrote:
> On Tue, Aug 27, 2013 at 1:14 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
> > Assuming no objections, I'll apply the attached patch to 9.3 and master
> > later tonight.
>
> Just a little stylistic nitpick: could we pluralize the --help outputs
> for the modified options so that they make clear that multiple
> specifications are supported. E.g. "restore named index(es)" instead
> of just "restore named index". The last round of such changes tried to
> make such --help tweaks, it would be nice to keep 'em consistent.

Agreed. Attached patch applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +