Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
On 2023-07-31 Mo 20:46, Zhang Mingli wrote: On Aug 1, 2023, at 03:35, Andrew Dunstan 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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
On Aug 1, 2023, at 03:35, Andrew Dunstan 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. v4-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch Description: Binary data Zhang Minglihttps://www.hashdata.xyz
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 * is specified, it will be applied in
all columns.
...
+ If * 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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
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 * is specified, it will be applied in all
> columns.
> ...
> + If * is specified, it will be applied in all
> columns.
>
> Please write "it will be applied in" as "the option will be applied to".
+1
>
> + boolforce_notnull_all; /* FORCE_NOT_NULL * */
> ...
> + boolforce_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
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 wrote:
>
> Hi,
>
> On Jul 7, 2023 at 18:00 +0800, Damir Belyalov , 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 * is specified, it will be applied in all columns.
...
+ If * is specified, it will be applied in all columns.
Please write "it will be applied in" as "the option will be applied to".
+ boolforce_notnull_all; /* FORCE_NOT_NULL * */
...
+ boolforce_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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi, On Jul 7, 2023 at 18:00 +0800, Damir Belyalov , 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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi,
On Jul 9, 2023 at 11:51 +0800, Zhang Mingli , wrote:
HI,
Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov , 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
>
v3-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch
Description: Binary data
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
HI,
Regards,
Zhang Mingli
On Jul 7, 2023, 18:00 +0800, Damir Belyalov , 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-0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-co.patch
Description: Binary data
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
HI,
On Dec 27, 2022, 19:02 +0800, Melih Mutlu , 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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
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
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
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: >> >> https://www.postgresql.org/message-id/CACJufxEnVqzOFtqhexF2%2BAwOKFrV8zHOY3y%3Dp%2BgPK6eB14pn_w%40mail.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
Re: [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: > > https://www.postgresql.org/message-id/CACJufxEnVqzOFtqhexF2%2BAwOKFrV8zHOY3y%3Dp%2BgPK6eB14pn_w%40mail.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
[feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hi, The previous discussion is: https://www.postgresql.org/message-id/CACJufxEnVqzOFtqhexF2%2BAwOKFrV8zHOY3y%3Dp%2BgPK6eB14pn_w%40mail.gmail.com We have FORCE_NULL/FORCE_NOT_NULL options when COPY FROM, but users must set the columns one by one. CREATE TABLE forcetest ( a INT NOT NULL, b TEXT NOT NULL, c TEXT, d TEXT, e TEXT ); \pset null NULL BEGIN; COPY forcetest (a, b, c, d) FROM STDIN WITH (FORMAT csv, FORCE_NOT_NULL(c,d), FORCE_NULL(c,d)); 1,'a',,"" \. COMMIT; SELECT c, d FROM forcetest WHERE a = 1; c | d ---+-- | NULL (1 row) We don’t have FORCE_NULL * or FORCE_NOT_NULL * for all columns of a table like FORCE_QUOTE *. They should be helpful if a table have many columns. This patch enables FORCE_NULL/FORCE_NOT_NULL options to select all columns of a table just like FORCE_QUOTE * (quote all columns). BEGIN COPY forcetest (a, b, c, d) FROM STDIN WITH (FORMAT csv, FORCE_NOT_NULL *, FORCE_NULL *); 2,'b',,"" \. COMMIT; SELECT c, d FROM forcetest WHERE a = 2; c | d ---+-- | NULL (1 row) Any thoughts? Regards, Zhang Mingli v0001-COPY-FROM-enable-FORCE_NULL-FORCE_NOT_NULL-on-all-columns.patch Description: Binary data
