Thread
-
on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2024-02-03T06:22:34Z
Hi. I previously did some work in COPY FROM save error information to a table. still based on this suggestion: https://www.postgresql.org/message-id/752672.1699474336%40sss.pgh.pa.us Now I refactored it. the syntax: ON_ERROR 'table', TABLE 'error_saving_tbl' if ON_ERROR is not specified with 'table', TABLE is specified, then error. if ON_ERROR is specified with 'table', TABLE is not specified or error_saving_tbl does not exist, then error. In BeginCopyFrom, we check the data definition of error_saving_table, we also check if the user has INSERT privilege to error_saving_table (all the columns). We also did a preliminary check of the lock condition of error_saving_table. if it does not meet these conditions, we quickly error out. error_saving_table will be the same schema as the copy from table. Because "table" is a keyword, I have to add the following changes to gram.y. --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -3420,6 +3420,10 @@ copy_opt_item: { $$ = makeDefElem("null", (Node *) makeString($3), @1); } + | TABLE opt_as Sconst + { + $$ = makeDefElem("table", (Node *) makeString($3), @1); + } since "table" is already a keyword, so there is no influence on the parsing speed? demo: create table err_tbl( userid oid, -- the user oid while copy generated this entry copy_tbl oid, --copy table filename text, lineno int8, line text, colname text, raw_field_value text, err_message text, err_detail text, errorcode text ); create table t_copy_tbl(a int, b int, c int); COPY t_copy_tbl FROM STDIN WITH (delimiter ',', on_error 'table', table err_tbl); 1,2,a \. table err_tbl \gx -[ RECORD 1 ]---+------------------------------------------- userid | 10 copy_tbl | 17920 filename | STDIN lineno | 1 line | 1,2,a colname | c raw_field_value | a err_message | invalid input syntax for type integer: "a" err_detail | errorcode | 22P02 -
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-07-15T05:42:40Z
Thanks for the patch! I was not able to understand why we need a special error table for COPY? Can't we just log it in a new log error file for COPY in a proper format? Like using table approach, PG would be unnecessarily be utilising its resources like extra CPU to format the data in pages to store in its table format, writing and keeping the table in its store (which may or may not be required), the user would be required to make sure it creates the error table with proper columns to be used in COPY, etc.. Meanwhile, please find some quick review comments:- 1) Patch needs re-base. 2) If the columns of the error table are fixed, then why not create it internally using some function or something instead of making the user create the table correctly with all the columns? 3) I think, error messages can be improved, which looks big to me. 4) I think no need of below pstrdup, As CStringGetTextDatum creates a text copy for the same:- err_code = pstrdup(unpack_sql_state(cstate->escontext->error_data->sqlerrcode)); t_values[9] = CStringGetTextDatum(err_code); 5) Should 'on_error_rel' as not NULL be checked along with below, as I can see it is passed as NULL from two locations? + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) + { 6) Below declarations can be shifted to the if block, where they are used. Instead of keeping them as global in function? + HeapTuple on_error_tup; + TupleDesc on_error_tupDesc; + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) + { 7) Below comment going beyond 80 char width:- * if on_error is specified with 'table', then on_error_rel is the error saving table 8) Need space after 'false' err_detail ? false: true; 9) Below call can fit in a single line. No need to keep the 2nd and 3rd parameter in nextlines. + on_error_tup = heap_form_tuple(on_error_tupDesc, + t_values, + t_isnull); 10) Variable declarations Tab Spacing issue at multiple places. 11) Comments in the patch are not matched to PG comment style. Kindly note I have not tested the patch properly yet. Only checked it with a positive test case. As I will wait for a conclusion on my opinion of the proposed patch. Regards, Nishant Sharma. EnterpriseDB, Pune. On Sat, Feb 3, 2024 at 11:52 AM jian he <jian.universality@gmail.com> wrote: > Hi. > I previously did some work in COPY FROM save error information to a table. > still based on this suggestion: > https://www.postgresql.org/message-id/752672.1699474336%40sss.pgh.pa.us > Now I refactored it. > > the syntax: > ON_ERROR 'table', TABLE 'error_saving_tbl' > > if ON_ERROR is not specified with 'table', TABLE is specified, then error. > if ON_ERROR is specified with 'table', TABLE is not specified or > error_saving_tbl does not exist, then error. > > In BeginCopyFrom, we check the data definition of error_saving_table, > we also check if the user has INSERT privilege to error_saving_table > (all the columns). > We also did a preliminary check of the lock condition of > error_saving_table. > > if it does not meet these conditions, we quickly error out. > error_saving_table will be the same schema as the copy from table. > > Because "table" is a keyword, I have to add the following changes to > gram.y. > --- a/src/backend/parser/gram.y > +++ b/src/backend/parser/gram.y > @@ -3420,6 +3420,10 @@ copy_opt_item: > { > $$ = makeDefElem("null", (Node *) makeString($3), @1); > } > + | TABLE opt_as Sconst > + { > + $$ = makeDefElem("table", (Node *) makeString($3), @1); > + } > > since "table" is already a keyword, so there is no influence on the > parsing speed? > > demo: > > create table err_tbl( > userid oid, -- the user oid while copy generated this entry > copy_tbl oid, --copy table > filename text, > lineno int8, > line text, > colname text, > raw_field_value text, > err_message text, > err_detail text, > errorcode text > ); > create table t_copy_tbl(a int, b int, c int); > COPY t_copy_tbl FROM STDIN WITH (delimiter ',', on_error 'table', > table err_tbl); > 1,2,a > \. > > table err_tbl \gx > -[ RECORD 1 ]---+------------------------------------------- > userid | 10 > copy_tbl | 17920 > filename | STDIN > lineno | 1 > line | 1,2,a > colname | c > raw_field_value | a > err_message | invalid input syntax for type integer: "a" > err_detail | > errorcode | 22P02 > -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2024-08-20T00:00:00Z
On Mon, Jul 15, 2024 at 1:42 PM Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: > > Thanks for the patch! > > I was not able to understand why we need a special error table for COPY? > Can't we just log it in a new log error file for COPY in a proper format? Like > using table approach, PG would be unnecessarily be utilising its resources like > extra CPU to format the data in pages to store in its table format, writing and > keeping the table in its store (which may or may not be required), the user > would be required to make sure it creates the error table with proper columns > to be used in COPY, etc.. > > > Meanwhile, please find some quick review comments:- > > 1) Patch needs re-base. > > 2) If the columns of the error table are fixed, then why not create it internally using > some function or something instead of making the user create the table correctly > with all the columns? I'll think about it more. previously, i tried to use SPI to create tables, but at that time, i thought that's kind of excessive. you need to create the table, check whether the table name is unique, check the privilege. now we quickly error out if the error saving table definition does not meet. I guess that's less work to do. > 3) I think, error messages can be improved, which looks big to me. > > 4) I think no need of below pstrdup, As CStringGetTextDatum creates a text copy for > the same:- > err_code = pstrdup(unpack_sql_state(cstate->escontext->error_data->sqlerrcode)); > > t_values[9] = CStringGetTextDatum(err_code); > > 5) Should 'on_error_rel' as not NULL be checked along with below, as I can see it is > passed as NULL from two locations? > + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) > + { > > 6) Below declarations can be shifted to the if block, where they are used. Instead of > keeping them as global in function? > + HeapTuple on_error_tup; > + TupleDesc on_error_tupDesc; > > + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) > + { > > 7) Below comment going beyond 80 char width:- > * if on_error is specified with 'table', then on_error_rel is the error saving table > > 8) Need space after 'false' > err_detail ? false: true; > > 9) Below call can fit in a single line. No need to keep the 2nd and 3rd parameter in > nextlines. > + on_error_tup = heap_form_tuple(on_error_tupDesc, > + t_values, > + t_isnull); > > 10) Variable declarations Tab Spacing issue at multiple places. > > 11) Comments in the patch are not matched to PG comment style. > > > Kindly note I have not tested the patch properly yet. Only checked it with a positive test > case. As I will wait for a conclusion on my opinion of the proposed patch. > almost all these issues have been addressed. The error messages are still not so good. I need to polish it. -
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-11-05T10:30:30Z
Thanks for the v2 patch! I see v1 review comments got addressed in v2 along with some further improvements. 1) v2 Patch again needs re-base. 2) I think we need not worry whether table name is unique or not, table name can be provided by user and we can check if it does not exists then simply we can create it with appropriate columns, if it exists we use it to check if its correct on_error table and proceed. 3) Using #define in between the code? I don't see that style in copyfromparse.c file. I do see such style in other src file. So, not sure if committer would allow it or not. #define ERROR_TBL_COLUMNS 10 4) Below appears redundant to me, it was not the case in v1 patch set, where it had only one return and one increment of error as new added code was at the end of the block:- + cstate->num_errors++; + return true; + } cstate->num_errors++; I was not able to test the v2 due to conflicts in v2, I did attempt to resolve but I saw many failures in make world. Regards, Nishant. On Tue, Aug 20, 2024 at 5:31 AM jian he <jian.universality@gmail.com> wrote: > On Mon, Jul 15, 2024 at 1:42 PM Nishant Sharma > <nishant.sharma@enterprisedb.com> wrote: > > > > Thanks for the patch! > > > > I was not able to understand why we need a special error table for COPY? > > Can't we just log it in a new log error file for COPY in a proper > format? Like > > using table approach, PG would be unnecessarily be utilising its > resources like > > extra CPU to format the data in pages to store in its table format, > writing and > > keeping the table in its store (which may or may not be required), the > user > > would be required to make sure it creates the error table with proper > columns > > to be used in COPY, etc.. > > > > > > Meanwhile, please find some quick review comments:- > > > > 1) Patch needs re-base. > > > > 2) If the columns of the error table are fixed, then why not create it > internally using > > some function or something instead of making the user create the table > correctly > > with all the columns? > > I'll think about it more. > previously, i tried to use SPI to create tables, but at that time, i > thought that's kind of excessive. > you need to create the table, check whether the table name is unique, > check the privilege. > now we quickly error out if the error saving table definition does not > meet. I guess that's less work to do. > > > > 3) I think, error messages can be improved, which looks big to me. > > > > 4) I think no need of below pstrdup, As CStringGetTextDatum creates a > text copy for > > the same:- > > err_code = > pstrdup(unpack_sql_state(cstate->escontext->error_data->sqlerrcode)); > > > > t_values[9] = CStringGetTextDatum(err_code); > > > > 5) Should 'on_error_rel' as not NULL be checked along with below, as I > can see it is > > passed as NULL from two locations? > > + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) > > + { > > > > 6) Below declarations can be shifted to the if block, where they are > used. Instead of > > keeping them as global in function? > > + HeapTuple on_error_tup; > > + TupleDesc on_error_tupDesc; > > > > + if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) > > + { > > > > 7) Below comment going beyond 80 char width:- > > * if on_error is specified with 'table', then on_error_rel is the error > saving table > > > > 8) Need space after 'false' > > err_detail ? false: true; > > > > 9) Below call can fit in a single line. No need to keep the 2nd and 3rd > parameter in > > nextlines. > > + on_error_tup = heap_form_tuple(on_error_tupDesc, > > + t_values, > > + t_isnull); > > > > 10) Variable declarations Tab Spacing issue at multiple places. > > > > 11) Comments in the patch are not matched to PG comment style. > > > > > > Kindly note I have not tested the patch properly yet. Only checked it > with a positive test > > case. As I will wait for a conclusion on my opinion of the proposed > patch. > > > almost all these issues have been addressed. > The error messages are still not so good. I need to polish it. > -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2024-12-03T04:28:53Z
On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: > > Thanks for the v2 patch! > > I see v1 review comments got addressed in v2 along with some > further improvements. > > 1) v2 Patch again needs re-base. > > 2) I think we need not worry whether table name is unique or not, > table name can be provided by user and we can check if it does > not exists then simply we can create it with appropriate columns, > if it exists we use it to check if its correct on_error table and > proceed. "simply we can create it with appropriate columns," that would be more work. so i stick to if there is a table can use to error saving then use it, otherwise error out. > > 3) Using #define in between the code? I don't see that style in > copyfromparse.c file. I do see such style in other src file. So, not > sure if committer would allow it or not. > #define ERROR_TBL_COLUMNS 10 > let's wait and see. > 4) Below appears redundant to me, it was not the case in v1 patch > set, where it had only one return and one increment of error as new > added code was at the end of the block:- > + cstate->num_errors++; > + return true; > + } > cstate->num_errors++; > changed per your advice. > I was not able to test the v2 due to conflicts in v2, I did attempt to > resolve but I saw many failures in make world. > I get rid of all the SPI code. Instead, now I iterate through AttributeRelationId to check if the error saving table is ok or not, using DirectFunctionCall3 to do the privilege check. removed gram.y change, turns out it is not necessary. and other kinds of refactoring. please check attached.
-
Re: on_error table, saving error info to a table
Kirill Reshke <reshkekirill@gmail.com> — 2024-12-03T10:28:40Z
On Tue, 3 Dec 2024 at 09:29, jian he <jian.universality@gmail.com> wrote: > > On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma > <nishant.sharma@enterprisedb.com> wrote: > > > > Thanks for the v2 patch! > > > > I see v1 review comments got addressed in v2 along with some > > further improvements. > > > > 1) v2 Patch again needs re-base. > > > > 2) I think we need not worry whether table name is unique or not, > > table name can be provided by user and we can check if it does > > not exists then simply we can create it with appropriate columns, > > if it exists we use it to check if its correct on_error table and > > proceed. > > "simply we can create it with appropriate columns," > that would be more work. > so i stick to if there is a table can use to > error saving then use it, otherwise error out. > > > > > > 3) Using #define in between the code? I don't see that style in > > copyfromparse.c file. I do see such style in other src file. So, not > > sure if committer would allow it or not. > > #define ERROR_TBL_COLUMNS 10 > > > let's wait and see. > > > 4) Below appears redundant to me, it was not the case in v1 patch > > set, where it had only one return and one increment of error as new > > added code was at the end of the block:- > > + cstate->num_errors++; > > + return true; > > + } > > cstate->num_errors++; > > > changed per your advice. > > > I was not able to test the v2 due to conflicts in v2, I did attempt to > > resolve but I saw many failures in make world. > > > I get rid of all the SPI code. > > Instead, now I iterate through AttributeRelationId to check if the > error saving table is ok or not, > using DirectFunctionCall3 to do the privilege check. > removed gram.y change, turns out it is not necessary. > and other kinds of refactoring. > > please check attached. Hi! 1) > + switch (attForm->attnum) > + { > + case 1: > + (.....) > + case 2: case 1,2,3 ... Is too random. Other parts of core tend to use `#define Anum_<relname>_<columname> <num>`. Can we follow this style? 2) >+ /* > + * similar to commit a9cf48a > + * (https://postgr.es/m/7bcfc39d4176faf85ab317d0c26786953646a411.camel@cybertec.at) > + * in COPY FROM keep error saving table locks until the transaction end. > + */ I can rarely see other comments referencing commits, and even few referencing a mail archive thread. Can we just write proper comment explaining the reasons? ===== overall Patch design is a little dubious for me. We give users some really incomprehensible API. To use on_error *relation* feature user must create tables with proper schema. Maybe a better design will be to auto-create on_error table if this table does not exist. Thoughts? -- Best regards, Kirill Reshke -
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-12-11T11:41:02Z
Thanks for the v3 patch! Please find review comments on v3:- 1) I think no need to change the below if condition, we can keep it the way it was before i.e with "cstate->opts.on_error != COPY_ON_ERROR_STOP" and we add a new error ereport the way v3 has. Because for cstate->opts.on_error as COPY_ON_ERROR_STOP cases we can avoid two if conditions inside upper if. + if (cstate->num_errors > 0 && cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT) 2) No need for the below "if" check for maxattnum. We can simply increment it with "++maxattnum" and later check if we have exactly 10 attributes for the error table. Because even if we drop any attribute and maxattnum is 10 in pg_attribute for that rel, we should still error out. Maybe we can rename it to "totalatts"? + if (maxattnum <= attForm->attnum) + maxattnum = attForm->attnum; 3) #define would be better, also as mentioned by Kirill switch condition with proper #define would be better. + if (maxattnum != 10) + on_error_tbl_ok = false; 4) > > that would be more work. > so i stick to if there is a table can use to > error saving then use it, otherwise error out. > YES. but that would lead to a better design with an error table. Also, I think Krill mentions the same. That is to auto create, if it does not exist. Regards, Nishant Sharma (EDB). On Tue, Dec 3, 2024 at 3:58 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > On Tue, 3 Dec 2024 at 09:29, jian he <jian.universality@gmail.com> wrote: > > > > On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma > > <nishant.sharma@enterprisedb.com> wrote: > > > > > > Thanks for the v2 patch! > > > > > > I see v1 review comments got addressed in v2 along with some > > > further improvements. > > > > > > 1) v2 Patch again needs re-base. > > > > > > 2) I think we need not worry whether table name is unique or not, > > > table name can be provided by user and we can check if it does > > > not exists then simply we can create it with appropriate columns, > > > if it exists we use it to check if its correct on_error table and > > > proceed. > > > > "simply we can create it with appropriate columns," > > that would be more work. > > so i stick to if there is a table can use to > > error saving then use it, otherwise error out. > > > > > > > > > > 3) Using #define in between the code? I don't see that style in > > > copyfromparse.c file. I do see such style in other src file. So, not > > > sure if committer would allow it or not. > > > #define ERROR_TBL_COLUMNS 10 > > > > > let's wait and see. > > > > > 4) Below appears redundant to me, it was not the case in v1 patch > > > set, where it had only one return and one increment of error as new > > > added code was at the end of the block:- > > > + cstate->num_errors++; > > > + return true; > > > + } > > > cstate->num_errors++; > > > > > changed per your advice. > > > > > I was not able to test the v2 due to conflicts in v2, I did attempt to > > > resolve but I saw many failures in make world. > > > > > I get rid of all the SPI code. > > > > Instead, now I iterate through AttributeRelationId to check if the > > error saving table is ok or not, > > using DirectFunctionCall3 to do the privilege check. > > removed gram.y change, turns out it is not necessary. > > and other kinds of refactoring. > > > > please check attached. > > > Hi! > > 1) > > + switch (attForm->attnum) > > + { > > + case 1: > > + (.....) > > + case 2: > > case 1,2,3 ... Is too random. Other parts of core tend to use `#define > Anum_<relname>_<columname> <num>`. Can we follow this style? > > 2) > >+ /* > > + * similar to commit a9cf48a > > + * ( > https://postgr.es/m/7bcfc39d4176faf85ab317d0c26786953646a411.camel@cybertec.at > ) > > + * in COPY FROM keep error saving table locks until the transaction end. > > + */ > > I can rarely see other comments referencing commits, and even few > referencing a mail archive thread. > Can we just write proper comment explaining the reasons? > > > ===== overall > > Patch design is a little dubious for me. We give users some really > incomprehensible API. To use on_error *relation* feature user must > create tables with proper schema. > Maybe a better design will be to auto-create on_error table if this > table does not exist. > > > Thoughts? > > -- > Best regards, > Kirill Reshke > -
Re: on_error table, saving error info to a table
Andrew Dunstan <andrew@dunslane.net> — 2024-12-11T14:52:19Z
On 2024-12-02 Mo 11:28 PM, jian he wrote: > On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma > <nishant.sharma@enterprisedb.com> wrote: >> Thanks for the v2 patch! >> >> I see v1 review comments got addressed in v2 along with some >> further improvements. >> >> 1) v2 Patch again needs re-base. >> >> 2) I think we need not worry whether table name is unique or not, >> table name can be provided by user and we can check if it does >> not exists then simply we can create it with appropriate columns, >> if it exists we use it to check if its correct on_error table and >> proceed. > "simply we can create it with appropriate columns," > that would be more work. > so i stick to if there is a table can use to > error saving then use it, otherwise error out. > > >> 3) Using #define in between the code? I don't see that style in >> copyfromparse.c file. I do see such style in other src file. So, not >> sure if committer would allow it or not. >> #define ERROR_TBL_COLUMNS 10 >> > let's wait and see. > >> 4) Below appears redundant to me, it was not the case in v1 patch >> set, where it had only one return and one increment of error as new >> added code was at the end of the block:- >> + cstate->num_errors++; >> + return true; >> + } >> cstate->num_errors++; >> > changed per your advice. > >> I was not able to test the v2 due to conflicts in v2, I did attempt to >> resolve but I saw many failures in make world. >> > I get rid of all the SPI code. > > Instead, now I iterate through AttributeRelationId to check if the > error saving table is ok or not, > using DirectFunctionCall3 to do the privilege check. > removed gram.y change, turns out it is not necessary. > and other kinds of refactoring. > > please check attached. I don't have a comment on the patch in general, but wouldn't it be better to use a typed table? Then all you would need to check is the reloftype to make sure it's the right type, instead of checking all the column names and types. Creating the table would be simpler, too. Just CREATE TABLE my_copy_errors OF TYPE pg_copy_error; cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
-
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2024-12-13T08:26:55Z
On Wed, Dec 11, 2024 at 7:41 PM Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: > > Thanks for the v3 patch! > > Please find review comments on v3:- > > 1) I think no need to change the below if condition, we can keep > it the way it was before i.e with > "cstate->opts.on_error != COPY_ON_ERROR_STOP" and we > add a new error ereport the way v3 has. Because for > cstate->opts.on_error as COPY_ON_ERROR_STOP cases we > can avoid two if conditions inside upper if. > > + if (cstate->num_errors > 0 && > cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT) > 2) No need for the below "if" check for maxattnum. We can simply > increment it with "++maxattnum" and later check if we have exactly > 10 attributes for the error table. Because even if we drop any > attribute and maxattnum is 10 in pg_attribute for that rel, we should > still error out. Maybe we can rename it to "totalatts"? > > + if (maxattnum <= attForm->attnum) > + maxattnum = attForm->attnum; > > 3) #define would be better, also as mentioned by Kirill switch > condition with proper #define would be better. > > + if (maxattnum != 10) > + on_error_tbl_ok = false; > > 4) hi. Thanks for the review. The attached v4 patch addressed these two issues. > > that would be more work. > > so i stick to if there is a table can use to > > error saving then use it, otherwise error out. > > > YES. but that would lead to a better design with an error table. > Also, I think Krill mentions the same. That is to auto create, if it > does not exist. > I decided not to auto-create the table. main reason not to do it: 1. utility COPY command with another SPI utility CREATE TABLE command may work. but there is no precedent. 2. if we auto-create the on_error table with BeginCopyFrom. then later we have to use get_relname_relid to get the newly created table Oid, I think it somehow counts as repeating name lookups, see relevant linke [1], [2]. [1] https://postgr.es/m/20240808171351.a9.nmisch@google.com [2] https://postgr.es/m/CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmail.com
-
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-12-16T11:50:24Z
On Fri, Dec 13, 2024 at 1:57 PM jian he <jian.universality@gmail.com> wrote: > On Wed, Dec 11, 2024 at 7:41 PM Nishant Sharma > <nishant.sharma@enterprisedb.com> wrote: > > > > Thanks for the v3 patch! > > > > Please find review comments on v3:- > > > > 1) I think no need to change the below if condition, we can keep > > it the way it was before i.e with > > "cstate->opts.on_error != COPY_ON_ERROR_STOP" and we > > add a new error ereport the way v3 has. Because for > > cstate->opts.on_error as COPY_ON_ERROR_STOP cases we > > can avoid two if conditions inside upper if. > > > > + if (cstate->num_errors > 0 && > > cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT) > > > 2) No need for the below "if" check for maxattnum. We can simply > > increment it with "++maxattnum" and later check if we have exactly > > 10 attributes for the error table. Because even if we drop any > > attribute and maxattnum is 10 in pg_attribute for that rel, we should > > still error out. Maybe we can rename it to "totalatts"? > > > > + if (maxattnum <= attForm->attnum) > > + maxattnum = attForm->attnum; > > > > 3) #define would be better, also as mentioned by Kirill switch > > condition with proper #define would be better. > > > > + if (maxattnum != 10) > > + on_error_tbl_ok = false; > > > > 4) > > hi. Thanks for the review. > The attached v4 patch addressed these two issues. > > > > that would be more work. > > > so i stick to if there is a table can use to > > > error saving then use it, otherwise error out. > > > > > YES. but that would lead to a better design with an error table. > > Also, I think Krill mentions the same. That is to auto create, if it > > does not exist. > > > I decided not to auto-create the table. > main reason not to do it: > 1. utility COPY command with another SPI utility CREATE TABLE command may > work. > but there is no precedent. > > 2. if we auto-create the on_error table with BeginCopyFrom. > then later we have to use get_relname_relid to get the newly created table > Oid, > I think it somehow counts as repeating name lookups, see relevant > linke [1], [2]. > > [1] https://postgr.es/m/20240808171351.a9.nmisch@google.com > [2] > https://postgr.es/m/CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmail.com Thanks for the v4 patch! Review comment on v4:- 1) The new switch logic does not look correct to me. It will pass for a failing scenario. I think you can use v3's logic instead with below changes:- a) while (HeapTupleIsValid(atup = systable_getnext(ascan))) --> while (HeapTupleIsValid(atup = systable_getnext(ascan)) && on_error_tbl_ok) b) attcnt++; --> just before the "switch (attForm->attnum)". Thats it. Also, I think Andrew's suggestion can resolve the concern me and Krill had on forcing users to create tables with correct column names and numbers. Also, will make error table checking simpler. No need for the above kind of checks. Regards, Nishant.
-
Re: on_error table, saving error info to a table
Kirill Reshke <reshkekirill@gmail.com> — 2024-12-17T04:31:46Z
On Mon, 16 Dec 2024 at 16:50, Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: > Also, I think Andrew's suggestion can resolve the concern me and Krill > had on forcing users to create tables with correct column names and > numbers. Also, will make error table checking simpler. No need for the > above kind of checks. +1 on that. -- Best regards, Kirill Reshke
-
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2025-04-25T13:46:39Z
On Mon, Dec 16, 2024 at 7:50 PM Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: > > > 1) The new switch logic does not look correct to me. It will pass for > a failing scenario. I think you can use v3's logic instead with below > changes:- > > a) > while (HeapTupleIsValid(atup = systable_getnext(ascan))) --> > while (HeapTupleIsValid(atup = systable_getnext(ascan)) && on_error_tbl_ok) > > b) > attcnt++; --> just before the "switch (attForm->attnum)". > > Thats it. > You are right about this. On Tue, Dec 17, 2024 at 12:31 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > On Mon, 16 Dec 2024 at 16:50, Nishant Sharma > <nishant.sharma@enterprisedb.com> wrote: > > Also, I think Andrew's suggestion can resolve the concern me and Krill > > had on forcing users to create tables with correct column names and > > numbers. Also, will make error table checking simpler. No need for the > > above kind of checks. > > +1 on that. > Syntax: COPY (on_error table, table error_saving_tbl); seems not ideal. but auto-create on_error table if this table does not exist, seems way more harder. Since we can not use SPI interface here, maybe we can use DefineRelation also, to auto-create a table, what if table already exists, then our operation would be stuck for not COPY related reason. also auto-create means we need to come up with a magic table name for all COPY (on_error table) operations, which seems not ideal IMO. i realized we should error out case like: COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table err_tbl); also by changing copy_generic_opt_arg, now we can COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table x); previously, we can only do COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error 'table', table x);
-
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-05-26T06:30:05Z
> > On Tue, Dec 17, 2024 at 12:31 PM Kirill Reshke <reshkekirill@gmail.com> > wrote: > > > > On Mon, 16 Dec 2024 at 16:50, Nishant Sharma > > <nishant.sharma@enterprisedb.com> wrote: > > > Also, I think Andrew's suggestion can resolve the concern me and Krill > > > had on forcing users to create tables with correct column names and > > > numbers. Also, will make error table checking simpler. No need for the > > > above kind of checks. > > > > +1 on that. > > > > Syntax: COPY (on_error table, table error_saving_tbl); > seems not ideal. > > but auto-create on_error table if this table does not exist, seems way > more harder. > > Since we can not use SPI interface here, maybe we can use DefineRelation > also, to auto-create a table, what if table already exists, then > our operation would be stuck for not COPY related reason. > also auto-create means we need to come up with a magic table name for > all COPY (on_error table) > operations, which seems not ideal IMO. > > i realized we should error out case like: > COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table > err_tbl); > > also by changing copy_generic_opt_arg, now we can > COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table x); > previously, we can only do > COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error 'table', table x); > I am not sure if you understood Andrew's suggestion. As per my understanding he did not suggest auto-creating the error table, he suggested using TYPED TABLES for the error saving table. For example: postgres=# CREATE TYPE error_saving_table_type AS (userid oid, copy_tbl oid, filename text, lineno bigint, line text, colname text, raw_field_value text, err_message text, err_detail text, errorcode text); CREATE TYPE We can have something similar like above in some initdb script, which will help in making the above type kind of derived or standard error saving table type in PG. And then, user can use above TYPE to create error saving table like below: postgres=# CREATE TABLE error_saving_table OF error_saving_table_type; CREATE TABLE After this, user can use error_saving_table with the COPY command like below: COPY t_copy_tbl(a,b) FROM STDIN WITH (DELIMITER ',', on_error table, table error_saving_table); Here's manual example of insert in that table: postgres=# INSERT INTO error_saving_table VALUES (1234, 4321, 'abcd', 12, 'This is was getting copied', 'xyz', 'pqr', 'testing type error table', 'inserting into typed table error saving table', 'test error code'); INSERT 0 1 postgres=# SELECT * from error_saving_table; userid | copy_tbl | filename | lineno | line | colname | raw_field_value | err_message | err_detail | errorcode --------+----------+----------+--------+----------------------------+---------+----------------- +--------------------------+------- ----------------------------------------+----------------- 1234 | 4321 | abcd | 12 | This is was getting copied | xyz | pqr | testing type error table | insert ing into typed table error saving table | test error code (1 row) With the above we don't need to check all the 12 column's count, their data types etc. in the patch. *"Then all you would need to check is the reloftype to* *make **sure it's the right type," *as quoted by Andrew. This will make patch simpler and also will remove burden on users to create error saving tables with correct columns. As its TYPE will be already available by default for the users to create error saving tables. Regards, Nishant. -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2025-08-13T06:33:40Z
On Mon, May 26, 2025 at 2:30 PM Nishant Sharma <nishant.sharma@enterprisedb.com> wrote: >> >> On Tue, Dec 17, 2024 at 12:31 PM Kirill Reshke <reshkekirill@gmail.com> wrote: >> > >> > On Mon, 16 Dec 2024 at 16:50, Nishant Sharma >> > <nishant.sharma@enterprisedb.com> wrote: >> > > Also, I think Andrew's suggestion can resolve the concern me and Krill >> > > had on forcing users to create tables with correct column names and >> > > numbers. Also, will make error table checking simpler. No need for the >> > > above kind of checks. >> > >> > +1 on that. >> > >> >> Syntax: COPY (on_error table, table error_saving_tbl); >> seems not ideal. >> >> but auto-create on_error table if this table does not exist, seems way >> more harder. >> >> Since we can not use SPI interface here, maybe we can use DefineRelation >> also, to auto-create a table, what if table already exists, then >> our operation would be stuck for not COPY related reason. >> also auto-create means we need to come up with a magic table name for >> all COPY (on_error table) >> operations, which seems not ideal IMO. >> >> i realized we should error out case like: >> COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table err_tbl); >> >> also by changing copy_generic_opt_arg, now we can >> COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error table, table x); >> previously, we can only do >> COPY err_tbl FROM STDIN WITH (DELIMITER ',', on_error 'table', table x); > > > I am not sure if you understood Andrew's suggestion. > As per my understanding he did not suggest auto-creating the error table, > he suggested using TYPED TABLES for the error saving table. For example: > > postgres=# CREATE TYPE error_saving_table_type AS (userid oid, copy_tbl oid, > filename text, lineno bigint, line text, colname text, raw_field_value text, > err_message text, err_detail text, errorcode text); > CREATE TYPE > > We can have something similar like above in some initdb script, which will help > in making the above type kind of derived or standard error saving table type in > PG. > > And then, user can use above TYPE to create error saving table like below: > postgres=# CREATE TABLE error_saving_table OF error_saving_table_type; > CREATE TABLE > > After this, user can use error_saving_table with the COPY command like below: > COPY t_copy_tbl(a,b) FROM STDIN WITH (DELIMITER ',', on_error table, > table error_saving_table); > but where "error_saving_table_type" TYPE/TABLE will come from? "error_saving_table_type" either comes from built-in or makes it user defined. Preserving it as a built-in type would require broader consensus, which is likely difficult to achieve. user-defined: for COPY command, we can not use SPI to create another table. also if there is already a user-defined table/type, we still need to check if it meets the condition or not. so overall manually check error saving table meet the criteria or not. rebased patch attached.
-
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2025-10-22T05:15:11Z
hi. The previous discussion mentioned using built-in typed tables for the error saving table. It's doable. first create an build-in composite type in system_functions.sql: CREATE TYPE copy_error_saving AS( userid oid, copy_tbl oid, filename text COLLATE "C", lineno bigint, line text COLLATE "C", colname text COLLATE "C", raw_field_value text COLLATE "C", err_message text COLLATE "C", err_detail text COLLATE "C", errorcode text COLLATE "C" ); then we can use it to create a table like: CREATE TABLE error_saving_table OF copy_error_saving; The downside of this: If the pg_catalog composite (type copy_error_saving) were to change, it could lead to potential compatibility issues. We need to be confident that copy_error_saving definitions are unlikely to occur in the future. For the above type copy_error_saving, I am wondering, do we aslo need add a timestamptz field like "starttime" to indicate COPY beginning time. anyway, please take a look at the attached patch. It introduces a built-in composite type, allowing users to simply use CREATE TABLE x OF copy_error_saving to create a table for storing COPY FROM error-related information. -
Re: on_error table, saving error info to a table
Kirill Reshke <reshkekirill@gmail.com> — 2025-10-28T08:57:55Z
On Wed, 22 Oct 2025 at 10:15, jian he <jian.universality@gmail.com> wrote: > > hi. > > The previous discussion mentioned using built-in typed tables for the > error saving table. > It's doable. > > first create an build-in composite type in system_functions.sql: > CREATE TYPE copy_error_saving AS( > userid oid, > copy_tbl oid, > filename text COLLATE "C", > lineno bigint, > line text COLLATE "C", > colname text COLLATE "C", > raw_field_value text COLLATE "C", > err_message text COLLATE "C", > err_detail text COLLATE "C", > errorcode text COLLATE "C" > ); > > then we can use it to create a table like: > CREATE TABLE error_saving_table OF copy_error_saving; > > The downside of this: > If the pg_catalog composite (type copy_error_saving) were to change, > it could lead to potential compatibility issues. > We need to be confident that copy_error_saving definitions are > unlikely to occur in the future. > > For the above type copy_error_saving, I am wondering, do we aslo need > add a timestamptz field like "starttime" to indicate COPY beginning time. > > anyway, please take a look at the attached patch. > It introduces a built-in composite type, allowing users to simply use > CREATE TABLE x OF copy_error_saving > to create a table for storing COPY FROM error-related information. Hi! I actually wonder if we should change security context to on_err_table owner before > + simple_heap_insert(cstate->error_saving, tuple); I mean, without this, the whole feature looks like a recipe for CVE. I mean, something like inserting invalid rows into a table to make a user, who executes that INSERT to do an 'err_tbl' insert instead, which will execute BEFORE INSERT TRIGGER, which in turn will alter something... Like 'alter user ... with superuser'? I do not have a complete real-life scenario though. -- Best regards, Kirill Reshke
-
Re: on_error table, saving error info to a table
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-07T05:28:06Z
On Wed, Oct 22, 2025 at 10:45 AM jian he <jian.universality@gmail.com> wrote: > hi. > > The previous discussion mentioned using built-in typed tables for the > error saving table. > It's doable. > > first create an build-in composite type in system_functions.sql: > CREATE TYPE copy_error_saving AS( > userid oid, > copy_tbl oid, > filename text COLLATE "C", > lineno bigint, > line text COLLATE "C", > colname text COLLATE "C", > raw_field_value text COLLATE "C", > err_message text COLLATE "C", > err_detail text COLLATE "C", > errorcode text COLLATE "C" > ); > > then we can use it to create a table like: > CREATE TABLE error_saving_table OF copy_error_saving; > Exactly. This is what I was trying to convey about the suggestion by Andrew. Please find review comments on v7:- 1) I think we can improve below, and can avoid 3 conditional check to only one >if (cstate->escontext->error_data->detail == NULL) > err_detail = NULL; >else > err_detail = cstate->escontext->error_data->detail; > >values[j] = err_detail ? CStringGetTextDatum(err_detail) : (Datum) 0; >isnull[j++] = err_detail ? false : true; TO >if (cstate->escontext->error_data->detail == NULL) >{ > values[j] = (Datum) 0; > isnull[j++] = true; >} > else > values[j++] = CStringGetTextDatum(cstate->escontext->error_data->detail); 2) We can have "#define ERROR_TBL_COLUMNS 10" instead of hardcoded 10 in file copyfromparse.c. Because now, it is used only in copyfromparse.c. 3) Do we need a 'j' variable to assign data to values[j]? I see in src code that hard coded numbers are directly used in other places. Like values[0] = data1; values[1] = data2, so on. With this we can avoid j++ execution cycles. Regards, Nishant Sharma. EDB, Pune. https://www.enterprisedb.com/ -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2026-05-06T08:08:33Z
On Wed, Apr 22, 2026 at 10:11 AM jian he <jian.universality@gmail.com> wrote: > > Hi, V8 is attached. > > I've refactored a significant portion of the code. Now the new syntax is: > > COPY FROM (ON_ERROR TABLE, ERROR_TABLE err_tbl); > > We now produce a ModifyTableState and ResultRelInfo node, form the slot, and > then use table_tuple_insert() + ExecInsertIndexTuples() to insert the error > metadata into the ERROR_TABLE. > > This is very similar to the normal ExecInsert() path. Since the ERROR_TABLE is a > user-defined table, we enforce the a lot of restriction, > in CopyFromErrorTableCheck, we have comments like: > + Actually, We can export ExecInsert, construct a ModifyTableContext, and let ExecInsert do the remaining job. This would be neater. V9-0001: Export ExecInsert, ExecSetupTransitionCaptureState, fireBSTriggers, fireASTrigger. v9-0002: Introduce a (ModifyTableContext *) field in CopyFromStateData. Initialize all useful subordinate structures, including such as ModifyTableState, EState, and ModifyTableState->ResultRelInfo. Set cstate->escontext->details_wanted to true to ensure that all input conversion error metadata is captured in cstate->escontext. Use this metadata to construct a TupleTableSlot, and then delegate the remaining processing to ExecInsert. Before calling ExecInsert() to insert error metadata into the ERROR_TABLE, we must first populate a ModifyTableState, similar to how it is done in ExecInitModifyTable. ERROR_TABLE currently cannot be a partitioned table there is no need to call ExecSetupPartitionTupleRouting. ERROR_TABLE cannot be a foreign table, ExecInitModifyTable foreign table related initialization handling is not needed. ERROR_TABLE does not support Row-Level Security (RLS), we do not need to handle ModifyTable->withCheckOptionLists. For the INSERT code path, we also do not need to worry about EvalPlanQual. By comparing side-by-side with ExecInitModifyTable, it is ok to populate the necessary information for the ERROR_TABLE using the following functions: ExecInitRangeTable, ExecInitResultRelation, ExecOpenIndices, and CheckValidResultRel. This will allow the ERROR_TABLE to safely use ExecInsert(). -- jian https://www.enterprisedb.com/
-
Re: on_error table, saving error info to a table
solaimurugan vellaipandiyan <drsolaimurugan.v@gmail.com> — 2026-05-07T11:32:56Z
Hi, I tested the v9 patch series on current PostgreSQL HEAD. The patches did not apply cleanly on the latest tree because of a small conflict in nodeModifyTable.c, but after resolving it manually, the build and installation completed successfully. I initialized a fresh cluster and verified that the built-in composite type copy_error_saving is created correctly during initdb. I tested the feature using the following setup: CREATE TABLE err_tbl OF copy_error_saving; CREATE TABLE t(a int, b int, c int); COPY t FROM STDIN WITH ( FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl ); Input used: 1,2,3 4,5,x 7,8,9 From my testing: - valid rows were inserted into the target table correctly - malformed rows were stored in ERROR_TABLE as expected - COPY continued processing the remaining rows successfully The latest executor-based approach using ExecInsert() looks cleaner and easier to follow compared to the earlier direct insertion approach. I also feel the built-in composite type approach makes the ERROR_TABLE validation simpler and more maintainable compared to manually checking all columns and types. Regards, Solaimurugan -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2026-05-11T02:06:23Z
On Wed, May 6, 2026 at 4:08 PM jian he <jian.universality@gmail.com> wrote: > > v9-0002: Introduce a (ModifyTableContext *) field in CopyFromStateData. > Initialize all useful subordinate structures, including such as > ModifyTableState, EState, and ModifyTableState->ResultRelInfo. Set > cstate->escontext->details_wanted to true to ensure that all input conversion > error metadata is captured in cstate->escontext. Use this metadata to construct > a TupleTableSlot, and then delegate the remaining processing to ExecInsert. > > Before calling ExecInsert() to insert error metadata into the ERROR_TABLE, we > must first populate a ModifyTableState, similar to how it is done in > ExecInitModifyTable. > > By comparing side-by-side with ExecInitModifyTable, it is ok to populate the > necessary information for the ERROR_TABLE using the following functions: > ExecInitRangeTable, ExecInitResultRelation, ExecOpenIndices, and > CheckValidResultRel. > This will allow the ERROR_TABLE to safely use ExecInsert(). > example CREATE TABLE t(a int); INSERT INTO t VALUES (1); Execution flow: ProcessQuery: queryDesc = CreateQueryDesc(plan, sourceText, GetActiveSnapshot(), InvalidSnapshot, dest, params, queryEnv, 0); CreateQueryDesc: qd->snapshot = RegisterSnapshot(snapshot); standard_ExecutorStart: estate->es_snapshot = RegisterSnapshot(queryDesc->snapshot); For COPY FROM, PortalRunUtility -> ProcessUtility -> standard_ProcessUtility -> DoCopy ensures that an ActiveSnapshot is available through PortalRunUtility()->PlannedStmtRequiresSnapshot(). Therefore, I think it is OK for the CopyFrom function to register a active snapshot (EState->es_snapshot) during constructing the EState, in case snapshot access is needed later. EState *estate = CreateExecutorState(); /* for ExecConstraints() */ Add: estate->es_snapshot = RegisterSnapshot(GetActiveSnapshot()); estate->es_crosscheck_snapshot = RegisterSnapshot(InvalidSnapshot); Similarly, it would be better let CopyFromStateData->ModifyTableContext->EState->es_snapshot also Register the current ActiveSnapshot. Just in case the ExecInsert function needs to use it. In the attached v10, the code flow for inserting content into the error-saving table involves constructing an EState, while the population and release of its subfields align more closely with how ExecutorStart, ExecutorFinish, and ExecutorEnd deal with EState. -- jian https://www.enterprisedb.com/ -
Re: on_error table, saving error info to a table
solaimurugan vellaipandiyan <drsolaimurugan.v@gmail.com> — 2026-05-11T07:31:26Z
Hi, I tested the v10 patch series further on current PostgreSQL HEAD. The patches applied successfully on my side, and the build/install completed without issues. I initialized a fresh cluster and verified that the built-in composite type copy_error_saving is created correctly during initdb. I re-tested the COPY functionality using malformed input data and confirmed that valid rows continue loading successfully while invalid rows are redirected into ERROR_TABLE with detailed metadata. I also tried a few additional edge cases. The self-reference protection works correctly. The following case correctly errors out: COPY err_tbl FROM STDIN WITH ( ON_ERROR table, ERROR_TABLE err_tbl ); with: cannot use relation "err_tbl" for COPY error saving while copying data to it I additionally tested ERROR_TABLE behavior with a BEFORE INSERT trigger attached to err_tbl. >From my testing, COPY still succeeds and malformed rows continue to be inserted into err_tbl even when triggers are present on the ERROR_TABLE. The trigger function also appears to execute normally during error row insertion. For example, using a trigger function with RAISE NOTICE produced: NOTICE: trigger fired for each malformed row redirected into err_tbl. Since the patch comments mention restrictions around triggers for ERROR_TABLE, I was not sure whether this current behavior is intentional or whether the trigger restriction checks are still incomplete. Overall, the executor-based integration in v10 feels cleaner and easier to follow compared to the earlier versions. Regards, Solaimurugan
-
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2026-05-12T07:06:46Z
On Mon, May 11, 2026 at 3:32 PM solaimurugan vellaipandiyan <drsolaimurugan.v@gmail.com> wrote: > > I additionally tested ERROR_TABLE behavior with a BEFORE INSERT > trigger attached to err_tbl. > From my testing, COPY still succeeds and malformed rows continue to be > inserted into err_tbl even when triggers are present on the > ERROR_TABLE. The trigger function also appears to execute normally > during error row insertion. For example, using a trigger function with > RAISE NOTICE produced: > > NOTICE: trigger fired > for each malformed row redirected into err_tbl. > Since the patch comments mention restrictions around triggers for > ERROR_TABLE, I was not sure whether this current behavior is > intentional or whether the trigger restriction checks are still > incomplete. With v11, I changed the behavior to: Statement-level triggers on the ERROR_TABLE are fired unconditionally, regardless of whether an error occurred or not. Each row inserted into the ERROR_TABLE will fire both the BEFORE INSERT FOR EACH ROW and AFTER INSERT FOR EACH ROW triggers.
-
Re: on_error table, saving error info to a table
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-14T17:14:29Z
Hello! + cstate->error_rel = table_open(err_relOid, NoLock); + ... and shortly after ... + + table_close(cstate->error_rel, NoLock); Bot this is still used by many other calls after closing. See the following example: SET debug_discard_caches = 1; CREATE TABLE src_tbl (a int, b int); CREATE TABLE err_tbl OF copy_error_saving; CREATE INDEX src_idx ON src_tbl(a); COPY src_tbl FROM STDIN (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl); 1,2 xx,3 3,4 \. -- ERROR: relation with OID 0 does not exist + /* Handle queued AFTER triggers */ + AfterTriggerEndQuery(cstate->mtcontext->estate); Is the order of this correct? See the following snippet that crashes the server: CREATE TABLE target_tbl (id int, val int); CREATE TABLE err_tbl OF copy_error_saving; CREATE OR REPLACE FUNCTION err_stmt_trans_fn() RETURNS trigger AS $$ BEGIN END; $$ LANGUAGE plpgsql; CREATE TRIGGER err_stmt_trans AFTER INSERT ON err_tbl REFERENCING NEW TABLE AS new_rows FOR EACH STATEMENT EXECUTE FUNCTION err_stmt_trans_fn(); \echo === COPY === COPY target_tbl FROM stdin WITH (on_error 'table', error_table 'err_tbl'); 1 100 bad 200 3 notanumber 4 400 \. + + cstate->num_errors = cstate->num_errors + estate->es_processed; Counting seems to miss if a before trigger returns null: \set ON_ERROR_STOP 0 CREATE TABLE t2 (a int, b int, c int); CREATE TABLE err_tbl2 OF copy_error_saving; CREATE FUNCTION drop_all() RETURNS TRIGGER LANGUAGE plpgsql AS $$ BEGIN RETURN NULL; END; $$; CREATE TRIGGER drop_all_t BEFORE INSERT ON err_tbl2 FOR EACH ROW EXECUTE FUNCTION drop_all(); COPY t2 FROM STDIN WITH (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl2); 1,2,a 3,4,b 5,6,c 7,8,d 9,10,e \. SELECT count(*) AS n FROM t2; SELECT count(*) AS n FROM err_tbl2; + typoid = GetSysCacheOid2(TYPENAMENSP, Anum_pg_type_oid, + PointerGetDatum("copy_error_saving"), + ObjectIdGetDatum(PG_CATALOG_NAMESPACE)); ... + if (reloftype != typoid) + ereport(ERROR, ... + errhint("The COPY error saving table must be a typed table based on type \"%s\".", + format_type_be_qualified(typoid))); Isn't an if (!OidIsValid(typoid)) check missing between the two? -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2026-05-15T09:05:47Z
On Fri, May 15, 2026 at 1:14 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > SET debug_discard_caches = 1; > CREATE TABLE src_tbl (a int, b int); > CREATE TABLE err_tbl OF copy_error_saving; > CREATE INDEX src_idx ON src_tbl(a); > COPY src_tbl FROM STDIN (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl); > 1,2 > xx,3 > 3,4 > \. > -- ERROR: relation with OID 0 does not exist > Yech. We need do this in EndCopyFrom + if (cstate->error_rel) + table_close(cstate->error_rel, NoLock); > > + /* Handle queued AFTER triggers */ > + AfterTriggerEndQuery(cstate->mtcontext->estate); > > Is the order of this correct? See the following snippet that crashes the server: > > CREATE TABLE target_tbl (id int, val int); > CREATE TABLE err_tbl OF copy_error_saving; > > CREATE OR REPLACE FUNCTION err_stmt_trans_fn() RETURNS trigger AS $$ > BEGIN > END; > $$ LANGUAGE plpgsql; > > CREATE TRIGGER err_stmt_trans > AFTER INSERT ON err_tbl > REFERENCING NEW TABLE AS new_rows > FOR EACH STATEMENT EXECUTE FUNCTION err_stmt_trans_fn(); > > \echo === COPY === > COPY target_tbl FROM stdin WITH (on_error 'table', error_table 'err_tbl'); > 1 100 > bad 200 > 3 notanumber > 4 400 > \. > We need do AfterTriggerEndQuery(cstate->mtcontext->estate); first then AfterTriggerEndQuery(estate); ExecForPortionOfLeftovers() handles this in the same way. ``static AfterTriggersData afterTriggers;`` and ExecASInsertTriggers, AfterTriggerEndQuery will change the value of afterTriggers constantly, therefore, the position of these functions is important! Therefore in copyfrom.c the order should be: ExecBSInsertTriggers(estate, resultRelInfo); ExecBSInsertTriggers(cstate->mtcontext->mtstate->ps.state, cstate->mtcontext->mtstate->rootResultRelInfo); ExecASInsertTriggers(cstate->mtcontext->estate, on_error_mtstate->rootResultRelInfo, on_error_mtstate->mt_transition_capture); ExecASInsertTriggers(estate, target_resultRelInfo, cstate->transition_capture); > + > + cstate->num_errors = cstate->num_errors + estate->es_processed; > > Counting seems to miss if a before trigger returns null: > > \set ON_ERROR_STOP 0 > CREATE TABLE t2 (a int, b int, c int); > CREATE TABLE err_tbl2 OF copy_error_saving; > CREATE FUNCTION drop_all() RETURNS TRIGGER LANGUAGE plpgsql AS $$ > BEGIN > RETURN NULL; > END; > $$; > CREATE TRIGGER drop_all_t BEFORE INSERT ON err_tbl2 FOR EACH ROW > EXECUTE FUNCTION drop_all(); > COPY t2 FROM STDIN WITH (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl2); > 1,2,a > 3,4,b > 5,6,c > 7,8,d > 9,10,e > \. > > SELECT count(*) AS n FROM t2; > SELECT count(*) AS n FROM err_tbl2; > I am not sure. Should we produce the NOTICE message below for the above test case? +NOTICE: 5 rows were saved to table "err_tbl2" due to data type incompatibility but because of the trigger, err_tbl2 has zero rows. > > + typoid = GetSysCacheOid2(TYPENAMENSP, Anum_pg_type_oid, > + PointerGetDatum("copy_error_saving"), > + ObjectIdGetDatum(PG_CATALOG_NAMESPACE)); > ... > + if (reloftype != typoid) > + ereport(ERROR, > ... > + errhint("The COPY error saving table must be a > typed table based on type \"%s\".", > + format_type_be_qualified(typoid))); > > Isn't an if (!OidIsValid(typoid)) check missing between the two? Sure, I have added a + if (!OidIsValid(typoid)) + elog(ERROR, "cache lookup failed for catalog type %s", "copy_error_saving"); -- jian https://www.enterprisedb.com/ -
Re: on_error table, saving error info to a table
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-15T11:32:49Z
> I am not sure. > Should we produce the NOTICE message below for the above test case? > +NOTICE: 5 rows were saved to table "err_tbl2" due to data type incompatibility > but because of the trigger, err_tbl2 has zero rows. I am also not sure about the exact details, however silently dropping rows from both the target and error table seems wrong to me, so I would definitely add some output about this happening. In my example I used a trigger that always returns null, but it is also possible that we deal with something that only returns null sometimes. And then we get an output like: NOTICE: 1 row was saved to table "err_tbl" due to data type incompatibility COPY 2 With an input or 4 or 5 rows. + else if (cstate->opts.on_error == COPY_ON_ERROR_TABLE) + ereport(NOTICE, + errmsg("saving error information to table \"%s\" row due to data type incompatibility at line %" PRIu64 " for column \"%s\": \"%s\"", + RelationGetRelationName(cstate->error_rel), + cstate->cur_lineno, + cstate->cur_attname, + attval)); Also this notice is emitted even if the before trigger returns null. The wording suggest that it is in progress ("saving", not "saved"), but then it can be still confusing to see a notice about this and then no result. I would also add documentation about how triggers work on the error table, for example (silently part is current behavior - if that changes, the documentation should too): Statement-level triggers (BEFORE/AFTER INSERT FOR EACH STATEMENT) on `error_saving_table` are fired on every COPY FROM that specifies this table, regardless of whether any errors occurred. Row-level triggers fire once per error row. A BEFORE INSERT FOR EACH ROW trigger that returns NULL silently drops the audit row. + /* + * Copy other important information into the EState, this aligned with + * ExecutorStart + */ + estate->es_snapshot = RegisterSnapshot(GetActiveSnapshot()); + estate->es_crosscheck_snapshot = RegisterSnapshot(InvalidSnapshot); + Maybe I'm missing something, but is this required for the patch? + /* Must have INSERT privilege on the table */ + aclresult = pg_class_aclcheck(RelationGetRelid(rel), + GetUserId(), + ACL_INSER ... + + ExecCheckPermissions(pstate->p_rtable, list_make1(perminfo), true); Isn't the first redundant with ExecCheckPermissions? + if (opts_out->reject_limit) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot set option %s when %s is specified as \"%s\"", "REJECT_LIMIT", "ON_ERROR", "TABLE")); Isn't this check repeated later with a different error message? "COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE" + /* Prepare to build the result tuple */ + TupleTableSlot *myslot = ExecGetReturningSlot(estate, + mtstate->resultRelInfo); Is reusing the returning slot for this the proper way to do it? I'm not saying that it's wrong, but I'm unsure about this. + /* TODO: Support cstate->error_rel when it is a partitioned table */ Is this todo relevant? Isn't this code unreachable with partitioned tables? There are also a few typos: realtion => relation resouces => resources vertified => verified -
Re: on_error table, saving error info to a table
jian he <jian.universality@gmail.com> — 2026-05-25T08:13:44Z
On Fri, May 15, 2026 at 7:32 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > > I am not sure. > > Should we produce the NOTICE message below for the above test case? > > +NOTICE: 5 rows were saved to table "err_tbl2" due to data type incompatibility > > but because of the trigger, err_tbl2 has zero rows. > > I am also not sure about the exact details, however silently dropping > rows from both the target and error table seems wrong to me, so I > would definitely add some output about this happening. > With v13, I have added: + <para> + Triggers on <replaceable class="parameter">error_saving_table</replaceable> + will be fired. Therefore, the <literal>NOTICE</literal> message regarding + rows inserted into <replaceable class="parameter">error_saving_table</replaceable> + may differ from what is actually being inserted. + </para> > + /* Prepare to build the result tuple */ > + TupleTableSlot *myslot = ExecGetReturningSlot(estate, > + mtstate->resultRelInfo); > > > Is reusing the returning slot for this the proper way to do it? I'm > not saying that it's wrong, but I'm unsure about this. Searching the codebase for ExecGetReturningSlot or ri_ReturningSlot shows that this is its only call site for CopyFromState->error_rel (Note that in ExecInsert, resultRelInfo->ri_projectReturning is NULL for the CopyFromState->error_rel). In ExecInsert, we also have these comments: --------- * Using ExecGetReturningSlot() to store the tuple for the * recheck isn't that pretty, but we can't trivially use * the input slot, because it might not be of a compatible * type. As there's no conflicting usage of * ExecGetReturningSlot() in the DO NOTHING case... --------- Therefore I think it's safe to reuse ResultRelInfo->ri_ReturningSlot. Regarding the trigger behavior, with v13: Statement-level and row-level triggers will fire for every error record insertion, which behaves very similarly to ExecForPortionOfLeftovers. I have attached version 13, which should resolve the rest of the issues you raised. -- jian https://www.enterprisedb.com/
-
Re: on_error table, saving error info to a table
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-28T22:41:11Z
Generally looks good to me, I only found a few typos: + errmsg("saving error information to table \"%s\" row due to data type incompatibility at line %" PRIu64 " for column \"%s\": \"%s\"", Is row needed there? + * TODO: Allow cstate->error_rel to be a partitioned table. This should be + * not difficult, but requires proper handling of constraints and triggers should not be difficult + privileges on it. During the error records inseration, + <literal>NOT NULL</literal> and <literal>CHECK</literal> constraints are enforced, + and both row-level and statement-level triggers will be fired. record's insertion Maybe this could explicitly mention that failure to insert into the error table will fail the copy statement? Or some better wording of that, as it is allowed behavior with triggers.