Re: file_fdw: Support multi-line HEADER option.

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/

In response to

Responses

Browse pgsql-hackers by date

  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