Re: Multiple --table options for other commands

Hi all,

I see there's already a TODO for allowing pg_restore to accept
multiple --table arguments[1], but would folks support adding this
capability to various other commands which currently accept only a
single --table argument, such as clusterdb, vacuumdb, and reindexdb?

Looking at pg_dump, it seems like these other commands would want to
use the SimpleStringList / SimpleStringCell machinery which currently
lives only in pg_dump. I'm guessing that ./src/bin/pg_dump/dumputils.c
would be the right place for such common code?

Josh

[1] http://archives.postgresql.org/pgsql-hackers/2010-08/msg00689.php

On Sun, Oct 28, 2012 at 4:30 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:

> I see there's already a TODO for allowing pg_restore to accept
> multiple --table arguments[1], but would folks support adding this
> capability to various other commands which currently accept only a
> single --table argument, such as clusterdb, vacuumdb, and reindexdb?

I went ahead and cooked up a patch to allow pg_restore, clusterdb,
vacuumdb, and reindexdb to accept multiple --table switches. Hope I
didn't overlook a similar tool, but these were the only other commands
I found taking a --table argument.

If you run, say:
pg_dump -Fc --table=tab1 --table=tab2 ... |
pg_restore --table=tab1 --table=tab2 ...

without the patch, only "tab2" will be restored and pg_restore will
make no mention of this omission to the user. With the patch, both
tables should be restored.

I also added support for multiple --index options for reindexdb, since
that use-case seemed symmetrical to --table. As suggested previously,
I moved the SimpleStringList types and functions out of pg_dump.h and
common.c into dumputils.{h,c}, so that the code in ./src/bin/scripts/
could use it.

If there are no objections, I can add the patch to the next CF.

Josh

On 10/30/2012 10:14:19 PM, Josh Kupershmidt wrote:

> I went ahead and cooked up a patch to allow pg_restore, clusterdb,
> vacuumdb, and reindexdb to accept multiple --table switches. Hope I
> didn't overlook a similar tool, but these were the only other
> commands
> I found taking a --table argument.

I believe you need ellipses behind --table in the syntax summaries
of the command reference docs.

(This was all I noticed while reading the patch.)

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

Hi Josh,

I've signed up to review this patch.

I configured with assertions, built, and tested using
the attached script. It seems to do what it's supposed
to and the code looks ok to me.

The docs build. The text is reasonable.

I also diffed the output of the attached script with
the output of an unpatched head and got what I expected.

Yes, the current pg_restore silently
ignores multiple --table arguments, and seems to use the last
one. You are introducing a backwards incompatible
change here. I don't know what to do about it, other
than perhaps to have the patch go into 10.0 (!?) and
introduce a patch now that complains about multiple
--table arguments. On the other hand, perhaps it's
simply undocumented what pg_restore does when
given repeated, conflicting, arguments and we're
free to change this. Any thoughts?

On 12/10/2012 09:23:03 PM, Karl O. Pinc wrote:
> On 10/30/2012 10:14:19 PM, Josh Kupershmidt wrote:
>
> > I went ahead and cooked up a patch to allow pg_restore, clusterdb,
> > vacuumdb, and reindexdb to accept multiple --table switches. Hope I
> > didn't overlook a similar tool, but these were the only other
> > commands
> > I found taking a --table argument.
>
> I believe you need ellipses behind --table in the syntax summaries
> of the command reference docs.

I also note that the pg_dump --help output says "table(s)" so
you probably want to have pg_restore say the same now that it
takes multiple tables.

These two minor points seem to be all that needs fixing.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Tue, Dec 11, 2012 at 1:46 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> Yes, the current pg_restore silently
> ignores multiple --table arguments, and seems to use the last
> one. You are introducing a backwards incompatible
> change here. I don't know what to do about it, other
> than perhaps to have the patch go into 10.0 (!?) and
> introduce a patch now that complains about multiple
> --table arguments. On the other hand, perhaps it's
> simply undocumented what pg_restore does when
> given repeated, conflicting, arguments and we're
> free to change this. Any thoughts?

I wouldn't worry about this. I don't think we're obliged to be
bug-compatible with our own prior releases. We've made far bigger
changes for far less meritorious reasons.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> Hi Josh,
>
> I've signed up to review this patch.

Thanks!

> I configured with assertions, built, and tested using
> the attached script. It seems to do what it's supposed
> to and the code looks ok to me.
>
> The docs build. The text is reasonable.
>
> I also diffed the output of the attached script with
> the output of an unpatched head and got what I expected.

Cool test script.

> Yes, the current pg_restore silently
> ignores multiple --table arguments, and seems to use the last
> one. You are introducing a backwards incompatible
> change here. I don't know what to do about it, other
> than perhaps to have the patch go into 10.0 (!?) and
> introduce a patch now that complains about multiple
> --table arguments. On the other hand, perhaps it's
> simply undocumented what pg_restore does when
> given repeated, conflicting, arguments and we're
> free to change this. Any thoughts?

Agreed with Robert that this change should be reasonable in a major
version (i.e. 9.3).

> On 12/10/2012 09:23:03 PM, Karl O. Pinc wrote:
>> On 10/30/2012 10:14:19 PM, Josh Kupershmidt wrote:
>>
>> > I went ahead and cooked up a patch to allow pg_restore, clusterdb,
>> > vacuumdb, and reindexdb to accept multiple --table switches. Hope I
>> > didn't overlook a similar tool, but these were the only other
>> > commands
>> > I found taking a --table argument.
>>
>> I believe you need ellipses behind --table in the syntax summaries
>> of the command reference docs.

Hrm, I was following pg_dump's lead here for the .sgml docs, and
didn't see anywhere that pg_dump makes the multiple --table syntax
explicit other than in the explanatory text underneath the option.

> I also note that the pg_dump --help output says "table(s)" so
> you probably want to have pg_restore say the same now that it
> takes multiple tables.

Good catch, will fix, and ditto reindexdb's --index help output. (It
is possible that the --help output for pg_dump was worded to say
"table(s)" because one can use a "pattern" --table specification with
pg_dump, though IMO it's helpful to mention "table(s)" in the --help
output for the rest of these programs as well, as a little reminder to
the user.)

Josh

On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:

> > Yes, the current pg_restore silently
> > ignores multiple --table arguments, and seems to use the last
> > one. You are introducing a backwards incompatible
> > change here.

> Agreed with Robert that this change should be reasonable in a major
> version (i.e. 9.3).

Good by me. Seemed worth a mention.

> >> I believe you need ellipses behind --table in the syntax summaries
> >> of the command reference docs.
>
> Hrm, I was following pg_dump's lead here for the .sgml docs, and
> didn't see anywhere that pg_dump makes the multiple --table syntax
> explicit other than in the explanatory text underneath the option.

Yes. I see. I didn't look at all the command's reference pages
but did happen to look at clusterdb, which does have --table
in the syntax summary. I just checked and you need to fix
clusterdb, reindexdb, and vacuumdb.

> > I also note that the pg_dump --help output says "table(s)" so
> > you probably want to have pg_restore say the same now that it
> > takes multiple tables.
>
> Good catch, will fix, and ditto reindexdb's --index help output. (It
> is possible that the --help output for pg_dump was worded to say
> "table(s)" because one can use a "pattern" --table specification with
> pg_dump, though IMO it's helpful to mention "table(s)" in the --help
> output for the rest of these programs as well, as a little reminder
> to
> the user.)

Agreed.

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
>> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>> >> I believe you need ellipses behind --table in the syntax summaries
>> >> of the command reference docs.
>>
>> Hrm, I was following pg_dump's lead here for the .sgml docs, and
>> didn't see anywhere that pg_dump makes the multiple --table syntax
>> explicit other than in the explanatory text underneath the option.
>
> Yes. I see. I didn't look at all the command's reference pages
> but did happen to look at clusterdb, which does have --table
> in the syntax summary. I just checked and you need to fix
> clusterdb, reindexdb, and vacuumdb.

OK, I made some changes to the command synopses for these three
commands. For some reason, reindexdb.sgml was slightly different from
the other two, in that the --table and --index bits were underneath a
<group> node instead of <arg>. I've changed that one to be like the
other two, and made them all have:
<arg choice="opt" rep="repeat">

to include the ellipses after the table -- I hope that matches what
you had in mind.

Josh

Hi Josh,

The good news is that there's only this little bit of
documentation formatting to fix....

On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote:
> On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
> >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com>
> wrote:
> >> >> I believe you need ellipses behind --table in the syntax
> summaries
> >> >> of the command reference docs.

> > Yes. I see. I didn't look at all the command's reference pages
> > but did happen to look at clusterdb, which does have --table
> > in the syntax summary. I just checked and you need to fix
> > clusterdb, reindexdb, and vacuumdb.
>
> OK, I made some changes to the command synopses for these three
> commands. For some reason, reindexdb.sgml was slightly different from
> the other two, in that the --table and --index bits were underneath a
> <group> node instead of <arg>. I've changed that one to be like the
> other two, and made them all have:
> <arg choice="opt" rep="repeat">
>
> to include the ellipses after the table -- I hope that matches what
> you had in mind.

Sadly, the ... is in the wrong place in all 3. Yours looks like (for
2 examples):

vacuumdb [connection-option...] [option...] [ --table | -t table
[( column [,...] )] ...] [dbname]

reindexdb [connection-option...] [ --table | -t table ...] [ --index |
-i index ...] [dbname]

I want the ... outside the square braces, because the --table (or -t)
must repeat for each table specified. Like:

vacuumdb [connection-option...] [option...] [ --table | -t table
[( column [,...] )] ]... [dbname]

reindexdb [connection-option...] [ --table | -t table ]... [ --index |
-i index ]... [dbname]

Sorry, I don't see any examples in the existing docs of how this is
done.

It seems like what you have should work, although perhaps
somehow that's the difference between the <arg> and <group>
and you need to switch them back to how reindex used to
be. Otherwise, there's the docs at docbook.org, and
the docbook mailing lists. Or maybe the stylesheets
are broken. (Always blame the tool! It's never _our_
fault! ;-)

Have you had trouble getting this to work?
Maybe someone who knows what they're doing will step
in and give us the answer.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On 12/13/2012 12:35:14 AM, Karl O. Pinc wrote:

> On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote:
> > On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> > > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
> > >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com>
> > wrote:
> > >> >> I believe you need ellipses behind --table in the syntax
> > summaries
> > >> >> of the command reference docs.
>
>
> > > Yes. I see. I didn't look at all the command's reference pages
> > > but did happen to look at clusterdb, which does have --table
> > > in the syntax summary. I just checked and you need to fix
> > > clusterdb, reindexdb, and vacuumdb.
> >
> > OK, I made some changes to the command synopses for these three
> > commands.

> I want the ... outside the square braces, because the --table (or -t)
> must repeat for each table specified. Like:
>
> vacuumdb [connection-option...] [option...] [ --table | -t table
> [( column [,...] )] ]... [dbname]
>
> reindexdb [connection-option...] [ --table | -t table ]... [ --index
> |
>
> -i index ]... [dbname]

> Have you had trouble getting this to work?

Perhaps <arg choice="opt"><arg rep="repeat"> ... would work?

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Thu, Dec 13, 2012 at 6:05 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> On 12/13/2012 12:35:14 AM, Karl O. Pinc wrote:
>
>> On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote:
>> > On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>> > > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
>> > >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com>
>> > wrote:
>> > >> >> I believe you need ellipses behind --table in the syntax
>> > summaries
>> > >> >> of the command reference docs.
>>
>>
>> > > Yes. I see. I didn't look at all the command's reference pages
>> > > but did happen to look at clusterdb, which does have --table
>> > > in the syntax summary. I just checked and you need to fix
>> > > clusterdb, reindexdb, and vacuumdb.
>> >
>> > OK, I made some changes to the command synopses for these three
>> > commands.
>
>> I want the ... outside the square braces, because the --table (or -t)
>> must repeat for each table specified. Like:
>>
>> vacuumdb [connection-option...] [option...] [ --table | -t table
>> [( column [,...] )] ]... [dbname]
>>
>> reindexdb [connection-option...] [ --table | -t table ]... [ --index
>> |
>>
>> -i index ]... [dbname]
>
>> Have you had trouble getting this to work?
>
> Perhaps <arg choice="opt"><arg rep="repeat"> ... would work?

Well, I fooled around with the SGML for a bit and came up with
something which has the ellipses in brackets:

[ --table | -t table ] [...]

which I like a little better than not having the second set of
brackets. New version attached.

Josh

On 12/13/2012 07:37:27 PM, Josh Kupershmidt wrote:
> On Thu, Dec 13, 2012 at 6:05 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> > On 12/13/2012 12:35:14 AM, Karl O. Pinc wrote:
> >
> >> On 12/12/2012 11:04:53 PM, Josh Kupershmidt wrote:
> >> > On Wed, Dec 12, 2012 at 8:14 AM, Karl O. Pinc <kop(at)meme(dot)com>
> wrote:
> >> > > On 12/11/2012 10:25:43 PM, Josh Kupershmidt wrote:
> >> > >> On Tue, Dec 11, 2012 at 11:46 AM, Karl O. Pinc <kop(at)meme(dot)com>
> >> > wrote:
> >> > >> >> I believe you need ellipses behind --table in the syntax
> >> > summaries
> >> > >> >> of the command reference docs.
> >>
> >>
> >> > > Yes. I see. I didn't look at all the command's reference
> pages
> >> > > but did happen to look at clusterdb, which does have --table
> >> > > in the syntax summary. I just checked and you need to fix
> >> > > clusterdb, reindexdb, and vacuumdb.
> >> >
> >> > OK, I made some changes to the command synopses for these three
> >> > commands.
> >
> >> I want the ... outside the square braces, because the --table (or
> -t)
> >> must repeat for each table specified. Like:
> >>
> >> vacuumdb [connection-option...] [option...] [ --table | -t table
> >> [( column [,...] )] ]... [dbname]
> >>
> >> reindexdb [connection-option...] [ --table | -t table ]... [
> --index
> >> |
> >>
> >> -i index ]... [dbname]
> >
> >> Have you had trouble getting this to work?
> >
> > Perhaps <arg choice="opt"><arg rep="repeat"> ... would work?
>
> Well, I fooled around with the SGML for a bit and came up with
> something which has the ellipses in brackets:
>
> [ --table | -t table ] [...]
>
> which I like a little better than not having the second set of
> brackets. New version attached.

The problem is that the pg man pages would then have a
syntax different from all the other man pages in the system,
which all have ... outside of square braces.
See "man cat", e.g.
(I wonder if the problem is because the style sheets
have been tweaked to work well with sql?)

Because I don't see the traditional man page ellipsis syntax
anywhere in the pg docs, and because all the pg
client command line programs that use repeating arguments
all have a simplified syntax summary with just [options ...]
or some such, I suspect that there may be a problem
putting the ellipsis outside of the square braces.

It would be nice if we could get some guidance from someone
more familiar with the pg docbook stylesheets.

As a fallback I'd do to the clusterdb, reindexdb, and vacuumdb
syntax summaries what was done to the pg_dump and pg_restore
syntax summaries. Remove all the detail. This is sucky,
and _still_ leaves the reference pages with a syntax summary syntax
that differs from regular man pages, but because the text
is relatively information free nobody notices.

My inclination, since you can't make it work
and we don't seem to be getting any help here,
is to remove all the detail in the syntax summaries,
push it through to a committer, and get some feedback that way.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Thu, Dec 13, 2012 at 7:24 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:

> The problem is that the pg man pages would then have a
> syntax different from all the other man pages in the system,
> which all have ... outside of square braces.
> See "man cat", e.g.

FWIW, `man cat` on my OS X machine has synopsis:

cat [-benstuv] [file ...]

though I see:

cat [OPTION] [FILE]...

on Debian.

> (I wonder if the problem is because the style sheets
> have been tweaked to work well with sql?)
>
> Because I don't see the traditional man page ellipsis syntax
> anywhere in the pg docs, and because all the pg
> client command line programs that use repeating arguments
> all have a simplified syntax summary with just [options ...]
> or some such, I suspect that there may be a problem
> putting the ellipsis outside of the square braces.

Yeah, I tried a few different ways and didn't manage to get:
[ --table | -t table ] ...

> It would be nice if we could get some guidance from someone
> more familiar with the pg docbook stylesheets.
>
> As a fallback I'd do to the clusterdb, reindexdb, and vacuumdb
> syntax summaries what was done to the pg_dump and pg_restore
> syntax summaries. Remove all the detail. This is sucky,
> and _still_ leaves the reference pages with a syntax summary syntax
> that differs from regular man pages, but because the text
> is relatively information free nobody notices.

That should be how the v2 patch has it.

> My inclination, since you can't make it work
> and we don't seem to be getting any help here,
> is to remove all the detail in the syntax summaries,
> push it through to a committer, and get some feedback that way.

If someone out there feels that the formatting of these commands'
synopses should look like:
[ --table | -t table ] ...

and knows how to massage the SGML to get that, I'm happy to
accommodate the change. Otherwise, I think either the v4 or v2 patch
should be acceptable.

Josh

On 12/13/2012 09:24:24 PM, Josh Kupershmidt wrote:
> On Thu, Dec 13, 2012 at 7:24 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:

> > As a fallback I'd do to the clusterdb, reindexdb, and vacuumdb
> > syntax summaries what was done to the pg_dump and pg_restore
> > syntax summaries. Remove all the detail. This is sucky,
> > and _still_ leaves the reference pages with a syntax summary syntax
> > that differs from regular man pages, but because the text
> > is relatively information free nobody notices.
>
> That should be how the v2 patch has it.

No. The v2 patch does not touch the syntax synopsis.

>
> > My inclination, since you can't make it work
> > and we don't seem to be getting any help here,
> > is to remove all the detail in the syntax summaries,
> > push it through to a committer, and get some feedback that way.
>
> If someone out there feels that the formatting of these commands'
> synopses should look like:
> [ --table | -t table ] ...
>
> and knows how to massage the SGML to get that, I'm happy to
> accommodate the change. Otherwise, I think either the v4 or v2 patch
> should be acceptable.

My brain seems to have turned itself on. I went and (re)read
the docbook manual and was able to come up with this,
which works:

<arg choice="plain" rep="repeat"><arg choice="opt">
<group choice="plain">
<arg choice="plain"><option>--table</option></arg>
<arg choice="plain"><option>-t</option></arg>
</group>
<replaceable>table</replaceable> </arg></arg>

Yay! (indentation et-al aside)

Sorry to be so persnickety, and unhelpful until now.
It seemed like it should be doable, but something
was going wrong between keyboard and chair. I guess
I should be doing this when better rested.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> My brain seems to have turned itself on. I went and (re)read
> the docbook manual and was able to come up with this,
> which works:
>
>
> <arg choice="plain" rep="repeat"><arg choice="opt">
> <group choice="plain">
> <arg choice="plain"><option>--table</option></arg>
> <arg choice="plain"><option>-t</option></arg>
> </group>
> <replaceable>table</replaceable> </arg></arg>
>
> Yay! (indentation et-al aside)

That does seem to work, thanks for figuring out the syntax.

> Sorry to be so persnickety, and unhelpful until now.
> It seemed like it should be doable, but something
> was going wrong between keyboard and chair. I guess
> I should be doing this when better rested.

No problem, here is v5 with changed synopses.

Josh

On 12/13/2012 11:02:56 PM, Josh Kupershmidt wrote:
> On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:

> > Sorry to be so persnickety, and unhelpful until now.
> > It seemed like it should be doable, but something
> > was going wrong between keyboard and chair. I guess
> > I should be doing this when better rested.
>
> No problem, here is v5 with changed synopses.

The clusterdb synopsis had tabs in it, which I understand
is frowned upon in the docs. I've fixed this.
It looks good to me, passes check and so forth.

Attached is a v6 patch, with no tabs in docs and based
off the latest head.

I'm marking it ready for committer.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Fri, Dec 14, 2012 at 4:14 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
> On 12/13/2012 11:02:56 PM, Josh Kupershmidt wrote:
>> On Thu, Dec 13, 2012 at 9:03 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>
>> > Sorry to be so persnickety, and unhelpful until now.
>> > It seemed like it should be doable, but something
>> > was going wrong between keyboard and chair. I guess
>> > I should be doing this when better rested.
>>
>> No problem, here is v5 with changed synopses.
>
> The clusterdb synopsis had tabs in it, which I understand
> is frowned upon in the docs. I've fixed this.
> It looks good to me, passes check and so forth.
>
> Attached is a v6 patch, with no tabs in docs and based
> off the latest head.
>
> I'm marking it ready for committer.

Thanks. Applied, with only some small whitespace changes.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/