Thread: Add support for DEFAULT specification in COPY FROM
Hello all,
With the current implementation of COPY FROM in PostgreSQL we are able to
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.
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
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
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.
Best regards,
Israel.
Attachment
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.
Does that address your concerns?
I am attaching the new patch, containing the above test in the regress suite.
Best regards,
Israel.
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:
- DEFAULT as \D: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)
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)
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
Attachment
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.
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
Hello Ilmari,
Thanks for checking it, too. I can study to implement these changes
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.
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
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
> . 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.
Attachment
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'msubmitting 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));
+ 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 thatyou 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 */
>
> + /* 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
I have applied both suggestions, thanks! I'll submit a 4th version
of the patch soon.
> + defaults = (bool *) palloc0(num_phys_attrs * sizeof(bool));
> + 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,
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.
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 versionof 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.
Israel.
Attachment
Hello Zhihong,
> For the last question, please take a look at:
> 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
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.
Best regards,
Israel.
Attachment
Hello all,
I'm submitting a new version of the patch. Instead of changing signature
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.
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 fixto the problem reported by Andres, and removes this uselessMemSet call.
Best regards,
Israel.
Attachment
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:
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
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
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