[update] Re: Patch to bring \copy syntax more in line with SQL copy

Please find a pair of updated patches attached.

The first is against src/bin/pgsql/copy.c  It changes the acceptable
syntax for \copy to allow the following:

[ [with|using] delimiter[s] [as] delimiter ]

Improvements over the previously posted patch:
1) c++ style comments removed.
2) Comments with explanation of the syntax have been updated.
3) Cases of dangling [with|using] will be caught and complained about.
4) The optional [as] is handled.

As far as I can tell, this should make the \copy syntax equivalent to the
SQL copy syntax (as described in the docs) while still maintaining
backward compatibility with older syntaxes.

Lucky for me, I'm working on a project that uses \copy to import a lot of
test data, so I made it a point to try every combination of the above
syntax that I could imagine, as well as testing the dangling [with|using]
to ensure it complained.  It works properly as well as I can tell.

The second is against doc/src/sgml/ref/psql-ref.sgml, and only changes the
explaination of the \copy syntax.  I can't seem to get the doc build process
to work on my working copy, so I can't be sure that this is OK, but it's
a trivial enough change that it's probably right.

Both patches are against the most recent versions in CVS at the time of
diffing.

Bill Moran wrote:
> Neil Conway wrote:
>
>> A few quick comments:
>>
>> - don't use C++-style comments
>
>
> Oops ... sorry ... been doing too much PHP.
>
> BTW: Why is this frowned apon so?  Are there compilers that have
> problems with it?  In my own code, I generally use // and /* */
> interchangeably, and I've never had any problems (with C).  Yet,
> this isn't the first time I've submitted a patch and had it
> pointed out that I shouldn't use C++ comments in C.
>
>> - please update the documentation
>
>
> I'm not aware that the documentation needs updated.  I changed the
> code to be up to date with the docs I found.  If there's docs that
> need updated, please point me specifically to them and I'll be
> happy to generate a patch to the best of my ability.
>
>> - update the comment at the top of copy.c
>
>
> Oops ... got it.
>
>> Bill Moran <wmoran@potentialtech.com> writes:
>>
>>> Ideally, this should cover all cases of old and new syntax, except
>>> where "AS" is present.
>>
>>
>> ISTM it would be easy to allow for an optional 'AS' token following
>> the 'DELIMITER[S]' token.
>
>
> Well ... keep in mind that I'm not as familiar with the code as you ...
> this is my first hack into PostgreSQL, and I'm not terribly familiar
> with the code yet.
>
> Now that I'm looking over the code for strtokx(), I'm seeing that
> you're absolutely right, it won't take much to accept the optional
> AS.
>
>>> The only drawback I can see is that \copy is now more liberal in
>>> what it accepts, and may accept incomplete statements without
>>> issuing an error (i.e. a WITH or USING clause at the end of a \copy
>>> statement will simply be ignored, and no error generated)
>>
>>
>> Why can't you check for this?
>
>
> I suppose I can.  It's just that the code falls together so simply
> without the check, and I can't see anyway to keep it that simple while
> still checking ... but I suppose it'd be better to have it fail
> properly than to save a few lines of code.
>
> Thanks for the quick feedback.  I hope you didn't see that post to
> hackers and take it as me bitching and whining or anything, it's just
> that I'm not 100% familiar with how things flow within the PostgreSQL
> group, and I was curious.
>
> A revised patch will be forthcoming, as soon as I find some time ...
> a day or two probably, over the weekend as worst case scenerio.
>


--
Bill Moran
Potential Technologies
http://www.potentialtech.com
*** copy.c.old    Sat Jan 17 22:17:58 2004
--- copy.c    Sat Jan 24 11:21:55 2004
***************
*** 35,41 ****
   * parse_slash_copy
   * -- parses \copy command line
   *
!  * Accepted syntax: \copy table [(columnlist)] [with oids] from|to filename [with ] [ oids ] [ delimiter char] [ null
asstring ] 
   * (binary is not here yet)
   *
   * Old syntax for backward compatibility: (2002-06-19):
--- 35,41 ----
   * parse_slash_copy
   * -- parses \copy command line
   *
!  * Accepted syntax: \copy table [(columnlist)] [with oids] from|to filename [ with ] [ oids ] [[ with | using ]
delimiter[s][as] char] [ null as string ] 
   * (binary is not here yet)
   *
   * Old syntax for backward compatibility: (2002-06-19):
***************
*** 101,106 ****
--- 101,107 ----
      char       *line;
      char       *token;
      const char *whitespace = " \t\n\r";
+     int        havewith = 0;

      if (args)
          line = xstrdup(args);
***************
*** 226,284 ****
      token = strtokx(NULL, whitespace, NULL, NULL,
                      0, false, pset.encoding);

!     /*
!      * Allows old COPY syntax for backward compatibility 2002-06-19
!      */
!     if (token && strcasecmp(token, "using") == 0)
      {
          token = strtokx(NULL, whitespace, NULL, NULL,
                          0, false, pset.encoding);
!         if (!(token && strcasecmp(token, "delimiters") == 0))
!             goto error;
          token = strtokx(NULL, whitespace, NULL, "'",
                          '\\', false, pset.encoding);
          if (!token)
              goto error;
          result->delim = xstrdup(token);
-         token = strtokx(NULL, whitespace, NULL, NULL,
-                         0, false, pset.encoding);
-     }
-
-     if (token)
-     {
-         if (strcasecmp(token, "with") != 0)
-             goto error;
-         while ((token = strtokx(NULL, whitespace, NULL, NULL,
-                                 0, false, pset.encoding)) != NULL)
-         {
-             if (strcasecmp(token, "delimiter") == 0)
-             {
-                 token = strtokx(NULL, whitespace, NULL, "'",
-                                 '\\', false, pset.encoding);
-                 if (token && strcasecmp(token, "as") == 0)
-                     token = strtokx(NULL, whitespace, NULL, "'",
-                                     '\\', false, pset.encoding);
-                 if (token)
-                     result->delim = xstrdup(token);
-                 else
-                     goto error;
-             }
-             else if (strcasecmp(token, "null") == 0)
-             {
-                 token = strtokx(NULL, whitespace, NULL, "'",
-                                 '\\', false, pset.encoding);
-                 if (token && strcasecmp(token, "as") == 0)
-                     token = strtokx(NULL, whitespace, NULL, "'",
-                                     '\\', false, pset.encoding);
-                 if (token)
-                     result->null = xstrdup(token);
-                 else
-                     goto error;
-             }
-             else
-                 goto error;
-         }
      }

      free(line);

--- 227,266 ----
      token = strtokx(NULL, whitespace, NULL, NULL,
                      0, false, pset.encoding);

!     /* Discard both "with" and "using" as syntactically neutral */
!     if (token &&
!         (strcasecmp(token, "using") == 0 ||
!          strcasecmp(token, "with") == 0))
      {
          token = strtokx(NULL, whitespace, NULL, NULL,
                          0, false, pset.encoding);
!         havewith = 1;
!     }
!
!     /*
!      * Allow both "delimiter" and "delimiters"
!      */
!     if (token &&
!         (strcasecmp(token, "delimiters") == 0 ||
!          strcasecmp(token, "delimiter") == 0))
!     {
          token = strtokx(NULL, whitespace, NULL, "'",
                          '\\', false, pset.encoding);
+         /* Discard "as" as syntactically neutral */
+         if (strcasecmp(token, "as") == 0)
+         {
+             token = strtokx(NULL, whitespace, NULL, "'",
+                             '\\', false, pset.encoding);
+         }
          if (!token)
              goto error;
          result->delim = xstrdup(token);
      }
+     else
+     {
+         if (havewith)
+             goto error;
+     }

      free(line);

*** psql-ref.sgml.old    Sun Jan 25 18:45:11 2004
--- psql-ref.sgml    Sun Jan 25 18:47:11 2004
***************
*** 708,715 ****
      { <replaceable class="parameter">filename</replaceable> | stdin | stdout | - }
          [ <literal>with</literal> ]
              [ <literal>oids</literal> ]
!             [ <literal>delimiter [as] </literal> '<replaceable class="parameter">character</replaceable>' ]
!             [ <literal>null [as] </literal> '<replaceable class="parameter">string</replaceable>' ]</literal>
          </term>

          <listitem>
--- 708,715 ----
      { <replaceable class="parameter">filename</replaceable> | stdin | stdout | - }
          [ <literal>with</literal> ]
              [ <literal>oids</literal> ]
!             [ [ <literal>with</literal> | <literal>using</literal> ]
!             <literal>delimiter[s] [as] </literal> '<replaceable class="parameter">character</replaceable>' ]
 [ <literal>null [as] </literal> '<replaceable class="parameter">string</replaceable>' ]</literal> 
          </term>

          <listitem>