Thread: Multiple -t options for pg_dump
Folks, Please find enclosed a patch against CVS TIP that allows people to specify more than one table via pg_dump -t. Cheers, D -- David Fetter david@fetter.org http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote!
Attachment
A few minor comments are below. This is for 8.2, right?
On Tue, 2005-20-09 at 08:51 -0700, David Fetter wrote:
> /* obsolete as of 7.3: */
> static Oid g_last_builtin_oid; /* value of the last builtin oid
> */
>
> ! static char **selectTableNames = NULL; /* name(s) of
> specified table(s) to dump */
> ! int tab_max = 0, tab_idx = 0;
> ! static char *selectSchemaName = NULL; /* name(s) of specified
> schema(ta) to dump */
> ! int schem_max = 0, schem_idx = 0;
>
> char g_opaque_type[10]; /* name for the opaque type */
AFAICS the schema changes are unused. However, it would be worth
enhancing -n to allow it to be specified multiple times as well. And to
nit-pick, "tab_max" and "tab_idx" should be static. Also, within the PG
source the plural of "schema" is "schemas".
> ! case 't': /* Dump data
> for th(is|ese) table(s) only */
> ! if (tab_idx == tab_max) {
> ! tab_max += 32;
> ! if ( (selectTableNames =
> realloc(selectTableNames, tab_max*sizeof(char *))) == 0)
> ! exit(-1);
> ! }
If you're going to check for calloc failure, you may as well check for
strdup failure as well. Also, exit(1) would be more consistent with the
rest of pg_dump. But personally I wouldn't bother checking for calloc
(or strdup) failure, as the rest of pg_dump doesn't.
> *** 2414,2420 ****
> {
> PGresult *res;
> int ntups;
> ! int i;
> PQExpBuffer query = createPQExpBuffer();
> PQExpBuffer delqry = createPQExpBuffer();
> PQExpBuffer lockquery = createPQExpBuffer();
> --- 2427,2433 ----
> {
> PGresult *res;
> int ntups;
> ! int i,j;
> PQExpBuffer query = createPQExpBuffer();
> PQExpBuffer delqry = createPQExpBuffer();
> PQExpBuffer lockquery = createPQExpBuffer();
You should move j's definition to the scope closest to its use.
> --- 2706,2724 ----
> * simplistic since we don't fully check the combination of -n
> and -t
> * switches.)
> */
> ! if (selectTableNames != NULL)
> {
> ! for (i = 0; i < ntups; i++) {
> ! for (j = 0; j < tab_idx; j++) {
> ! if (strcmp(tblinfo[i].dobj.name,
> selectTableNames[j]) == 0)
> ! goto check_match;
> ! }
> ! }
> /* Didn't find a match */
> ! check_match: if (i == ntups)
> {
> write_msg(NULL, "specified table \"%s\" does
> not exist\n",
> ! selectTableNames[j]);
> exit_nicely();
> }
> }
AFAICS the goto is unnecessary -- just break out of the loop when you
find a match, and test i == ntups outside the loop.
-Neil
On Tue, Sep 20, 2005 at 12:15:23PM -0400, Neil Conway wrote:
> A few minor comments are below. This is for 8.2, right?
I am hoping to make a case for inclusion in 8.1. The code is
completely isolated in one command and does not affect anyone who
does not use the new features.
> On Tue, 2005-20-09 at 08:51 -0700, David Fetter wrote:
> > /* obsolete as of 7.3: */
> > static Oid g_last_builtin_oid; /* value of the last builtin oid
> > */
> >
> > ! static char **selectTableNames = NULL; /* name(s) of
> > specified table(s) to dump */
> > ! int tab_max = 0, tab_idx = 0;
> > ! static char *selectSchemaName = NULL; /* name(s) of specified
> > schema(ta) to dump */
> > ! int schem_max = 0, schem_idx = 0;
> >
> > char g_opaque_type[10]; /* name for the opaque type */
>
> AFAICS the schema changes are unused. However, it would be worth
> enhancing -n to allow it to be specified multiple times as well. And
> to nit-pick, "tab_max" and "tab_idx" should be static. Also, within
> the PG source the plural of "schema" is "schemas".
OK, will fix when I get home from work.
>
> > ! case 't': /* Dump data
> > for th(is|ese) table(s) only */
> > ! if (tab_idx == tab_max) {
> > ! tab_max += 32;
> > ! if ( (selectTableNames =
> > realloc(selectTableNames, tab_max*sizeof(char *))) == 0)
> > ! exit(-1);
> > ! }
>
> If you're going to check for calloc failure, you may as well check for
> strdup failure as well. Also, exit(1) would be more consistent with the
> rest of pg_dump. But personally I wouldn't bother checking for calloc
> (or strdup) failure, as the rest of pg_dump doesn't.
OK :)
> > *** 2414,2420 ****
> > {
> > PGresult *res;
> > int ntups;
> > ! int i;
> > PQExpBuffer query = createPQExpBuffer();
> > PQExpBuffer delqry = createPQExpBuffer();
> > PQExpBuffer lockquery = createPQExpBuffer();
> > --- 2427,2433 ----
> > {
> > PGresult *res;
> > int ntups;
> > ! int i,j;
> > PQExpBuffer query = createPQExpBuffer();
> > PQExpBuffer delqry = createPQExpBuffer();
> > PQExpBuffer lockquery = createPQExpBuffer();
>
> You should move j's definition to the scope closest to its use.
OK
>
> > --- 2706,2724 ----
> > * simplistic since we don't fully check the combination of -n
> > and -t
> > * switches.)
> > */
> > ! if (selectTableNames != NULL)
> > {
> > ! for (i = 0; i < ntups; i++) {
> > ! for (j = 0; j < tab_idx; j++) {
> > ! if (strcmp(tblinfo[i].dobj.name,
> > selectTableNames[j]) == 0)
> > ! goto check_match;
> > ! }
> > ! }
> > /* Didn't find a match */
> > ! check_match: if (i == ntups)
> > {
> > write_msg(NULL, "specified table \"%s\" does
> > not exist\n",
> > ! selectTableNames[j]);
> > exit_nicely();
> > }
> > }
>
> AFAICS the goto is unnecessary -- just break out of the loop when you
> find a match, and test i == ntups outside the loop.
I tried that. How do I break out of both loops without a goto?
Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
On Tue, 2005-20-09 at 11:33 -0700, David Fetter wrote:
> I am hoping to make a case for inclusion in 8.1. The code is
> completely isolated in one command and does not affect anyone who
> does not use the new features.
On those grounds we could include a lot of new features during the 8.1
beta period. IMHO it is too late for new features at this point in the
release cycle.
> > AFAICS the goto is unnecessary -- just break out of the loop when you
> > find a match, and test i == ntups outside the loop.
>
> I tried that. How do I break out of both loops without a goto?
Oh, yeah, I guess you need a goto, but you can at least avoid rechecking
the loop condition:
for (;;)
{
for (;;)
{
if (match_found)
goto done;
}
}
/* If we got here, no match */
error();
done: /* Otherwise we're good */
-Neil
On Tue, Sep 20, 2005 at 11:33:39AM -0700, David Fetter wrote: > On Tue, Sep 20, 2005 at 12:15:23PM -0400, Neil Conway wrote: > > A few minor comments are below. This is for 8.2, right? > > I am hoping to make a case for inclusion in 8.1. The code is > completely isolated in one command and does not affect anyone who > does not use the new features. On these grounds, this feature should have been included back when 8.1 started, because a patch to do this was posted when 8.0 was in beta ... In fact, IIRC, this is the third time a patch for this is posted, all of them in late beta :-( -- Alvaro Herrera Architect, http://www.EnterpriseDB.com "La grandeza es una experiencia transitoria. Nunca es consistente. Depende en gran parte de la imaginación humana creadora de mitos" (Irulan)
David Fetter <david@fetter.org> writes:
> I am hoping to make a case for inclusion in 8.1.
We are months past feature freeze. Don't waste your breath.
regards, tom lane
On Tue, Sep 20, 2005 at 05:55:52PM -0400, Tom Lane wrote: > David Fetter <david@fetter.org> writes: > > I am hoping to make a case for inclusion in 8.1. > > We are months past feature freeze. Don't waste your breath. OK, I won't. Cheers, D -- David Fetter david@fetter.org http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote!
On Tue, Sep 20, 2005 at 12:15:23PM -0400, Neil Conway wrote: > A few minor comments are below. This is for 8.2, right? Yes. Please find enclosed round 2 of the patch. It implements multiple -n's and multiple -t's, although I think it's broken for multiple -n's with no -t's. Also included are a few wrapper functions for doing the right thing should any of calloc(), malloc(), realloc() and strdup() fail. Comments? Suggestions? Cheers, D -- David Fetter david@fetter.org http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote!
Attachment
On Fri, 2005-23-09 at 00:14 -0700, David Fetter wrote: > Yes. Please find enclosed round 2 of the patch. It implements > multiple -n's and multiple -t's, although I think it's broken for > multiple -n's with no -t's. BTW, have you read the (extensive) prior discussion of this topic? For example, http://archives.postgresql.org/pgsql-patches/2004-07/msg00229.php There have been *at least* two previous implementations of the patch posted, so it might be worth taking a look at those other versions as well. > Also included are a few wrapper functions for doing the right thing > should any of calloc(), malloc(), realloc() and strdup() fail. Can you post this as a separate patch, please? It is unrelated, and the bulk of these changes make it difficult to review the "-t" option work. *** src/bin/pg_dump/pg_dump.h 5 Sep 2005 23:50:49 -0000 1.121 --- src/bin/pg_dump/pg_dump.h 23 Sep 2005 07:13:02 -0000 *************** *** 387,390 **** --- 387,395 ---- extern CastInfo *getCasts(int *numCasts); extern void getTableAttrs(TableInfo *tbinfo, int numTables); + void *pg_calloc(size_t nmemb, size_t size); + void *pg_malloc(size_t size); + void *pg_realloc(void *ptr, size_t size); + char *pg_strdup(const char *s); By convention, global function declarations use the "extern" keyword, so you want: extern void *pg_malloc(size_t sz); etc. -Neil