| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Shinya Kato <shinya11(dot)kato(at)gmail(dot)com> |
| Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
| Subject: | Re: file_fdw: Support multi-line HEADER option. |
| Date: | 2026-01-20 05:53:45 |
| Message-ID: | DD1F416F-C2B6-468D-8D92-3AA2654A0808@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Jan 15, 2026, at 19:44, Shinya Kato <shinya11(dot)kato(at)gmail(dot)com> wrote:
>
> On Mon, Jan 12, 2026 at 1:01 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>> Thanks for the patch. Here are a few review comments:
>
> Thank you for the review!
>
>> 1
>> ```
>> - * Allow 0, 1, "true", "false", "on", "off", a non-negative integer, or
>> - * "match".
>> + * Allow 0, 1, "true", "false", "on", "off", a non-negative integer (also
>> + * as a string, to support file_fdw options), or "match".
>> */
>> ```
>>
>> From this comment, I cannot get how “0” and “1” will behave, and I cannot find a test case to show me that.
>>
>> With this patch, “2” acts the same as 2, so “1” acts the same as 1. Will “1” be a line count or a boolean true?
>
> The header option ends up as an integer line count in
> defGetCopyHeaderOption whether the value is quoted or not, so we don't
> need to distinguish between them. But as you said, it is ambiguous, so
> I updated the comment and added a test case.
>
I am sorry maybe I didn’t express myself clear. But in v4, this problem is clearer:
1 - 0001
```
/*
- * Allow 0, 1, "true", "false", "on", "off", a non-negative integer, or
- * "match".
+ * Allow 0, 1, "true", "false", "on", "off", an integer greater than or
+ * equal to zero, or "match".
*/
```
Here, “0, 1” is a duplicate of “an integer greater than or equal to zero”, so the commend can be simplified as:
```
Allow “true”, “false”, “on”, “off”, an integer greater than or equal to zero, or ...
```
And one more comment for 0002:
2 - 0002
Looking at the two error branches:
```
+ else
+ {
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+
+ /* Check if the header is a valid integer */
+ ival = pg_strtoint32_safe(sval, (Node *) &escontext);
+ if (escontext.error_occurred)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s requires a Boolean value, an integer greater than or "
+ "equal to zero, or the string \"match\"",
+ def->defname)));
+ }
```
and
```
+ if (ival < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("a negative integer value cannot be "
+ "specified for %s", def->defname)));
```
For the "ival<0" case, I think we can use the same error message as the first one, because the error message “an integer greater than or equal to zero” has covered the error of “ival<0”. It would be better to generate less different error messages.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Japin Li | 2026-01-20 06:01:21 | Re: [PATCH] Add pg_get_role_ddl() functions for role recreation |
| Previous Message | vignesh C | 2026-01-20 05:30:36 | Re: Logical Replication of sequences |