Reject HEADER with binary and json COPY formats by option presence

Поиск
Список
Период
Сортировка
Искать
От
Chao Li
Тема
Reject HEADER with binary and json COPY formats by option presence
Дата
в 04:57:06
Msg-id
CD33AC99-0175-4AD9-A33F-88E57736DA8E@gmail.com
Список
Дерево обсуждения
Reject HEADER with binary and json COPY formats by option presence Chao Li <li.evan.chao@gmail.com>
Re:Reject HEADER with binary and json COPY formats by option presence "Tingchuan Sun" <suntingchuan1996@163.com>
Re: Reject HEADER with binary and json COPY formats by option presence Shinya Kato <shinya11.kato@gmail.com>
Re: Reject HEADER with binary and json COPY formats by option presence Fujii Masao <masao.fujii@gmail.com>
Re: Reject HEADER with binary and json COPY formats by option presence Chao Li <li.evan.chao@gmail.com>
Hi,

While testing “file_fdw: Support multi-line HEADER option”, I noticed a small issue.

The doc says that the HEADER option cannot be used with the “binary" or “json" format:
```
   
    HEADER
    
     
      On output, if this option is set to true
      (or an equivalent Boolean value), the first line of the output will
      contain the column names from the table.
      Integer values 0 and 1 are
      accepted as Boolean values, but other integers are not allowed for
      COPY TO commands.
     
     
      On input, if this option is set to true
      (or an equivalent Boolean value), the first line of the input is
      discarded.  If set to a non-negative integer, that number of
      lines are discarded.  If set to MATCH, the first line
      is discarded, and it must contain column names that exactly match the
      table's columns, in both number and order; otherwise, an error is raised.
      The MATCH value is only valid for
      COPY FROM commands.
     
     
      This option is not allowed when using binary or json format.
     
    
   
```

However, when I specified "header ‘0", the command did not fail. That means the current behavior depends on the value of the “header" option, not on presence:
```
evantest=# create foreign table ft (i int) server fs options (format 'binary', filename '/tmp/ft.bin',  header '1');
ERROR:  cannot specify HEADER in BINARY mode
evantest=# create foreign table ft (i int) server fs options (format 'binary', filename '/tmp/ft.bin',  header '0');
CREATE FOREIGN TABLE
```

As we can see, "header 1" fails, but header 0" is silently accepted. I don't think this behavior matches what the documentation describes.

For comparison, VACUUM has a similar option. “BUFFER_USAGE_LIMIT" is not allowed with "VACUUM FULL", and a value of 0 means disabling the buffer access strategy:
```
   
    BUFFER_USAGE_LIMIT
    
     
      Specifies the
      Buffer Access Strategy
      ring buffer size for VACUUM.  This size is used to
      calculate the number of shared buffers which will be reused as part of
      this strategy.  0 disables use of a
      Buffer Access Strategy.  If 
      is also specified, the  value is used
      for both the vacuum and analyze stages.  This option can't be used with
      the  option except if  is
      also specified.  When this option is not specified,
      VACUUM uses the value from
      .  Higher settings can
      allow VACUUM to run more quickly, but having too
      large a setting may cause too many other useful pages to be evicted from
      shared buffers.  The minimum value is 128 kB and the
      maximum value is 16 GB.
     
    
   
```

Using BUFFER_USAGE_LIMIT 0 with FULL is still rejected:
```
evantest=# vacuum (full, BUFFER_USAGE_LIMIT 0) t;
ERROR:  BUFFER_USAGE_LIMIT cannot be specified for VACUUM FULL
```

So VACUUM rejects BUFFER_USAGE_LIMIT based on the presence of the option, not its value. I think we should keep the behavior consistent here, and VACUUM's behavior better matches the documentation. Otherwise, I am afraid this could encourage more inconsistencies in the future.

The fix is straightforward. Since we already have the “header_specified" variable to indicate whether the option is present, we can check “header_specified" instead.

I reported a similar issue for the COPY command earlier in thread [1]. If this patch is accepted, then that one may be worth considering as well.

[1] https://www.postgresql.org/message-id/C1D2509E-E5D1-46B0-932C-B57AA7B963A1%40gmail.com

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/




В списке pgsql-hackers по дате отправления
От: Andres Freund
Дата:
От: Chao Li
Дата:
FAQ