Multiple -t options for pg_dump

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!

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