Add support for DEFAULT specification in COPY FROM

On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote:

Hello all,

With the current implementation of COPY FROM in PostgreSQL we are able to
load the DEFAULT value/expression of a column if the column is absent
in the
list of specified columns. We are not able to explicitly ask that
PostgreSQL uses
the DEFAULT value/expression in a column that is being fetched from
the input
file, though.

This patch adds support for handling DEFAULT values in COPY FROM. It
works
similarly to NULL in COPY FROM: whenever the marker that was set for
DEFAULT
value/expression is read from the input stream, it will evaluate the
DEFAULT
value/expression of the corresponding column.

I'm currently working as a support engineer, and both me and some
customers had
already faced a situation where we missed an implementation like this
in COPY
FROM, and had to work around that by using an input file where the
column which
has a DEFAULT value/expression was removed.

That does not solve all issues though, as it might be the case that we
just want a
DEFAULT value to take place if no other value was set for the column
in the input
file, meaning we would like to have a column in the input file that
sometimes assume
the DEFAULT value/expression, and sometimes assume an actual given value.

The implementation was performed about one month ago and included all
regression
tests regarding the changes that were introduced. It was just rebased
on top of the
master branch before submitting this patch, and all tests are still
succeeding.

The implementation takes advantage of the logic that was already
implemented to
handle DEFAULT values for missing columns in COPY FROM. I just
modified it to
make it available the DEFAULT values/expressions for all columns
instead of only
for the ones that were missing in the specification. I had to change
the variables
accordingly, so it would index the correct positions in the new array
of DEFAULT
values/expressions.

Besides that, I also copied and pasted most of the checks that are
performed for the
NULL feature of COPY FROM, as the DEFAULT behaves somehow similarly.

Interesting, and probably useful. I've only had a brief look, but it's
important that the default marker not be quoted in CSV mode (c.f. NULL)
-f it is it should be taken as a literal rather than a special value.
Maybe that's taken care of, but there should at least be a test for it,
which I didn't see.

cheers

andrew

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

Hello Andrew,

Thanks for reviewing this patch.

It is worth noting that DEFAULT will only take place if explicitly
specified, meaning there is
no default value for the option DEFAULT. The usage of \D in the tests was
only a suggestion.
Also, NULL marker will be an unquoted empty string by default in CSV mode.

In any case I have manually tested it and the behavior is compliant to what
we see in NULL
if it is defined to use \N both in text and CSV modes.

- NULL as \N:

postgres=# CREATE TEMP TABLE copy_null (id integer primary key, value text);
CREATE TABLE
postgres=# copy copy_null from stdin with (format text, NULL '\N');
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.

1 \N
2 \\N
3 "\N"
\.

COPY 3
postgres=# TABLE copy_null ;
id | value
----+-------
1 |
2 | \N
3 | "N"
(3 rows)

postgres=# TRUNCATE copy_null ;
TRUNCATE TABLE
postgres=# copy copy_null from stdin with (format csv, NULL '\N');
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.

1,\N
2,\\N
3,"\N"
\.

COPY 3
postgres=# TABLE copy_null ;
id | value
----+-------
1 |
2 | \\N
3 | \N
(3 rows)

- DEFAULT as \D:

postgres=# CREATE TEMP TABLE copy_default (id integer primary key, value
text default 'test');
CREATE TABLE
postgres=# copy copy_default from stdin with (format text, DEFAULT '\D');
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.

1 \D
2 \\D
3 "\D"
\.

COPY 3
postgres=# TABLE copy_default ;
id | value
----+-------
1 | test
2 | \D
3 | "D"
(3 rows)

postgres=# TRUNCATE copy_default ;
TRUNCATE TABLE
postgres=# copy copy_default from stdin with (format csv, DEFAULT '\D');
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.

1,\D
2,\\D
3,"\D"
\.

COPY 3
postgres=# TABLE copy_default ;
id | value
----+-------
1 | test
2 | \\D
3 | \D
(3 rows)

If you do not specify DEFAULT in COPY FROM, it will have no default value
for
that option. So, if you try to load \D in CSV mode, then it will load the
literal value:

postgres=# CREATE TEMP TABLE copy (id integer primary key, value text
default 'test');
CREATE TABLE
postgres=# copy copy from stdin with (format csv);
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.

1,\D
2,\\D
3,"\D"
\.

COPY 3
postgres=# TABLE copy ;
id | value
----+-------
1 | \D
2 | \\D
3 | \D
(3 rows)

Does that address your concerns?

I am attaching the new patch, containing the above test in the regress
suite.

Best regards,
Israel.

Em ter., 16 de ago. de 2022 às 17:27, Andrew Dunstan <andrew@dunslane.net>
escreveu:

On 2022-08-17 We 17:12, Israel Barth Rubio wrote:

Does that address your concerns?

I am attaching the new patch, containing the above test in the regress
suite.

Thanks, yes, that all looks sane.

Please add this to the next CommitFest if you haven't already done so.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote:

Hello all,

With the current implementation of COPY FROM in PostgreSQL we are
able to load the DEFAULT value/expression of a column if the column
is absent in the list of specified columns. We are not able to
explicitly ask that PostgreSQL uses the DEFAULT value/expression in a
column that is being fetched from the input file, though.

This patch adds support for handling DEFAULT values in COPY FROM. It
works similarly to NULL in COPY FROM: whenever the marker that was
set for DEFAULT value/expression is read from the input stream, it
will evaluate the DEFAULT value/expression of the corresponding
column.

[…]

Interesting, and probably useful. I've only had a brief look, but it's
important that the default marker not be quoted in CSV mode (c.f. NULL)
-f it is it should be taken as a literal rather than a special value.

For the NULL marker that can be overridden for individual columns with
the FORCE(_NOT)_NULL option. This feature should have a similar
FORCE(_NOT)_DEFAULT option to allow the DEFAULT marker to be ignored, or
recognised even when quoted, respectively.

- ilmari

Hello,

Thanks for your review. I submitted the patch to the next commit fest
(https://commitfest.postgresql.org/39/3822/).

Regards,
Israel.

Em qua., 17 de ago. de 2022 às 18:56, Andrew Dunstan <andrew@dunslane.net>
escreveu:

Hello Ilmari,

Thanks for checking it, too. I can study to implement these changes
to include a way of overriding the behavior for the given columns.

Regards,
Israel.

Em qui., 18 de ago. de 2022 às 06:56, Dagfinn Ilmari Mannsåker <
ilmari@ilmari.org> escreveu:

On 2022-08-18 Th 05:55, Dagfinn Ilmari Mannsåker wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2022-08-16 Tu 14:12, Israel Barth Rubio wrote:

Hello all,

With the current implementation of COPY FROM in PostgreSQL we are
able to load the DEFAULT value/expression of a column if the column
is absent in the list of specified columns. We are not able to
explicitly ask that PostgreSQL uses the DEFAULT value/expression in a
column that is being fetched from the input file, though.

This patch adds support for handling DEFAULT values in COPY FROM. It
works similarly to NULL in COPY FROM: whenever the marker that was
set for DEFAULT value/expression is read from the input stream, it
will evaluate the DEFAULT value/expression of the corresponding
column.

[…]

Interesting, and probably useful. I've only had a brief look, but it's
important that the default marker not be quoted in CSV mode (c.f. NULL)
-f it is it should be taken as a literal rather than a special value.

For the NULL marker that can be overridden for individual columns with
the FORCE(_NOT)_NULL option. This feature should have a similar
FORCE(_NOT)_DEFAULT option to allow the DEFAULT marker to be ignored, or
recognised even when quoted, respectively.

That seems to be over-egging the pudding somewhat. FORCE_NOT_DEFAULT
should not be necessary at all, since here if there's no default
specified nothing will be taken as the default. I suppose a quoted
default is just faintly possible, but I'd like a concrete example of a
producer that emitted it.

cheers

andrew

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

On 2022-08-17 We 17:12, Israel Barth Rubio wrote:

Hello Andrew,

Thanks for reviewing this patch

[...]

I am attaching the new patch, containing the above test in the regress
suite.

Thanks, this looks good but there are some things that need attention:

. There needs to be a check that this is being used with COPY FROM, and
the restriction needs to be stated in the docs and tested for. c.f.
FORCE NULL.

. There needs to be support for this in psql's tab_complete.c, and
appropriate tests added

. There needs to be support for it in contrib/file_fdw/file_fdw.c, and a
test added

. The tests should include psql's \copy as well as sql COPY

. I'm not sure we need a separate regression test file for this.
Probably these tests can go at the end of src/test/regress/sql/copy2.sql.

cheers

andrew

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

Hello Andrew,

. There needs to be a check that this is being used with COPY FROM, and
the restriction needs to be stated in the docs and tested for. c.f.
FORCE NULL.

. There needs to be support for this in psql's tab_complete.c, and
appropriate tests added

. There needs to be support for it in contrib/file_fdw/file_fdw.c, and a
test added

. The tests should include psql's \copy as well as sql COPY

. I'm not sure we need a separate regression test file for this.
Probably these tests can go at the end of src/test/regress/sql/copy2.sql.

Thanks for your review! I have applied the suggested changes, and I'm
submitting the new patch version.

Kind regards,
Israel.

On Mon, Sep 26, 2022 at 8:12 AM Israel Barth Rubio <barthisrael@gmail.com>
wrote:

Hello Andrew,

. There needs to be a check that this is being used with COPY FROM, and
the restriction needs to be stated in the docs and tested for. c.f.
FORCE NULL.

. There needs to be support for this in psql's tab_complete.c, and
appropriate tests added

. There needs to be support for it in contrib/file_fdw/file_fdw.c, and a
test added

. The tests should include psql's \copy as well as sql COPY

. I'm not sure we need a separate regression test file for this.
Probably these tests can go at the end of src/test/regress/sql/copy2.sql.

Thanks for your review! I have applied the suggested changes, and I'm
submitting the new patch version.

Kind regards,
Israel.

Hi,

+ /* attribute is NOT to be copied from input */

I think saying `is NOT copied from input` should suffice.

+   defaults = (bool *) palloc0(num_phys_attrs * sizeof(bool));
+   MemSet(defaults, false, num_phys_attrs * sizeof(bool));

Is the MemSet() call necessary ?

+ /* fieldno is 0-index and attnum is 1-index */

0-index -> 0-indexed

Cheers

Hi,

On 2022-09-26 12:12:15 -0300, Israel Barth Rubio wrote:

Thanks for your review! I have applied the suggested changes, and I'm
submitting the new patch version.

cfbot shows that tests started failing with this version:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3822

https://cirrus-ci.com/task/5354378189078528?logs=test_world#L267

[11:03:09.595] ============== running regression test queries ==============
[11:03:09.595] test file_fdw ... FAILED (test process exited with exit code 2) 441 ms
[11:03:09.595] ============== shutting down postmaster ==============
[11:03:09.595]
[11:03:09.595] ======================
[11:03:09.595] 1 of 1 tests failed.
[11:03:09.595] ======================
[11:03:09.595]
[11:03:09.595] The differences that caused some tests to fail can be viewed in the
[11:03:09.595] file "/tmp/cirrus-ci-build/build/testrun/file_fdw/regress/regression.diffs". A copy of the test summary that you see
[11:03:09.595] above is saved in the file "/tmp/cirrus-ci-build/build/testrun/file_fdw/regress/regression.out".
[11:03:09.595]
[11:03:09.595] # test failed

The reason for the failure is a crash:
https://api.cirrus-ci.com/v1/artifact/task/5354378189078528/testrun/build/testrun/file_fdw/regress/log/postmaster.log

2022-09-30 11:01:29.228 UTC client backend[26885] pg_regress/file_fdw ERROR: cannot insert into foreign table "p1"
2022-09-30 11:01:29.228 UTC client backend[26885] pg_regress/file_fdw STATEMENT: UPDATE pt set a = 1 where a = 2;
TRAP: FailedAssertion("CurrentMemoryContext == econtext->ecxt_per_tuple_memory", File: "../src/backend/commands/copyfromparse.c", Line: 956, PID: 26885)
postgres: postgres regression [local] SELECT(ExceptionalCondition+0x8d)[0x559ed2fdf600]
postgres: postgres regression [local] SELECT(NextCopyFrom+0x3e4)[0x559ed2c4e3cb]
/tmp/cirrus-ci-build/build/tmp_install/usr/local/lib/x86_64-linux-gnu/postgresql/file_fdw.so(+0x2eef)[0x7ff42d072eef]
postgres: postgres regression [local] SELECT(+0x2cc400)[0x559ed2cff400]
postgres: postgres regression [local] SELECT(+0x2ba0eb)[0x559ed2ced0eb]
postgres: postgres regression [local] SELECT(ExecScan+0x6d)[0x559ed2ced178]
postgres: postgres regression [local] SELECT(+0x2cc43e)[0x559ed2cff43e]
postgres: postgres regression [local] SELECT(+0x2af6d5)[0x559ed2ce26d5]
postgres: postgres regression [local] SELECT(standard_ExecutorRun+0x15f)[0x559ed2ce28b0]
postgres: postgres regression [local] SELECT(ExecutorRun+0x25)[0x559ed2ce297e]
postgres: postgres regression [local] SELECT(+0x47275b)[0x559ed2ea575b]
postgres: postgres regression [local] SELECT(PortalRun+0x307)[0x559ed2ea71af]
postgres: postgres regression [local] SELECT(+0x47013a)[0x559ed2ea313a]
postgres: postgres regression [local] SELECT(PostgresMain+0x774)[0x559ed2ea5054]
postgres: postgres regression [local] SELECT(+0x3d41f4)[0x559ed2e071f4]
postgres: postgres regression [local] SELECT(+0x3d73a5)[0x559ed2e0a3a5]
postgres: postgres regression [local] SELECT(+0x3d75b7)[0x559ed2e0a5b7]
postgres: postgres regression [local] SELECT(PostmasterMain+0x1215)[0x559ed2e0bc52]
postgres: postgres regression [local] SELECT(main+0x231)[0x559ed2d46f17]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xea)[0x7ff43892dd0a]
postgres: postgres regression [local] SELECT(_start+0x2a)[0x559ed2b0204a]

A full backtrace is at https://api.cirrus-ci.com/v1/task/5354378189078528/logs/cores.log

Regards,

Andres Freund

Hello Zhihong,

+ /* attribute is NOT to be copied from input */

I think saying `is NOT copied from input` should suffice.

+ /* fieldno is 0-index and attnum is 1-index */

0-index -> 0-indexed

I have applied both suggestions, thanks! I'll submit a 4th version
of the patch soon.

+   defaults = (bool *) palloc0(num_phys_attrs * sizeof(bool));
+   MemSet(defaults, false, num_phys_attrs * sizeof(bool));

Is the MemSet() call necessary ?

I would say it is, so it initializes the array with all flags set to false.
Later, if it detects attributes that should evaluate their default
expression,
it would set the flag to true.

Am I missing something?

Regards,
Israel.

On Fri, Oct 7, 2022 at 12:09 PM Israel Barth Rubio <barthisrael@gmail.com>
wrote:

Hello Zhihong,

+ /* attribute is NOT to be copied from input */

I think saying `is NOT copied from input` should suffice.

+ /* fieldno is 0-index and attnum is 1-index */

0-index -> 0-indexed

I have applied both suggestions, thanks! I'll submit a 4th version
of the patch soon.

+   defaults = (bool *) palloc0(num_phys_attrs * sizeof(bool));
+   MemSet(defaults, false, num_phys_attrs * sizeof(bool));

Is the MemSet() call necessary ?

I would say it is, so it initializes the array with all flags set to false.
Later, if it detects attributes that should evaluate their default
expression,
it would set the flag to true.

Am I missing something?

Regards,
Israel.

Hi,
For the last question, please take a look at:

#define MemSetAligned(start, val, len) \

which is called by palloc0().

Hello Andres,

cfbot shows that tests started failing with this version:

https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3822

A full backtrace is at

https://api.cirrus-ci.com/v1/task/5354378189078528/logs/cores.log

Thanks for pointing this out. I had initially missed this as my local runs
of *make check*
were working fine, sorry!

I'm attaching a new version of the patch, containing the memory context
switches.

Regards,
Israel.

Hello Zhihong,

For the last question, please take a look at:

#define MemSetAligned(start, val, len) \

which is called by palloc0().

Oh, I totally missed that. Thanks for the heads up!

I'm attaching the new patch version, which contains both the fix
to the problem reported by Andres, and removes this useless
MemSet call.

Best regards,
Israel.

Hello all,

I'm submitting a new version of the patch. Instead of changing signature
of several functions in order to use the defaults parameter, it is now
storing
that in the cstate structure, which is already passed to all functions that
were previously modified.

Best regards,
Israel.

Em sex., 7 de out. de 2022 às 17:54, Israel Barth Rubio <
barthisrael@gmail.com> escreveu:

On 2022-12-02 Fr 09:11, Israel Barth Rubio wrote:

Hello all,

I'm submitting a new version of the patch. Instead of changing signature
of several functions in order to use the defaults parameter, it is now
storing
that in the cstate structure, which is already passed to all functions
that
were previously modified.

I'm reviewing this and it looks in pretty good shape. I notice that in
file_fdw.c:fileIterateForeignScan() we unconditionally generate the
estate, switch context etc, whether or not there is a default option
used. I guess there's no harm in that, and the performance impact should
be minimal, but I thought it worth mentioning, as it's probably not
strictly necessary.

cheers

andrew

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

On 2022-12-02 Fr 09:11, Israel Barth Rubio wrote:

Hello all,

I'm submitting a new version of the patch. Instead of changing signature
of several functions in order to use the defaults parameter, it is now
storing
that in the cstate structure, which is already passed to all functions
that
were previously modified.

Thanks, committed.

cheers

andrew

--

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

Hello,
13.03.2023 17:15, Andrew Dunstan wrote:

On 2022-12-02 Fr 09:11, Israel Barth Rubio wrote:

Hello all,

I'm submitting a new version of the patch. Instead of changing signature
of several functions in order to use the defaults parameter, it is now storing
that in the cstate structure, which is already passed to all functions that
were previously modified.

Thanks, committed.

Please look at the query:
create table t (f1 int);
copy t from stdin with (format csv, default '\D');
1,\D

that invokes an assertion failure after 9f8377f7a:
Core was generated by `postgres: law regression [local]
COPY                                         '.
Program terminated with signal SIGABRT, Aborted.

warning: Section `.reg-xstate/3253881' in core file too small.
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140665061189440)
at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140665061189440)
at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140665061189440) at
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140665061189440, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3  0x00007fef2250e476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4  0x00007fef224f47f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00005600fd395750 in ExceptionalCondition (
    conditionName=conditionName@entry=0x5600fd3fa751 "n >= 0 && n < list->length",
    fileName=fileName@entry=0x5600fd416db8
"../../../src/include/nodes/pg_list.h", lineNumber=lineNumber@entry=280)
    at assert.c:66
#6  0x00005600fd02626d in list_nth_cell (n=<optimized out>, list=<optimized out>)
    at ../../../src/include/nodes/pg_list.h:280
#7  list_nth_int (n=<optimized out>, list=<optimized out>) at
../../../src/include/nodes/pg_list.h:313
#8  CopyReadAttributesCSV (cstate=<optimized out>) at copyfromparse.c:1905
#9  0x00005600fd0265a5 in NextCopyFromRawFields (cstate=0x5600febdd238,
fields=0x7fff12ef7130, nfields=0x7fff12ef712c)
    at copyfromparse.c:833
#10 0x00005600fd0267f9 in NextCopyFrom (cstate=cstate@entry=0x5600febdd238,
econtext=econtext@entry=0x5600fec9c5c8,
    values=0x5600febdd5c8, nulls=0x5600febdd5d0) at copyfromparse.c:885
#11 0x00005600fd0234db in CopyFrom (cstate=cstate@entry=0x5600febdd238) at
copyfrom.c:989
#12 0x00005600fd0222e5 in DoCopy (pstate=0x5600febdc568, stmt=0x5600febb2d58,
stmt_location=0, stmt_len=49,
    processed=0x7fff12ef7340) at copy.c:308
#13 0x00005600fd25c5e9 in standard_ProcessUtility (pstmt=0x5600febb2e78,
    queryString=0x5600febb2178 "copy t from stdin with (format csv, default
'\\D');", readOnlyTree=<optimized out>,
    context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x5600febb3138, qc=0x7fff12ef7600)
    at utility.c:742
#14 0x00005600fd25a9f1 in PortalRunUtility (portal=portal@entry=0x5600fec4ea48,
pstmt=pstmt@entry=0x5600febb2e78,
    isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x5600febb3138,
    qc=qc@entry=0x7fff12ef7600) at pquery.c:1158
#15 0x00005600fd25ab2d in PortalRunMulti (portal=portal@entry=0x5600fec4ea48,
isTopLevel=isTopLevel@entry=true,
    setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x5600febb3138,
    altdest=altdest@entry=0x5600febb3138, qc=qc@entry=0x7fff12ef7600) at
pquery.c:1315
#16 0x00005600fd25b1c1 in PortalRun (portal=portal@entry=0x5600fec4ea48,
count=count@entry=9223372036854775807,
    isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x5600febb3138,
    altdest=altdest@entry=0x5600febb3138, qc=0x7fff12ef7600) at pquery.c:791
#17 0x00005600fd256f34 in exec_simple_query (
    query_string=0x5600febb2178 "copy t from stdin with (format csv, default
'\\D');") at postgres.c:1240
#18 0x00005600fd258ae7 in PostgresMain (dbname=<optimized out>,
username=<optimized out>) at postgres.c:4572
#19 0x00005600fd1c2d3f in BackendRun (port=0x5600febe05c0, port=0x5600febe05c0)
at postmaster.c:4461
#20 BackendStartup (port=0x5600febe05c0) at postmaster.c:4189
#21 ServerLoop () at postmaster.c:1779
#22 0x00005600fd1c3d63 in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x5600febad640) at postmaster.c:1463
#23 0x00005600fced4fc6 in main (argc=3, argv=0x5600febad640) at main.c:200

Best regards,
Alexander