Thread: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

[Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Zhang Mingli
Date:
Hi,


FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.

And copyto.c and copyfrom.c are split in this commit https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df .

There is no need to handle these options when COPY TO.


Regards,
Zhang Mingli



Attachment

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Kyotaro Horiguchi
Date:
At Wed, 27 Jul 2022 00:16:33 +0800, Zhang Mingli <zmlpostgres@gmail.com> wrote in 
> FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.
> 
> And copyto.c and copyfrom.c are split in this commit
https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df
<https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df>.
 
> 
> There is no need to handle these options when COPY TO.

Agreed.

ProcessCopyOptions previously rejects force_quote_all for COPY FROM
and copyfrom.c is not even conscious of the option (that is, even no
assertion on it). The two options are rejected for COPY TO by the same
function so it seems like a thinko of the commit. Getting rid of the
code would be good in the view of code coverage and maintenance.

On the otherhand I wonder if it is good that we have assertions on the
option values.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




On Wed, Jul 27, 2022 at 12:55 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
ProcessCopyOptions previously rejects force_quote_all for COPY FROM
and copyfrom.c is not even conscious of the option (that is, even no
assertion on it). The two options are rejected for COPY TO by the same
function so it seems like a thinko of the commit. Getting rid of the
code would be good in the view of code coverage and maintenance.

Yeah, ProcessCopyOptions() does have the check for force_notnull and
force_null whether it is using COPY FROM and whether it is in CSV mode.
So the codes in copyto.c processing force_notnull/force_null are
actually dead codes. 
 
On the otherhand I wonder if it is good that we have assertions on the
option values.

Agree. Assertions would be better.

Thanks
Richard
Hi, all

Assertions added. 

Thanks for review.

Regards,

Zhang Mingli

Sent with a Spark
On Jul 27, 2022, 14:37 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:

On Wed, Jul 27, 2022 at 12:55 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
ProcessCopyOptions previously rejects force_quote_all for COPY FROM
and copyfrom.c is not even conscious of the option (that is, even no
assertion on it). The two options are rejected for COPY TO by the same
function so it seems like a thinko of the commit. Getting rid of the
code would be good in the view of code coverage and maintenance.

Yeah, ProcessCopyOptions() does have the check for force_notnull and
force_null whether it is using COPY FROM and whether it is in CSV mode.
So the codes in copyto.c processing force_notnull/force_null are
actually dead codes. 
 
On the otherhand I wonder if it is good that we have assertions on the
option values.

Agree. Assertions would be better.

Thanks
Richard
Attachment

On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
Assertions added. 

Can we also add assertions to make sure force_quote, force_notnull and
force_null are available only in CSV mode?

Thanks
Richard

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Zhang Mingli
Date:
HI,

More assertions added.

Thanks.

Regards,
Zhang Mingli
On Jul 29, 2022, 11:24 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:

On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
Assertions added. 

Can we also add assertions to make sure force_quote, force_notnull and
force_null are available only in CSV mode?

Thanks
Richard
Attachment

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Kyotaro Horiguchi
Date:
At Mon, 1 Aug 2022 09:59:49 +0800, Zhang Mingli <zmlpostgres@gmail.com> wrote in 
> On Jul 29, 2022, 11:24 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:
> >
> > > On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
> > > > Assertions added.
> > >
> > > Can we also add assertions to make sure force_quote, force_notnull and
> > > force_null are available only in CSV mode?
> 
> More assertions added.

An empty List is not NULL but NIL (which values are identical,
though).  There are some other option combinations that are rejected
by ProcessCopyOptions.  On the other hand *re*checking all
combinations that the function should have rejected is kind of silly.
Addition to that, I doubt the assertions are really needed even though
the wrong values don't lead to any serious consequence.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Zhang Mingli
Date:

Regards,
Zhang Mingli
On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, wrote:
An empty List is not NULL but NIL (which values are identical,
though). 
Thanks for pointing that out. Fix it in new patch.
There are some other option combinations that are rejected
by ProcessCopyOptions. On the other hand *re*checking all
combinations that the function should have rejected is kind of silly.
Addition to that, I doubt the assertions are really needed even though
the wrong values don't lead to any serious consequence.

Agree. 
ProcessCopyOptions has rejected all invalid combinations and assertions are optional.
regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Michael Paquier
Date:
On Tue, Aug 02, 2022 at 04:13:30PM +0800, Zhang Mingli wrote:
> On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, wrote:
>> There are some other option combinations that are rejected
>> by ProcessCopyOptions. On the other hand *re*checking all
>> combinations that the function should have rejected is kind of silly.
>> Addition to that, I doubt the assertions are really needed even though
>> the wrong values don't lead to any serious consequence.
>
> ProcessCopyOptions has rejected all invalid combinations and assertions are optional.

I agree with Horiguchi-san's point here: there is no real point in
having these assertions, especially just after the options are
processed.  A few extensions in-core (or even outside core) that I
know of, could call BeginCopyTo() or BeginCopyFrom(), but the option
processing is the same for all.

The point about cleaning up the attribute handling of FORCE_NOT_NULL
and FORCE_NULL in the COPY TO path is a good catch, though, so let's
remove all that.  I'll go apply this part of the patch in a bit, or
tomorrow.
--
Michael

Attachment

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Richard Guo
Date:

On Tue, Nov 1, 2022 at 3:41 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 02, 2022 at 04:13:30PM +0800, Zhang Mingli wrote:
> On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, wrote:
>> There are some other option combinations that are rejected
>> by ProcessCopyOptions. On the other hand *re*checking all
>> combinations that the function should have rejected is kind of silly.
>> Addition to that, I doubt the assertions are really needed even though
>> the wrong values don't lead to any serious consequence.
>
> ProcessCopyOptions has rejected all invalid combinations and assertions are optional.

I agree with Horiguchi-san's point here: there is no real point in
having these assertions, especially just after the options are
processed.  A few extensions in-core (or even outside core) that I
know of, could call BeginCopyTo() or BeginCopyFrom(), but the option
processing is the same for all.
 
I'm OK with not having these assertions.  I have to admit they look
somewhat redundant here, after what ProcessCopyOptions has done.

Thanks
Richard

Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Mingli Zhang
Date:
Hi,


The point about cleaning up the attribute handling of FORCE_NOT_NULL
and FORCE_NULL in the COPY TO path is a good catch, though, so let's
remove all that.  I'll go apply this part of the patch in a bit, or
tomorrow.
--
Michael

Thanks for review!


Re: [Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO

From
Michael Paquier
Date:
On Tue, Nov 01, 2022 at 05:51:42PM +0800, Richard Guo wrote:
> I'm OK with not having these assertions.  I have to admit they look
> somewhat redundant here, after what ProcessCopyOptions has done.

Thanks, and done.

While on it, I have noticed some gaps with the coverage of the code,
where we did not check that FORCE_NULL & co are not allowed in some
cases.  With two tests for BINARY, that made a total of 8 patterns,
applied as of 451d116.
--
Michael

Attachment