On Dec 27, 2022, 19:02 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
Hi,
Having FORCE_NULL(*) and FORCE_NOT_NULL(*) sounds good, since postgres already has FORCE_QUOTE(*).
I just quickly tried out your patch. It worked for me as expected.
One little suggestion:
+ if (cstate->opts.force_notnull_all)
+ {
+ int i;
+ for(i = 0; i < num_phys_attrs; i++)
+ cstate->opts.force_notnull_flags[i] = true;
+ }
Instead of setting force_null/force_notnull flags for all columns, what about simply setting "attnums" list to cstate->attnumlist?
Something like the following should be enough :
if (cstate->opts.force_null_all)
attnums = cstate->attnumlist;
else
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
Tanks very much for review.
I got your point and we have to handle the case that there are no force_* options at all.
So the codes will be like:
```
List *attnums = NIL;
if (cstate->opts.force_notnull_all)
attnums = cstate->attnumlist;
else if (cstate->opts.force_notnull)
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
if (attnums != NIL)
{
// process force_notnull columns
attnums = NIL; // to process other options later
}
if (cstate->opts.force_null_all)
attnums = cstate->attnumlist;
else if (cstate->opts.force_null)
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
if (attnums != NIL)
{
// process force_null columns
attnums = NIL; // to process other options later
}
```
That seems a little odd.
Or, we could keep attnums as local variables, then the codes will be like:
```
if (cstate->opts.force_notnull_all || cstate->opts.force_notnull)
{
if (cstate->opts.force_notnull_all)
attnums = cstate->attnumlist;
else
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
// process force_notnull columns
}
```
Any other suggestions?