Thread: COPY TO (FREEZE)?

I noticed that COPY TO accepts FREEZE option but it is pointless.

Don't we reject that option as the first-attached does?  I tempted to
add tests for those option combinations that are to be rejected but I
didin't come up with a clean way to do that.


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
"unrecognizedparameter <name>" and "parameter <name> specified more than once" (or some others?).  At least a quote
stringlonger 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
willbe worth the trouble.
 

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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 <name>" and
> "parameter <name> 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

At Tue, 2 Aug 2022 14:17:46 +0800, Julien Rouhaud <rjuju123@gmail.com> 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 <name>" and
> > "parameter <name> 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

On Tue, Aug  2, 2022 at 05:17:35PM +0900, Kyotaro Horiguchi wrote:
> At Tue, 2 Aug 2022 14:17:46 +0800, Julien Rouhaud <rjuju123@gmail.com> 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 <name>" and
> > > "parameter <name> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> 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

On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> 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

On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
> 
> Bruce Momjian <bruce@momjian.us>于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  <bruce@momjian.us>        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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

At Sun, 29 Oct 2023 15:35:02 -0400, Bruce Momjian <bruce@momjian.us> 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

On Mon, Oct 30, 2023 at 03:16:58PM +0900, Kyotaro Horiguchi wrote:
> At Sun, 29 Oct 2023 15:35:02 -0400, Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> 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

On Mon, Oct 30, 2023 at 02:29:05PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

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 <bruce@momjian.us> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> writes:
> Patch applied to master.

The buildfarm is quite unhappy with you.

            regards, tom lane

On Mon, Nov 13, 2023 at 01:17:32PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Patch applied to master.
> 
> The buildfarm is quite unhappy with you.

Wow, I never suspeced that, fixed.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.