Re: [PATCH] Add error hints for invalid COPY options - Mailing list pgsql-hackers

From Sugamoto Shinya
Subject Re: [PATCH] Add error hints for invalid COPY options
Date
Msg-id CAAe3y+-gVJHx+R6=yNS9qC_-4754NOLg5y9haiJ=V=GDfHZCXw@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Add error hints for invalid COPY options  (Sugamoto Shinya <shinya34892@gmail.com>)
Responses Re: [PATCH] Add error hints for invalid COPY options
List pgsql-hackers


On Wed, Nov 26, 2025 at 5:55 PM Sugamoto Shinya <shinya34892@gmail.com> wrote:


2025年11月25日(火) 6:50 Nathan Bossart <nathandbossart@gmail.com>:
On Mon, Nov 24, 2025 at 11:56:34AM -0800, Masahiko Sawada wrote:
> On Sat, Nov 22, 2025 at 8:33 PM Sugamoto Shinya <shinya34892@gmail.com> wrote:
>> This follows the pattern already used elsewhere in PostgreSQL for providing
>> helpful error hints to users.
>
> Given we have 15 COPY options now, it sounds like a reasonable idea.
>
> One concern about the patch is that when adding a new COPY option, we
> could miss updating valid_copy_options list, resulting in providing a
> wrong suggestion. I think we can consider refactoring the COPY option
> handling so that we check the given option is a valid name or not by
> checking valid_copy_options array and then process the option value.

+1.  Ideally, folks wouldn't need to update a separate list when adding new
options.

>> Additionally, this patch corrects a misleading comment for the
>> convert_selectively option. The comment stated it was "not-accessible-from-SQL",
>> but actualy it has been accessible from SQL due to PostgreSQL's generic option parser.
>> The updated comment clarifies that while technically accessible, it's intended for
>> internal use and not recommended for end-user use due to potential data loss.
>
> Hmm, I'm not sure the proposed comment improves the clarification.
> It's essentially non-accessible from SQL since we cannot provide a
> valid value for convert_selectively from SQL commands.

Yeah, I'd leave it alone, at least for this patch.

--
nathan



Thanks for checking my proposal. 


For the refactoring of the COPY options, it sounds reasonable to me. Let me take that changes in my patch.


Regarding the option “convert_selectively”, at first i thought it should be blocked to be used like in the lexer and parser level but later I succeeded in specifying and using it in the SQL interface, psql , in my local environment, and also these accept the grammar for the option. You can verify that it’s actually accessible in any SQL interface using the following SQLs. Please let me know if there might be something I missed. Also I would be happy to make another thread about this matter


‘’’sql
CREATE TABLE conv_test (
a int,
b int,
c text
);

COPY conv_test FROM STDIN (
    FORMAT csv,
    convert_selectively (a, b) 
);

— for stdin 
1,2,foo
3,4,bar

SELECT * FROM conv_test;  

— result is 
a | b | c
---+---+------
1 | 2 | NULL
3 | 4 | NULL
(2 rows)

‘’’

Regards,


>> One concern about the patch is that when adding a new COPY option, we
>> could miss updating valid_copy_options list, resulting in providing a
>> wrong suggestion. I think we can consider refactoring the COPY option
>> handling so that we check the given option is a valid name or not by
>> checking valid_copy_options array and then process the option value.
>
>+1.  Ideally, folks wouldn't need to update a separate list when adding new
options. 

I updated my patch similar to src/backend/utils/adt/encode.c so that we can put both
all the COPY options and its validation functions into one place, because I believe this
should be the strongest way to prevent anyone from forgetting to add a new
option name to multiple places. However, I was wondering if any other simple solutions
, such as putting some simple validation right under each iteration loop, would be sufficient
or not, for example like the following pseudo code. It would be the minimum and simplest
changes, although it would leave some room for some people to skip adding proper code.
Please let me know freely if you have any opinions about this.

```
list valid_options = ["format", "force_null", ...]

foreach(opt, options)
{
   validate_option(opt, valid_options)   <--- THIS

   if (opt.name == "format") ...
   if (opt.name == "force_null") ...
   ...
}
```


Regarding the option `convert_selectively`, what do you think about this?
Still it seems to me that this option is accessible from SQL interfaces. But for now
I just removed the comment changes from my v2 patch and would like to discuss
this with you here or in another thread.

Regards,

Attachment

pgsql-hackers by date:

Previous
From: Dean Rasheed
Date:
Subject: Re: Second RewriteQuery complains about first RewriteQuery in edge case
Next
From: Mihail Nikalayeu
Date:
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements