Thread: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
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?
Attachment
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
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
+ if (cstate->opts.force_notnull_all)
+ {
+ int i;
+ for(i = 0; i < num_phys_attrs; i++)
+ cstate->opts.force_notnull_flags[i] = true;
+ }
if (cstate->opts.force_null_all)
attnums = cstate->attnumlist;
else
attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
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?
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.
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.
Attachment
On Jul 9, 2023 at 11:51 +0800, Zhang Mingli <zmlpostgres@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.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.
Attachment
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.
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
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.
https://www.hashdata.xyz
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.
Attachment
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, AndrewPatch rebased and updated like above, thanks.
Pushed at last, thanks.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com