Content-Type: multipart/alternative;
 boundary="------------YZlLla20Rh2kOWf30Dv827VJ"
Message-ID: <513a82bf-2710-b4be-73e8-7251616a3b01@dunslane.net>
Date: Sat, 20 May 2023 10:01:44 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Content-Language: en-US
To: Joel Jacobson <joel@compiler.org>, pgsql-hackers@lists.postgresql.org
References: <748b7bfc-9f16-4bea-b7e1-4a59dc9354f5@app.fastmail.com>
 <fd88c4f8-a6f7-a111-c4e9-80b6a71ff451@dunslane.net>
 <0400a90b-ff40-4be5-8f90-6c205c9cd99b@app.fastmail.com>
From: Andrew Dunstan <andrew@dunslane.net>
Subject: Re: New COPY options: DELIMITER NONE and QUOTE NONE
In-Reply-To: <0400a90b-ff40-4be5-8f90-6c205c9cd99b@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/513a82bf-2710-b4be-73e8-7251616a3b01%40dunslane.net>
Precedence: bulk

This is a multi-part message in MIME format.
--------------YZlLla20Rh2kOWf30Dv827VJ
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2023-05-20 Sa 02:59, Joel Jacobson wrote:
> On Fri, May 19, 2023, at 19:03, Andrew Dunstan wrote:
> > I think you've been a bit too cute with the grammar changes, but as 
> you say this is a POC.
>
> Thanks for feedback.
>
> The approach I took for the new grammar rules was inspired by previous 
> commits,
> such as de7531a971b, which introduced support for 'FORCE QUOTE '*''. 
> In that
> case, a new separate grammar rule was crafted.
>
> Not sure what you mean with it being "too cute", but maybe you think 
> it's a bit
> verbose with another grammar rule and it would be better to integrate 
> it into
> the existing one?
>
> Example:
>
> | DELIMITER opt_as (Sconst | NONE)
>         {
>                 if ($3 == NONE)
>                         $$ = makeDefElem("delimiter", (Node *) 
> makeString("\0"), @1);
>                 else
>                         $$ = makeDefElem("delimiter", (Node *) 
> makeString($3), @1);
>         }
>
>


I would probably go for something like this for "DELIMITER NONE" in a 
separate rule:

  | DELIMITER NONE
     {

        $$ = makeDefElem("delimiter_none", (Node *)makeInteger(true), @1);

     }

and deal with that element further down the stack. It looks to me at 
first glance that your changes would allow "DELIMITER ''" which is 
probably not what we want.

Similarly for "QUOTE NONE".


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

--------------YZlLla20Rh2kOWf30Dv827VJ
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 2023-05-20 Sa 02:59, Joel Jacobson
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:0400a90b-ff40-4be5-8f90-6c205c9cd99b@app.fastmail.com">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <title></title>
      <style type="text/css">p.MsoNormal,p.MsoNoSpacing{margin:0}</style>
      <div>On Fri, May 19, 2023, at 19:03, Andrew Dunstan wrote:<br>
      </div>
      <div>&gt; I think you've been a bit too cute with the grammar
        changes, but as you say this is a POC.<br>
      </div>
      <div><br>
      </div>
      <div>Thanks for feedback.<br>
      </div>
      <div><br>
      </div>
      <div>The approach I took for the new grammar rules was inspired by
        previous commits,<br>
      </div>
      <div>such as de7531a971b, which introduced support for 'FORCE
        QUOTE '*''. In that<br>
      </div>
      <div>case, a new separate grammar rule was crafted.<br>
      </div>
      <div><br>
      </div>
      <div>Not sure what you mean with it being "too cute", but maybe
        you think it's a bit<br>
      </div>
      <div>verbose with another grammar rule and it would be better to
        integrate it into<br>
      </div>
      <div>the existing one?<br>
      </div>
      <div><br>
      </div>
      <div>Example:<br>
      </div>
      <div><br>
      </div>
      <div>| DELIMITER opt_as (Sconst | NONE)<br>
      </div>
      <div>        {<br>
      </div>
      <div>                if ($3 == NONE)<br>
      </div>
      <div>                        $$ = makeDefElem("delimiter", (Node
        *) makeString("\0"), @1);<br>
      </div>
      <div>                else<br>
      </div>
      <div>                        $$ = makeDefElem("delimiter", (Node
        *) makeString($3), @1);<br>
      </div>
      <div>        }<br>
      </div>
      <div><br>
      </div>
      <div><br>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p><br>
    </p>
    <p>I would probably go for something like this for "DELIMITER NONE"
      in a separate rule:</p>
    <p> | DELIMITER NONE<br>
          {<br>
    </p>
    <p>       $$ = makeDefElem("delimiter_none", (Node
      *)makeInteger(true), @1);</p>
    <p>    }<br>
    </p>
    <p>and deal with that element further down the stack. It looks to me
      at first glance that your changes would allow "DELIMITER ''" which
      is probably not what we want.</p>
    <p>Similarly for "QUOTE NONE".<br>
    </p>
    <p><br>
    </p>
    <p>cheers</p>
    <p><br>
    </p>
    <p>andrew<br>
    </p>
    <pre class="moz-signature" signature-switch-id="d0437855-2267-4610-80b3-83167ec45b0b" cols="72">--
Andrew Dunstan
EDB: <a class="moz-txt-link-freetext" href="https://www.enterprisedb.com">https://www.enterprisedb.com</a>
</pre>
  </body>
</html>

--------------YZlLla20Rh2kOWf30Dv827VJ--