HI, On Dec 27, 2022, 19:02 +0800, Melih Mutlu <m.melihmu...@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? Regards, Zhang Mingli