Regression in COPY FROM caused by 9f8377f7a2

Started by Laurenz Albeover 2 years ago9 messageshackers
Jump to latest
#1Laurenz Albe
laurenz.albe@cybertec.at

In v16 and later, the following fails:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

COPY boom FROM STDIN;
ERROR: value too long for type character varying(5)

In PostgreSQL v15 and earlier, the COPY statement succeeds.

The error is thrown in BeginCopyFrom in line 1578 (HEAD)

defexpr = expression_planner(defexpr);

Bisecting shows that the regression was introduced by commit 9f8377f7a2,
which introduced DEFAULT values for COPY FROM.

The table definition is clearly silly, so I am not sure if that
regression is worth fixing. On the other hand, it is not cool if
something that worked without an error in v15 starts to fail later on.

Yours,
Laurenz Albe

#2Laurenz Albe
laurenz.albe@cybertec.at
In reply to: Laurenz Albe (#1)
Re: Regression in COPY FROM caused by 9f8377f7a2

On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote:

In v16 and later, the following fails:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

COPY boom FROM STDIN;
ERROR:  value too long for type character varying(5)

In PostgreSQL v15 and earlier, the COPY statement succeeds.

The error is thrown in BeginCopyFrom in line 1578 (HEAD)

  defexpr = expression_planner(defexpr);

Bisecting shows that the regression was introduced by commit 9f8377f7a2,
which introduced DEFAULT values for COPY FROM.

I suggest the attached fix, which evaluates default values only if
the DEFAULT option was specified or if the column does not appear in
the column list of COPY.

Yours,
Laurenz Albe

Attachments:

0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.patchtext/x-patch; charset=UTF-8; name=0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.patchDownload+8-2
#3Andrew Dunstan
andrew@dunslane.net
In reply to: Laurenz Albe (#2)
Re: Regression in COPY FROM caused by 9f8377f7a2

On 2023-09-25 Mo 04:59, Laurenz Albe wrote:

On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote:

In v16 and later, the following fails:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

COPY boom FROM STDIN;
ERROR:  value too long for type character varying(5)

In PostgreSQL v15 and earlier, the COPY statement succeeds.

The error is thrown in BeginCopyFrom in line 1578 (HEAD)

  defexpr = expression_planner(defexpr);

Bisecting shows that the regression was introduced by commit 9f8377f7a2,
which introduced DEFAULT values for COPY FROM.

Oops :-(

I suggest the attached fix, which evaluates default values only if
the DEFAULT option was specified or if the column does not appear in
the column list of COPY.

Patch looks reasonable, haven't tested yet.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

#4Andrew Dunstan
andrew@dunslane.net
In reply to: Andrew Dunstan (#3)
Re: Regression in COPY FROM caused by 9f8377f7a2

On 2023-09-25 Mo 11:06, Andrew Dunstan wrote:

On 2023-09-25 Mo 04:59, Laurenz Albe wrote:

On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote:

In v16 and later, the following fails:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

COPY boom FROM STDIN;
ERROR:  value too long for type character varying(5)

In PostgreSQL v15 and earlier, the COPY statement succeeds.

The error is thrown in BeginCopyFrom in line 1578 (HEAD)

  defexpr = expression_planner(defexpr);

Bisecting shows that the regression was introduced by commit 9f8377f7a2,
which introduced DEFAULT values for COPY FROM.

Thinking about this a little more, wouldn't it be better if we checked
at the time we set the default that the value is actually valid for the
given column? This is only one manifestation of a problem you could run
into given this table definition.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#4)
Re: Regression in COPY FROM caused by 9f8377f7a2

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-09-25 Mo 11:06, Andrew Dunstan wrote:

On 2023-09-25 Mo 04:59, Laurenz Albe wrote:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

Thinking about this a little more, wouldn't it be better if we checked
at the time we set the default that the value is actually valid for the
given column? This is only one manifestation of a problem you could run
into given this table definition.

I dunno, it seems at least possible that someone would do this
deliberately as a means of preventing the column from being defaulted.
In any case, the current behavior has stood for a very long time and
no one has complained that an error should be thrown sooner.

regards, tom lane

#6Laurenz Albe
laurenz.albe@cybertec.at
In reply to: Tom Lane (#5)
Re: Regression in COPY FROM caused by 9f8377f7a2

On Mon, 2023-09-25 at 17:49 -0400, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-09-25 Mo 11:06, Andrew Dunstan wrote:

On 2023-09-25 Mo 04:59, Laurenz Albe wrote:

CREATE TABLE boom (t character varying(5) DEFAULT 'a long string');

Thinking about this a little more, wouldn't it be better if we checked
at the time we set the default that the value is actually valid for the
given column? This is only one manifestation of a problem you could run
into given this table definition.

I dunno, it seems at least possible that someone would do this
deliberately as a means of preventing the column from being defaulted.
In any case, the current behavior has stood for a very long time and
no one has complained that an error should be thrown sooner.

Moreover, this makes restoring a pg_dump from v15 to v16 fail, which
should never happen. This is how I got that bug report.

Yours,
Laurenz Albe

#7Laurenz Albe
laurenz.albe@cybertec.at
In reply to: Laurenz Albe (#6)
Re: Regression in COPY FROM caused by 9f8377f7a2

Here is an improved version of the patch with regression tests.

Yours,
Laurenz Albe

Attachments:

0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.V2.patchtext/x-patch; charset=UTF-8; name=0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.V2.patchDownload+40-2
#8Andrew Dunstan
andrew@dunslane.net
In reply to: Laurenz Albe (#7)
Re: Regression in COPY FROM caused by 9f8377f7a2

On 2023-09-26 Tu 04:11, Laurenz Albe wrote:

Here is an improved version of the patch with regression tests.

Thanks, pushed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

#9Laurenz Albe
laurenz.albe@cybertec.at
In reply to: Andrew Dunstan (#8)
Re: Regression in COPY FROM caused by 9f8377f7a2

On Sun, 2023-10-01 at 10:55 -0400, Andrew Dunstan wrote:

Thanks, pushed.

Thanks for taking care of that.

Yours,
Laurenz Albe