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

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

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

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

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