Re: COPY TO (FREEZE)?
On Mon, Nov 13, 2023 at 01:17:32PM -0500, Tom Lane wrote: > Bruce Momjian writes: > > Patch applied to master. > > The buildfarm is quite unhappy with you. Wow, I never suspeced that, fixed. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: COPY TO (FREEZE)?
Bruce Momjian writes: > Patch applied to master. The buildfarm is quite unhappy with you. regards, tom lane
Re: COPY TO (FREEZE)?
On Mon, Oct 30, 2023 at 03:55:21PM -0400, Bruce Momjian wrote: > On Mon, Oct 30, 2023 at 02:29:05PM -0400, Tom Lane wrote: > > Bruce Momjian writes: > > > That is a good point. I reviewed more of the messages and added > > > capitalization where appropriate, patch attached. > > > > This is starting to look pretty good. I have one more thought, > > as long as we're touching all these messages anyway: how about > > s/FOO available only in CSV mode/FOO requires CSV mode/ ? > > That's both shorter and less telegraphic, as it's not omitting the verb. > > Sure, updated patch attached. Patch applied to master. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: COPY TO (FREEZE)?
HI, Zhang Mingli www.hashdata.xyz On Oct 31, 2023 at 03:55 +0800, Bruce Momjian , wrote: > > Sure, updated patch attached. LGTM.
Re: COPY TO (FREEZE)?
On Mon, Oct 30, 2023 at 02:29:05PM -0400, Tom Lane wrote:
> Bruce Momjian writes:
> > That is a good point. I reviewed more of the messages and added
> > capitalization where appropriate, patch attached.
>
> This is starting to look pretty good. I have one more thought,
> as long as we're touching all these messages anyway: how about
> s/FOO available only in CSV mode/FOO requires CSV mode/ ?
> That's both shorter and less telegraphic, as it's not omitting the verb.
Sure, updated patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..18ecc69c33 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is only allowed in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..cfad47b562 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -671,7 +671,7 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && opts_out->quote != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY quote available only in CSV mode")));
+ errmsg("COPY QUOTE requires CSV mode")));
if (opts_out->csv_mode && strlen(opts_out->quote) != 1)
ereport(ERROR,
@@ -687,7 +687,7 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && opts_out->escape != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY escape available only in CSV mode")));
+ errmsg("COPY ESCAPE requires CSV mode")));
if (opts_out->csv_mode && strlen(opts_out->escape) != 1)
ereport(ERROR,
@@ -698,46 +698,52 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && (opts_out->force_quote || opts_out->force_quote_all))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force quote available only in CSV mode")));
+ errmsg("COPY FORCE_QUOTE requires CSV mode")));
if ((opts_out->force_quote || opts_out->force_quote_all) && is_from)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force quote only available using COPY TO")));
+ errmsg("COPY FORCE_QUOTE cannot be used with COPY FROM")));
/* Check force_notnull */
if (!opts_out->csv_mode && opts_out->force_notnull != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null available only in CSV mode")));
+ errmsg("COPY FORCE_NOT_NULL requires CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FORCE_NOT_NULL cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null available only in CSV mode")));
+ errmsg("COPY FORCE_NULL requires CSV mode")));
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FORCE_NULL cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY delimiter must not appear in the NULL specification")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY delimiter character must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE cannot be used with COPY TO")));
+
if (opts_out->default_print)
{
if (!is_from)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 95ec7363af..c4178b9c07 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -83,17 +83,17 @@ ERROR: cannot specify DELIMITER in BINARY mode
COPY x to stdin (format BINARY, null
Re: COPY TO (FREEZE)?
Bruce Momjian writes: > That is a good point. I reviewed more of the messages and added > capitalization where appropriate, patch attached. This is starting to look pretty good. I have one more thought, as long as we're touching all these messages anyway: how about s/FOO available only in CSV mode/FOO requires CSV mode/ ? That's both shorter and less telegraphic, as it's not omitting the verb. regards, tom lane
Re: COPY TO (FREEZE)?
On Mon, Oct 30, 2023 at 03:16:58PM +0900, Kyotaro Horiguchi wrote:
> At Sun, 29 Oct 2023 15:35:02 -0400, Bruce Momjian wrote in
> > You are 100% correct. Updated patch attached.
>
> - errmsg("COPY force not null only available
> using COPY FROM")));
> + errmsg("COPY force not null cannot be used
> with COPY TO")));
>
> I find the term "force not null" hard to translate, especially into
> Japaese, as its literal translation doesn't align with the entire
> message. The most recent translation for it is the literal rendition
> of "FORCE_NOT_NULL option of COPY can only be used with COPY FROM".
>
> In short, for translation convenience, I would prefer if "force not
> null" were "FORCE_NOT_NULL".
That is a good point. I reviewed more of the messages and added
capitalization where appropriate, patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..18ecc69c33 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is only allowed in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..145315debe 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -671,7 +671,7 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && opts_out->quote != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY quote available only in CSV mode")));
+ errmsg("COPY QUOTE available only in CSV mode")));
if (opts_out->csv_mode && strlen(opts_out->quote) != 1)
ereport(ERROR,
@@ -687,7 +687,7 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && opts_out->escape != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY escape available only in CSV mode")));
+ errmsg("COPY ESCAPE available only in CSV mode")));
if (opts_out->csv_mode && strlen(opts_out->escape) != 1)
ereport(ERROR,
@@ -698,46 +698,52 @@ ProcessCopyOptions(ParseState *pstate,
if (!opts_out->csv_mode && (opts_out->force_quote || opts_out->force_quote_all))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force quote available only in CSV mode")));
+ errmsg("COPY FORCE_QUOTE available only in CSV mode")));
if ((opts_out->force_quote || opts_out->force_quote_all) && is_from)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force quote only available using COPY TO")));
+ errmsg("COPY FORCE_QUOTE cannot be used with COPY FROM")));
/* Check force_notnull */
if (!opts_out->csv_mode && opts_out->force_notnull != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null available only in CSV mode")));
+ errmsg("COPY FORCE_NOT_NULL available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FORCE_NOT_NULL cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null available only in CSV mode")));
+ errmsg("COPY FORCE_NULL available only in CSV mode")));
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FORCE_NULL cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY delimiter must not appear in the NULL specification")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY delimiter character must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+
Re: COPY TO (FREEZE)?
At Sun, 29 Oct 2023 15:35:02 -0400, Bruce Momjian wrote in
> You are 100% correct. Updated patch attached.
-errmsg("COPY force not null only available
using COPY FROM")));
+errmsg("COPY force not null cannot be used
with COPY TO")));
I find the term "force not null" hard to translate, especially into
Japaese, as its literal translation doesn't align with the entire
message. The most recent translation for it is the literal rendition
of "FORCE_NOT_NULL option of COPY can only be used with COPY FROM".
In short, for translation convenience, I would prefer if "force not
null" were "FORCE_NOT_NULL".
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: COPY TO (FREEZE)?
HI,
Zhang Mingli
www.hashdata.xyz
On Oct 30, 2023 at 10:58 +0800, Bruce Momjian , wrote:
> On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
> >
> > Bruce Momjian 于2023年10月30日周一03:35写道：
> >
> > On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > > I guess you want to write “cannot be used with COPY TO”
> >
> > You are 100% correct. Updated patch attached.
> >
> > --
> > Bruce Momjian https://momjian.us
> > EDB https://enterprisedb.com
> >
> > Only you can decide what is important to you.
> >
> >
> >
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >
> > + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
> >
> > +
> >
> >
> > COPY FROM-> COPY TO
>
> Agreed, patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
LGTM.
Re: COPY TO (FREEZE)?
On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
>
> Bruce Momjian 于2023年10月30日周一03:35写道：
>
> On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > I guess you want to write “cannot be used with COPY TO”
>
> You are 100% correct. Updated patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
>
>
>
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
>
> +
>
>
> COPY FROM-> COPY TO
Agreed, patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..8da4e6c226 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -711,8 +711,8 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY force not null available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force not null cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
@@ -722,22 +722,28 @@ ProcessCopyOptions(ParseState *pstate,
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force null cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode cannot be used with COPY TO")));
+
if (opts_out->default_print)
{
if (!is_from)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 95ec7363af..98ef2ef140 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -89,11 +89,11 @@ ERROR: COPY force quote only available using COPY TO
COPY x to stdout (format TEXT, force_not_null(a));
ERROR: COPY force not null available only in CSV mode
COPY x to stdin (format CSV, force_not_null(a));
-ERROR: COPY force not null only available using COPY FROM
+ERROR: COPY force not null cannot be used with COPY TO
COPY x to stdout (format TEXT, force_null(a));
ERROR: COPY force null available only in CSV mode
COPY x to stdin (format CSV, force_null(a));
-ERROR: COPY force null only available using COPY FROM
+ERROR: COPY force null cannot be used with COPY TO
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
Re: COPY TO (FREEZE)?
Bruce Momjian 于2023年10月30日 周一03:35写道：
> On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > I guess you want to write “cannot be used with COPY TO”
>
> You are 100% correct. Updated patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
>
> +
>
>
COPY FROM-> COPY TO
>
Re: COPY TO (FREEZE)?
On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> I guess you want to write “cannot be used with COPY TO”
You are 100% correct. Updated patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..bb6d93627b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -711,8 +711,8 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY force not null available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force not null cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
@@ -722,22 +722,28 @@ ProcessCopyOptions(ParseState *pstate,
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force null cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
+
if (opts_out->default_print)
{
if (!is_from)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 95ec7363af..98ef2ef140 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -89,11 +89,11 @@ ERROR: COPY force quote only available using COPY TO
COPY x to stdout (format TEXT, force_not_null(a));
ERROR: COPY force not null available only in CSV mode
COPY x to stdin (format CSV, force_not_null(a));
-ERROR: COPY force not null only available using COPY FROM
+ERROR: COPY force not null cannot be used with COPY TO
COPY x to stdout (format TEXT, force_null(a));
ERROR: COPY force null available only in CSV mode
COPY x to stdin (format CSV, force_null(a));
-ERROR: COPY force null only available using COPY FROM
+ERROR: COPY force null cannot be used with COPY TO
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
Re: COPY TO (FREEZE)?
Mingli Zhang 于2023年10月29日 周日14:35写道：
>
>
> Bruce Momjian 于2023年10月29日 周日10:04写道：
>
>> On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
>> > Bruce Momjian writes:
>> > > On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
>> > >> Not thrilled by the wording here.
>> >
>> > > I think it is modeled after:
>> >
>> > > errmsg("COPY force null only available using COPY FROM")));
>> >
>> > Well, now that you bring it up, that's no sterling example of
>> > clear writing either. Maybe change that while we're at it,
>> > say to "FORCE NULL option must not be used in COPY TO"?
>>
>> I used:
>>
>> "COPY FREEZE mode cannot be used with COPY FROM"
>>
>> and adjusted the others.
>>
>> > (Also, has it got the right ERRCODE?)
>>
>> Fixed, and the other cases too. Patch attached.
>>
>> --
>> Bruce Momjian https://momjian.us
>> EDB https://enterprisedb.com
>>
>> Only you can decide what is important to you.
>
>
> errmsg("COPY force not null only available using COPY FROM")));
>>
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>>
>> + errmsg("COPY force not null cannot be used with COPY FROM")));
>>
>>
> cannot -> can ?
>
I guess you want to write “cannot be used with COPY TO”
>
Re: COPY TO (FREEZE)?
Bruce Momjian 于2023年10月29日 周日10:04写道：
> On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
> > Bruce Momjian writes:
> > > On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
> > >> Not thrilled by the wording here.
> >
> > > I think it is modeled after:
> >
> > > errmsg("COPY force null only available using COPY FROM")));
> >
> > Well, now that you bring it up, that's no sterling example of
> > clear writing either. Maybe change that while we're at it,
> > say to "FORCE NULL option must not be used in COPY TO"?
>
> I used:
>
> "COPY FREEZE mode cannot be used with COPY FROM"
>
> and adjusted the others.
>
> > (Also, has it got the right ERRCODE?)
>
> Fixed, and the other cases too. Patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
errmsg("COPY force not null only available using COPY FROM")));
>
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY force not null cannot be used with COPY FROM")));
>
>
cannot -> can ?
>
Re: COPY TO (FREEZE)?
On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
> Bruce Momjian writes:
> > On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
> >> Not thrilled by the wording here.
>
> > I think it is modeled after:
>
> > errmsg("COPY force null only available using COPY FROM")));
>
> Well, now that you bring it up, that's no sterling example of
> clear writing either. Maybe change that while we're at it,
> say to "FORCE NULL option must not be used in COPY TO"?
I used:
"COPY FREEZE mode cannot be used with COPY FROM"
and adjusted the others.
> (Also, has it got the right ERRCODE?)
Fixed, and the other cases too. Patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..bda3f8b9f9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -711,8 +711,8 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY force not null available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force not null cannot be used with COPY FROM")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
@@ -722,22 +722,28 @@ ProcessCopyOptions(ParseState *pstate,
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force null cannot be used with COPY FROM")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
+
if (opts_out->default_print)
{
if (!is_from)
Re: COPY TO (FREEZE)?
Bruce Momjian writes:
> On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
>> Not thrilled by the wording here.
> I think it is modeled after:
> errmsg("COPY force null only available using COPY FROM")));
Well, now that you bring it up, that's no sterling example of
clear writing either. Maybe change that while we're at it,
say to "FORCE NULL option must not be used in COPY TO"?
(Also, has it got the right ERRCODE?)
regards, tom lane
Re: COPY TO (FREEZE)?
On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
> Bruce Momjian writes:
> > My apologies, wrong patch attached, right one attached now.
>
> I think this one is fine as-is:
>
> /* Only single-byte delimiter strings are supported. */
> if (strlen(opts_out->delim) != 1)
> ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>errmsg("COPY delimiter must be a single
> one-byte character")));
>
> While we have good implementation reasons for this restriction,
> there's nothing illogical about wanting the delimiter to be more
> general. It's particularly silly, from an end-user's standpoint,
> that for example 'é' is an allowed delimiter in LATIN1 encoding
> but not when the server is using UTF8. So I don't see how the
> distinction you presented justifies this change.
Agreed, my mistake.
> + if (opts_out->freeze && !is_from)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("COPY freeze only available using COPY
> FROM")));
>
> Not thrilled by the wording here. I don't like the fact that the
> keyword FREEZE isn't capitalized, and I think you omitted too many
> words for intelligibility to be preserved. Notably, all the adjacent
> examples use "must" or "must not", and this decides that that can be
> omitted.
I think it is modeled after:
errmsg("COPY force null only available using COPY FROM")));
> I realize that you probably modeled the non-capitalization on nearby
> messages like "COPY delimiter", but there's a difference IMO:
> "delimiter" can be read as an English noun, but it's hard to read
> "freeze" as a noun.
>
> How about, say,
>
> errmsg("COPY FREEZE must not be used in COPY TO")));
>
> or perhaps that's redundant and we could write
>
> errmsg("FREEZE option must not be used in COPY TO")));
I now have:
errmsg("COPY FREEZE mode only available using COPY FROM")));
Updated patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..92558b6bb0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -728,16 +728,22 @@ ProcessCopyOptions(ParseState *pstate,
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode only available using COPY FROM")));
+
if (opts_out->default_print)
{
if (!is_from)
Re: COPY TO (FREEZE)?
Bruce Momjian writes:
> My apologies, wrong patch attached, right one attached now.
I think this one is fine as-is:
/* Only single-byte delimiter strings are supported. */
if (strlen(opts_out->delim) != 1)
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must be a single
one-byte character")));
While we have good implementation reasons for this restriction,
there's nothing illogical about wanting the delimiter to be more
general. It's particularly silly, from an end-user's standpoint,
that for example 'é' is an allowed delimiter in LATIN1 encoding
but not when the server is using UTF8. So I don't see how the
distinction you presented justifies this change.
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("COPY freeze only available using COPY
FROM")));
Not thrilled by the wording here. I don't like the fact that the
keyword FREEZE isn't capitalized, and I think you omitted too many
words for intelligibility to be preserved. Notably, all the adjacent
examples use "must" or "must not", and this decides that that can be
omitted.
I realize that you probably modeled the non-capitalization on nearby
messages like "COPY delimiter", but there's a difference IMO:
"delimiter" can be read as an English noun, but it's hard to read
"freeze" as a noun.
How about, say,
errmsg("COPY FREEZE must not be used in COPY TO")));
or perhaps that's redundant and we could write
errmsg("FREEZE option must not be used in COPY TO")));
regards, tom lane
Re: COPY TO (FREEZE)?
On Sat, Oct 28, 2023 at 08:38:26PM -0400, Bruce Momjian wrote:
> I would like to apply the attached patch to master. Looking at your
> adjustments for ERRCODE_FEATURE_NOT_SUPPORTED to
> ERRCODE_INVALID_PARAMETER_VALUE, I only changed the cases where it would
> be illogical to implement the feature, not just that we have no
> intention of implementing the feature. I read "invalid" as "illogical".
My apologies, wrong patch attached, right one attached now.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..4c23c133e1 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -617,7 +617,7 @@ ProcessCopyOptions(ParseState *pstate,
/* Only single-byte delimiter strings are supported. */
if (strlen(opts_out->delim) != 1)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must be a single one-byte character")));
/* Disallow end-of-line characters */
@@ -728,16 +728,22 @@ ProcessCopyOptions(ParseState *pstate,
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY freeze only available using COPY FROM")));
+
if (opts_out->default_print)
{
if (!is_from)
Re: COPY TO (FREEZE)?
On Tue, Aug 2, 2022 at 05:17:35PM +0900, Kyotaro Horiguchi wrote: > At Tue, 2 Aug 2022 14:17:46 +0800, Julien Rouhaud wrote > in > > Hi, > > > > On Tue, Aug 02, 2022 at 01:30:46PM +0900, Kyotaro Horiguchi wrote: > > > I noticed that COPY TO accepts FREEZE option but it is pointless. > > > > > > Don't we reject that option as the first-attached does? > > > > I agree that we should reject it, +1 for the patch. > > Thanks for looking it! > > > > By the way, most of the invalid option combinations for COPY are > > > marked as ERRCODE_FEATURE_NOT_SUPPORTED. I looks to me saying that > > > "that feature is theoretically possible or actually realized > > > elsewhere, but impossible now or here". > > > > > > If it is correct, aren't they better be ERRCODE_INVALID_PARAMETER_VALUE? > > > The > > > code is being used for similar messages "unrecognized parameter " > > > and > > > "parameter specified more than once" (or some others?). At least a > > > quote string longer than a single character seems like to fit > > > INVALID_PARAMETER_VALUE. (I believe we don't mean to support > > > multicharacter > > > (or even multibyte) escape/quote character anddelimiter). That being > > > said, > > > I'm not sure if the change will be worth the trouble. > > > > I also feel weird about it. I raised the same point recently about COPY > > FROM + > > HEADER MATCH (1), and at that time there wasn't a real consensus on the way > > to > > go, just keep the things consistent. I'm +0.5 on that patch for the same > > reason as back then. My only concern is that it can in theory break things > > if > > you rely on the current sqlstate, but given the errors I don't think it's > > really a problem. > > Exactly. That is the exact reason for my to say "I'm not sure if..". > > > [1]: > > https://www.postgresql.org/message-id/flat/20220614091319.jk4he5migtpwyd7r%40jrouhaud#b18bf3705fb9f69d0112b6febf0fa1be > > > Maybe that's just me but I understand "not supported" as "this makes > > sense, but this is currently a limitation that might be lifted > > later". > > FWIW I understand it the same way. I would like to apply the attached patch to master. Looking at your adjustments for ERRCODE_FEATURE_NOT_SUPPORTED to ERRCODE_INVALID_PARAMETER_VALUE, I only changed the cases where it would be illogical to implement the feature, not just that we have no intention of implementing the feature. I read "invalid" as "illogical". -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index d94e3cacfc..cc7d797159 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1119,6 +1119,10 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g destination, because all data must pass through the client/server connection. For large amounts of data the SQL command might be preferable. +Also, because of this pass-through method, \copy +... from in CSV mode will erroneously +treat a \. data value alone on a line as an +end-of-input marker. diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c index b3cc3d9a29..8d6ce4cedd 100644 --- a/src/bin/psql/copy.c +++ b/src/bin/psql/copy.c @@ -627,6 +627,7 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res) * This code erroneously assumes '\.' on a line alone * inside a quoted CSV string terminates the \copy. * https://www.postgresql.org/message-id/e1tdnvq-0001ju...@wrigleys.postgresql.org + * https://www.postgresql.org/message-id/bfcd57e4-8f23-4c3e-a5db-2571d0920...@beta.fastmail.com */ if ((linelen == 3 && memcmp(fgresult, "\\.\n", 3) == 0) || (linelen == 4 && memcmp(fgresult, "\\.\r\n", 4) == 0))
Re: COPY TO (FREEZE)?
Regards, Zhang Mingli On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi , wrote: > I noticed that COPY TO accepts FREEZE option but it is pointless. > > Don't we reject that option as the first-attached does? I +1, should be rejected like other invalid options. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center
Re: COPY TO (FREEZE)?
At Tue, 2 Aug 2022 14:17:46 +0800, Julien Rouhaud wrote in > Hi, > > On Tue, Aug 02, 2022 at 01:30:46PM +0900, Kyotaro Horiguchi wrote: > > I noticed that COPY TO accepts FREEZE option but it is pointless. > > > > Don't we reject that option as the first-attached does? > > I agree that we should reject it, +1 for the patch. Thanks for looking it! > > By the way, most of the invalid option combinations for COPY are > > marked as ERRCODE_FEATURE_NOT_SUPPORTED. I looks to me saying that > > "that feature is theoretically possible or actually realized > > elsewhere, but impossible now or here". > > > > If it is correct, aren't they better be ERRCODE_INVALID_PARAMETER_VALUE? > > The > > code is being used for similar messages "unrecognized parameter " and > > "parameter specified more than once" (or some others?). At least a > > quote string longer than a single character seems like to fit > > INVALID_PARAMETER_VALUE. (I believe we don't mean to support multicharacter > > (or even multibyte) escape/quote character anddelimiter). That being said, > > I'm not sure if the change will be worth the trouble. > > I also feel weird about it. I raised the same point recently about COPY FROM > + > HEADER MATCH (1), and at that time there wasn't a real consensus on the way to > go, just keep the things consistent. I'm +0.5 on that patch for the same > reason as back then. My only concern is that it can in theory break things if > you rely on the current sqlstate, but given the errors I don't think it's > really a problem. Exactly. That is the exact reason for my to say "I'm not sure if..". > [1]: > https://www.postgresql.org/message-id/flat/20220614091319.jk4he5migtpwyd7r%40jrouhaud#b18bf3705fb9f69d0112b6febf0fa1be > Maybe that's just me but I understand "not supported" as "this makes > sense, but this is currently a limitation that might be lifted > later". FWIW I understand it the same way. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: COPY TO (FREEZE)?
Hi, On Tue, Aug 02, 2022 at 01:30:46PM +0900, Kyotaro Horiguchi wrote: > I noticed that COPY TO accepts FREEZE option but it is pointless. > > Don't we reject that option as the first-attached does? I agree that we should reject it, +1 for the patch. > By the way, most of the invalid option combinations for COPY are > marked as ERRCODE_FEATURE_NOT_SUPPORTED. I looks to me saying that > "that feature is theoretically possible or actually realized > elsewhere, but impossible now or here". > > If it is correct, aren't they better be ERRCODE_INVALID_PARAMETER_VALUE? The > code is being used for similar messages "unrecognized parameter " and > "parameter specified more than once" (or some others?). At least a > quote string longer than a single character seems like to fit > INVALID_PARAMETER_VALUE. (I believe we don't mean to support multicharacter > (or even multibyte) escape/quote character anddelimiter). That being said, > I'm not sure if the change will be worth the trouble. I also feel weird about it. I raised the same point recently about COPY FROM + HEADER MATCH (1), and at that time there wasn't a real consensus on the way to go, just keep the things consistent. I'm +0.5 on that patch for the same reason as back then. My only concern is that it can in theory break things if you rely on the current sqlstate, but given the errors I don't think it's really a problem. [1]: https://www.postgresql.org/message-id/flat/20220614091319.jk4he5migtpwyd7r%40jrouhaud#b18bf3705fb9f69d0112b6febf0fa1be
