Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns

On 2023-07-26 We 03:03, Zhang Mingli wrote:
> HI,
>
>> I've looked at this patch and it looks mostly fine, though I do not
>> intend to commit it myself; perhaps Andrew will.
>
> HI, Amit, thanks for review.
>
>>
>> A few minor things to improve:
>>
>> +      If <literal>*</literal> is specified, it will be applied in
>> all columns.
>> ...
>> +      If <literal>*</literal> is specified, it will be applied in
>> all columns.
>>
>> Please write "it will be applied in" as "the option will be applied to".
>
> +1
>
>>
>> +   bool        force_notnull_all;  /* FORCE_NOT_NULL * */
>> ...
>> +   bool        force_null_all;     /* FORCE_NULL * */
>>
>> Like in the comment for force_quote, please add a "?" after * in the
>> above comments.
>
> +1
>
>>
>> +   if (cstate->opts.force_notnull_all)
>> +       MemSet(cstate->opts.force_notnull_flags, true, num_phys_attrs
>> * sizeof(bool));
>> ...
>> +   if (cstate->opts.force_null_all)
>> +       MemSet(cstate->opts.force_null_flags, true, num_phys_attrs *
>> sizeof(bool));
>>
>> While I am not especially opposed to using this 1-line variant to set
>> the flags array, it does mean that there are now different styles
>> being used for similar code, because force_quote_flags uses a for
>> loop:
>>
>>    if (cstate->opts.force_quote_all)
>>    {
>>        int         i;
>>
>>        for (i = 0; i < num_phys_attrs; i++)
>>            cstate->opts.force_quote_flags[i] = true;
>>    }
>>
>> Perhaps we could fix the inconsistency by changing the force_quote_all
>> code to use MemSet() too.  I'll defer whether to do that to Andrew's
>> judgement.
>
> Sure, let’s wait for Andrew and I will put everything in one pot then.
>
>

I was hoping it be able to get to it today but that's not happening. If
you want to submit a revised patch as above that will be good. I hope to
get to it later this week.

cheers

andrew

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