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

Hello,

On Thu, Jul 20, 2023 at 4:06 PM Zhang Mingli <zmlpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Jul 7, 2023 at 18:00 +0800, Damir Belyalov <dam(dot)bel07(at)gmail(dot)com>, wrote:
>
>
> V2 patch still have some errors when apply file doc/src/sgml/ref/copy.sgml, rebased and fixed it in V3 path.
> Thanks a lot for review.
>
> I have updated https://commitfest.postgresql.org/43/3896/ to staus Ready for Committer, thanks again.

I've looked at this patch and it looks mostly fine, though I do not
intend to commit it myself; perhaps Andrew will.

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

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

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

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com