Horton

Thread

Commits

  1. Add a DEFAULT option to COPY FROM

  1. Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-08-16T18:12:20Z

    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.

Best regards,
Israel.

  2. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-16T20:27:28Z

    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

  3. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-08-17T21:12:04Z

    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-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
>
>

  4. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-17T21:56:07Z

    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

  5. Re: Add support for DEFAULT specification in COPY FROM

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-08-18T09:55:58Z

    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

  6. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-08-18T14:36:05Z

    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:

>
> 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
>
>

  7. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-08-18T14:39:45Z

    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:

> 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
>

  8. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-18T15:01:48Z

    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

  9. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2022-09-14T22:29:16Z

    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

  10. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-09-26T15:12:15Z

    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.

  11. Re: Add support for DEFAULT specification in COPY FROM

    Zhihong Yu <zyu@yugabyte.com> — 2022-09-26T15:23:02Z

    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

  12. Re: Add support for DEFAULT specification in COPY FROM

    Andres Freund <andres@anarazel.de> — 2022-10-02T17:17:39Z

    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

  13. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-10-07T19:09:28Z

    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.

>

  14. Re: Add support for DEFAULT specification in COPY FROM

    Zhihong Yu <zyu@yugabyte.com> — 2022-10-07T19:16:22Z

    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().

  15. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-10-07T19:17:35Z

    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.

  16. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-10-07T20:54:46Z

    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.

  17. Re: Add support for DEFAULT specification in COPY FROM

    Israel Barth Rubio <barthisrael@gmail.com> — 2022-12-02T14:11:28Z

    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:

> 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.
>

  18. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-09T13:52:41Z

    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

  19. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-13T14:15:38Z

    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

  20. Re: Add support for DEFAULT specification in COPY FROM

    Alexander Law <exclusion@gmail.com> — 2023-03-15T17:00:00Z

    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

  21. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-15T20:43:33Z

    On 2023-03-15 We 13:00, Alexander Lakhin wrote:
> 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
>
>

Thanks for the test case. Will fix.


cheers


andrew


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

  22. Re: Add support for DEFAULT specification in COPY FROM

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-15T21:21:49Z

    On 2023-03-15 We 13:00, Alexander Lakhin wrote:
> 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.
>
>

Fix pushed, thanks for the report.


cheers


andrew

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