Re: on_error table, saving error info to a table

On 2024-12-02 Mo 11:28 PM, jian he wrote:
> On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma
> <nishant(dot)sharma(at)enterprisedb(dot)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