Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] Add switches for DELIMITER and NULL in pg_dump COPY

From: Neil Conway <neilc(at)samurai(dot)com>
To: David Fetter <david(at)fetter(dot)org>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>, PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [PATCHES] Add switches for DELIMITER and NULL in pg_dump COPY
Date: 2006-03-08 16:10:04
Message-ID: 1141834204.20504.57.camel@localhost.localdomain (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
On Wed, 2006-03-08 at 07:47 -0800, David Fetter wrote:
> From the earlier discussion, it appears that there is a variety of
> opinions on what the COPY delimiter should be in pg_dump.  This patch
> allows people to set it and the NULL string.

I'm still not convinced there is a reasonable use-case for this feature.
I can't recall: did the previous discussion conclude that we actually
want this functionality?

> *** src/bin/pg_dump/pg_dump.c   5 Mar 2006 15:58:50 -0000       1.433
> --- src/bin/pg_dump/pg_dump.c   6 Mar 2006 07:32:12 -0000
> ***************
> *** 114,119 ****
> --- 114,125 ----
>   /* flag to turn on/off dollar quoting */
>   static int    disable_dollar_quoting = 0;
>   
> + /* Things used when caller invokes COPY options. */
> + #define ARG_COPY_DELIMITER 2
> + #define ARG_COPY_NULL 3
> + char *copy_delimiter = "\t";
> + char *copy_null;
> + 

The variables should be declared static.
  
>   static void help(const char *progname);
>   static NamespaceInfo *findNamespace(Oid nsoid, Oid objoid);
> ***************
> *** 181,186 ****
> --- 187,193 ----
>                                  ExecStatusType expected);
>   
>   
> + 
>   int
>   main(int argc, char **argv)
>   {
> ***************
> *** 211,217 ****
>         char       *outputSuperuser = NULL;
>   
>         RestoreOptions *ropt;
> ! 
>         static struct option long_options[] = {
>                 {"data-only", no_argument, NULL, 'a'},
>                 {"blobs", no_argument, NULL, 'b'},
> --- 218,224 ----
>         char       *outputSuperuser = NULL;
>   
>         RestoreOptions *ropt;
> !       
>         static struct option long_options[] = {
>                 {"data-only", no_argument, NULL, 'a'},
>                 {"blobs", no_argument, NULL, 'b'},

Please review patches and eliminate content-free hunks like these before
submitting.

> ***************
> *** 427,432 ****
> --- 464,479 ----
>                 }
>         }
>   
> +       if (copy_null == NULL)
> +               copy_null = malloc(3);
> +               strcpy(copy_null, "\\N");

You're missing some braces.

> +       if (strstr(copy_null, copy_delimiter))
> +       {
> +               fprintf(stderr, _("In %s, the NULL AS string cannot
> contain the COPY delimiter.\n"), progname);
> +               exit(1);
> +       }

I'm not sure as to whether you should be using write_msg() or fprintf()
here, but we should probably pick one and be consistent. Also ISTM we
should be to refactor the code to use exit_nicely() anyway, provided
that g_conn is initialized to NULL before we have connected to the DB.

> ***************
> *** 702,707 ****
> --- 749,756 ----
>                          "                           use SESSION
> AUTHORIZATION commands instead of\n"
>                          "                           OWNER TO commands
> \n"));
>   
> +       printf(_("  --copy-delimiter         string to use as column
> DELIMITER in COPY statements\n"));

Capitalizing "DELIMITER" here is not good style, IMHO: it is just a
normal word.

> *** 844,849 ****
> --- 893,904 ----
>         int                     ret;
>         char       *copybuf;
>         const char *column_list;
> +       char *local_copy_delimiter;
> +       char *local_copy_null;
> +       local_copy_delimiter = malloc(2*strlen(copy_delimiter)+1);
> +       PQescapeString (local_copy_delimiter, copy_delimiter,
> 2*strlen(copy_delimiter)+1);
> +       local_copy_null = malloc(2*strlen(copy_null)+1);
> +       PQescapeString (local_copy_null, copy_null,
> 2*strlen(copy_null)+1);

Spacing: spaces around operands to mathematical operators, no spaces
before the parameter list to a function call.

You should also fix this compiler warning:

[...]/pg_dump.c:440: warning: format '%d' expects type 'int', but
argument 4 has type 'size_t'

-Neil



In response to

Responses

pgsql-hackers by date

Next:From: Jim C. NasbyDate: 2006-03-08 16:12:19
Subject: Re: Merge algorithms for large numbers of "tapes"
Previous:From: Tom LaneDate: 2006-03-08 16:03:00
Subject: Re: Add switches for DELIMITER and NULL in pg_dump COPY

pgsql-patches by date

Next:From: David FetterDate: 2006-03-08 16:14:54
Subject: Re: Add switches for DELIMITER and NULL in pg_dump COPY
Previous:From: Tom LaneDate: 2006-03-08 16:03:00
Subject: Re: Add switches for DELIMITER and NULL in pg_dump COPY

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group