public inbox for [email protected]help / color / mirror / Atom feed
Re: COPY TO (FREEZE)? 11+ messages / 5 participants [nested] [flat]
* Re: COPY TO (FREEZE)? @ 2023-10-29 01:39 Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2023-10-29 01:39 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers Bruce Momjian <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 01:47 Bruce Momjian <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Bruce Momjian @ 2023-10-29 01:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote: > Bruce Momjian <[email protected]> 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] freeze.diff (1.7K, ../../[email protected]/2-freeze.diff) download | inline diff: 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 { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable open and there are no older snapshots held by this transaction. It is currently not possible to perform a <command>COPY FREEZE</command> on a partitioned table. + This option is allowed only in <command>COPY FROM</command>. </para> <para> 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) ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 01:54 Tom Lane <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2023-10-29 01:54 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers Bruce Momjian <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 02:03 Bruce Momjian <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Bruce Momjian @ 2023-10-29 02:03 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote: > Bruce Momjian <[email protected]> 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] freeze.diff (2.5K, ../../[email protected]/2-freeze.diff) download | inline diff: 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 { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable open and there are no older snapshots held by this transaction. It is currently not possible to perform a <command>COPY FREEZE</command> on a partitioned table. + This option is allowed only in <command>COPY FROM</command>. </para> <para> 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) ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 06:35 Mingli Zhang <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Mingli Zhang @ 2023-10-29 06:35 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] Bruce Momjian <[email protected]>于2023年10月29日 周日10:04写道： > On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote: > > Bruce Momjian <[email protected]> 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 <[email protected]> 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 ? > ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 06:50 Mingli Zhang <[email protected]> parent: Mingli Zhang <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Mingli Zhang @ 2023-10-29 06:50 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] Mingli Zhang <[email protected]>于2023年10月29日 周日14:35写道： > > > Bruce Momjian <[email protected]>于2023年10月29日 周日10:04写道： > >> On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote: >> > Bruce Momjian <[email protected]> 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 <[email protected]> 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” > ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 19:35 Bruce Momjian <[email protected]> parent: Mingli Zhang <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Bruce Momjian @ 2023-10-29 19:35 UTC (permalink / raw) To: Mingli Zhang <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] freeze.diff (3.4K, ../../[email protected]/2-freeze.diff) download | inline diff: 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 { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable open and there are no older snapshots held by this transaction. It is currently not possible to perform a <command>COPY FREEZE</command> on a partitioned table. + This option is allowed only in <command>COPY FROM</command>. </para> <para> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-29 21:07 Mingli Zhang <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Mingli Zhang @ 2023-10-29 21:07 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] Bruce Momjian <[email protected]>于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 <[email protected]> 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 > ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-30 02:58 Bruce Momjian <[email protected]> parent: Mingli Zhang <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Bruce Momjian @ 2023-10-30 02:58 UTC (permalink / raw) To: Mingli Zhang <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote: > > Bruce Momjian <[email protected]>于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 <[email protected]> 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] freeze.diff (3.4K, ../../[email protected]/2-freeze.diff) download | inline diff: 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 { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable open and there are no older snapshots held by this transaction. It is currently not possible to perform a <command>COPY FREEZE</command> on a partitioned table. + This option is allowed only in <command>COPY FROM</command>. </para> <para> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: COPY TO (FREEZE)? @ 2023-10-30 03:22 Zhang Mingli <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Zhang Mingli @ 2023-10-30 03:22 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers; [email protected] HI, Zhang Mingli www.hashdata.xyz On Oct 30, 2023 at 10:58 +0800, Bruce Momjian <[email protected]>, wrote: > On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote: > > > > Bruce Momjian <[email protected]>于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 <[email protected]> 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 <[email protected]> https://momjian.us > EDB https://enterprisedb.com > > Only you can decide what is important to you. LGTM. ^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH 2/2] Provide the executor with information on updated columns. @ 2026-07-01 13:18 Antonin Houska <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Antonin Houska @ 2026-07-01 13:18 UTC (permalink / raw) When replaying data changes, REPACK calls ExecInsertIndexTuples() for each INSERT and UPDATE. In the latter case, the function needs to determine the value of the 'indexUnchanged' hint for the index AM. (The hint is always false for INSERT.) Therefore, REPACK is supposed to specify which columns are changed by the UPDATE. So far, REPACK missed to specify the set of updated columns, so the 'indexUnchanged' hint could incorrectly evaluate to true. Although it should not affect correctness, it can make the index AM use optimizations that are not appropriate. Ideally, we should compute the set of updated columns for each individual UPDATE, however the comparison of the old and new tuple might add too much overhead. This patch initializes the set as if all columns were updated. Thus 'indexUnchanged' always evaluates to false. This way the index AM never uses the related optimizations. It might result in worse structure of the index, however it seems better to not use the optimizations than to misuse them. --- src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index c9b0c047477..f37bb9a21af 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -62,6 +62,7 @@ #include "libpq/pqmq.h" #include "miscadmin.h" #include "optimizer/optimizer.h" +#include "parser/parse_relation.h" #include "pgstat.h" #include "replication/logicalrelation.h" #include "storage/bufmgr.h" @@ -3005,13 +3006,50 @@ static void initialize_change_context(ChangeContext *chgcxt, Relation relation, Oid ident_index_id) { + Bitmapset *updatedCols = NULL; + RangeTblEntry *rte; + List *perminfos = NIL; + RTEPermissionInfo *perminfo; + chgcxt->cc_rel = relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); + /* + * Initialize updatedCols. + * + * The point is that ExecInsertIndexTuples() should not pass the + * indexUnchanged hint to the index AM unless there's a reason to do + * so. For simplicity, we consider all columns updated. + * + * XXX Should we spend more effort to compare the old and new tuple when + * replaying UPDATE, or at least exclude unchanged TOAST values, like we + * do in logicalrep_write_tuple()? + */ + for (int i = 0; i < RelationGetDescr(relation)->natts; i++) + updatedCols = bms_add_member(updatedCols, + i + 1 - FirstLowInvalidHeapAttributeNumber); + + /* + * In this case, RTE only needs to have ->perminfoindex initialized, but + * there's no reason to not set the fields whose values we have at hand. + */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(relation); + rte->relkind = RelationGetForm(relation)->relkind; + /* Create the RTEPermissionInfo instance (and set ->perminfoindex). */ + addRTEPermissionInfo(&perminfos, rte); + /* Make updatedCols available to the executor functions. */ + perminfo = getRTEPermissionInfo(perminfos, rte); + perminfo->updatedCols = updatedCols; + + ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos, + bms_make_singleton(1)); + chgcxt->cc_rri = makeNode(ResultRelInfo); - InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0); + InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0); ExecOpenIndices(chgcxt->cc_rri, false); /* -- 2.52.0 --=-=-=-- ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-01 13:18 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-10-29 01:39 Re: COPY TO (FREEZE)? Tom Lane <[email protected]> 2023-10-29 01:47 ` Bruce Momjian <[email protected]> 2023-10-29 01:54 ` Tom Lane <[email protected]> 2023-10-29 02:03 ` Bruce Momjian <[email protected]> 2023-10-29 06:35 ` Mingli Zhang <[email protected]> 2023-10-29 06:50 ` Mingli Zhang <[email protected]> 2023-10-29 19:35 ` Bruce Momjian <[email protected]> 2023-10-29 21:07 ` Mingli Zhang <[email protected]> 2023-10-30 02:58 ` Bruce Momjian <[email protected]> 2023-10-30 03:22 ` Zhang Mingli <[email protected]> 2026-07-01 13:18 [PATCH 2/2] Provide the executor with information on updated columns. Antonin Houska <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox