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

On 2022-08-01 Mo 09:56, Zhang Mingli wrote:

Hi, 

The previous discussion is:

/messages/by-id/CACJufxEnVqzOFtqhexF2+AwOKFrV8zHOY3y=p+gPK6eB14pn_w@mail.gmail.com

Starting a new thread is pointless and annoying. As I said in the
previous thread, we would need a patch.

cheers

andrew

--

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

On 2022-08-01 Mo 15:50, Andrew Dunstan wrote:

On 2022-08-01 Mo 09:56, Zhang Mingli wrote:

Hi, 

The previous discussion is:

/messages/by-id/CACJufxEnVqzOFtqhexF2+AwOKFrV8zHOY3y=p+gPK6eB14pn_w@mail.gmail.com

Starting a new thread is pointless and annoying. As I said in the
previous thread, we would need a patch.

Apologies, I se you have sent a patch. I will check it out.

cheers

andrew

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

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);

Thanks,
--
Melih Mutlu
Microsoft

HI,

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?

Regards,
Zhang Mingli

Hello!

The patch does not work for the current version of postgres, it needs to be
updated.
I tested your patch. Everything looks simple and works well.

There is a suggestion to simplify the code: instead of using

if (cstate->opts.force_notnull_all)
{
int i;
for(i = 0; i < num_phys_attrs; i++)
cstate->opt.force_notnull_flags[i] = true;
}

you can use MemSet():

if (cstate->opts.force_notnull_all)
MemSet(cstate->opt.force_notnull_flags, true, num_phys_attrs *
sizeof(bool));

The same for the force_null case.

Regards,
Damir Belyalov,
Postgres Professional

HI,

Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov <dam.bel07@gmail.com>, wrote:

The patch does not work for the current version of postgres, it needs to be updated.
I tested your patch. Everything looks simple and works well.

There is a suggestion to simplify the code: instead of using

if (cstate->opts.force_notnull_all)
{
int i;
for(i = 0; i < num_phys_attrs; i++)
cstate->opt.force_notnull_flags[i] = true;
}

Thanks very much for review.

Nice suggestion, patch rebased and updated.

Hi,

On Jul 9, 2023 at 11:51 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
HI,

Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov <dam.bel07@gmail.com>, wrote:

The patch does not work for the current version of postgres, it needs to be updated.
I tested your patch. Everything looks simple and works well.

There is a suggestion to simplify the code: instead of using

if (cstate->opts.force_notnull_all)
{
int i;
for(i = 0; i < num_phys_attrs; i++)
cstate->opt.force_notnull_flags[i] = true;
}

Thanks very much for review.

Nice suggestion, patch rebased and updated.

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.

Zhang Mingli

www.hashdata.xyz

Hi,

On Jul 7, 2023 at 18:00 +0800, Damir Belyalov <dam.bel07@gmail.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.

Zhang Mingli
www.hashdata.xyz

Hello,

On Thu, Jul 20, 2023 at 4:06 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:

Hi,

On Jul 7, 2023 at 18:00 +0800, Damir Belyalov <dam.bel07@gmail.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

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.

Zhang Mingli
https://www.hashdata.xyz

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

On Aug 1, 2023, at 03:35, Andrew Dunstan <andrew@dunslane.net> wrote:

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.

HI, Andrew

Patch rebased and updated like above, thanks.


Zhang Mingli
https://www.hashdata.xyz

On 2023-07-31 Mo 20:46, Zhang Mingli wrote:

On Aug 1, 2023, at 03:35, Andrew Dunstan <andrew@dunslane.net> wrote:

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.

HI, Andrew

Patch rebased and updated like above, thanks.

Pushed at last, thanks.

cheers

andrew

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