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