Message-ID: <8eba1968-9503-5be7-b473-37383f17bb72@enterprisedb.com>
Date: Thu, 3 Feb 2022 07:37:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.5.1
Subject: Re: Add header support to text format and matching feature
Content-Language: en-US
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: =?UTF-8?Q?R=c3=a9mi_Lapeyre?= <remi.lapeyre@lenstra.fr>
Cc: Zhihong Yu <zyu@yugabyte.com>, David Steele <david@pgmasters.net>,
 Michael Paquier <michael@paquier.xyz>, vignesh C <vignesh21@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Daniel Verite <daniel@manitou-mail.org>
References: <018819f7-6cbe-4f20-903f-de1b9e434119@manitou-mail.org>
 <9B3E4BEB-F6E2-4D4D-AFB8-58FB3957EB35@lenstra.fr>
 <29be62a4-e931-b745-3e24-7aa111963447@pgmasters.net>
 <8A231E8D-4491-4DC2-AF72-EBEAEF0F0967@lenstra.fr>
 <CALNJ-vRUVcnSO_WDJS5RRjsh26jRK2rVsPoos+xeJ4Uu1fOtWw@mail.gmail.com>
 <683DD77E-38B3-4BC2-810E-4122C7A5A053@lenstra.fr>
 <CALNJ-vRCei7pSp6-Atf3Zy3Y88UDzKEVTCxPxA4AVvUebxSfNg@mail.gmail.com>
 <8964EEB0-7AE1-4549-BA4C-119EF8CD266C@lenstra.fr>
 <94FDDB44-A608-4C75-A733-9F8FC5180D6B@lenstra.fr>
 <4816b2df-646f-dc19-e5cc-4bfbd04c0e43@enterprisedb.com>
 <07891B9D-2140-4C06-AF96-C14F1E1B0E8E@lenstra.fr>
 <f62f0262-2a3c-8839-5ced-25ca2421d738@enterprisedb.com>
In-Reply-To: <f62f0262-2a3c-8839-5ced-25ca2421d738@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/8eba1968-9503-5be7-b473-37383f17bb72%40enterprisedb.com>
Precedence: bulk

On 31.01.22 07:54, Peter Eisentraut wrote:
> On 30.01.22 23:56, Rémi Lapeyre wrote:
>>> I notice in the 0002 patch that there is no test case for the error 
>>> "wrong header for column \"%s\": got \"%s\"", which I think is really 
>>> the core functionality of this patch.  So please add that.
>>>
>>
>> I added a test for it in this new version of the patch.
> 
> The file_fdw.sql tests contain this
> 
> +CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER 
> file_server
> +OPTIONS (format 'csv', filename :'filename', delimiter ',', header 
> 'match');   -- ERROR
> 
> but no actual error is generated.  Please review the additions on the 
> file_fdw tests to see that they make sense.

A few more comments on your latest patch:

- The DefGetCopyHeader() function seems very bulky and might not be 
necessary.  I think you can just check for the string "match" first and 
then use defGetBoolean() as before if it didn't match.

- If you define COPY_HEADER_ABSENT = 0 in the enum, then most of the 
existing truth checks don't need to be changed.

- In NextCopyFromRawFields(), it looks like you have code that replaces 
the null_print value if the supplied column name is null.  I don't think 
we should allow null column values.  Someone, this should be an error. 
(Do we use null_print on output and make the column name null if it 
matches?)