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

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