Horton

Thread

Commits

  1. Remove code handling FORCE_NULL and FORCE_NOT_NULL for COPY TO

  2. Add more tests for COPY with incorrect option combinations

  3. Split copy.c into four files.

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

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-07-26T16:16:33Z

    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 <https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df> .

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


Regards,
Zhang Mingli

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-27T04:55:49Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2022-07-27T06:37:40Z

    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

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

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-07-28T13:04:04Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2022-07-29T03:23:57Z

    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

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

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-08-01T01:59:49Z

    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

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-02T04:30:50Z

    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

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

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-08-02T08:13:30Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2022-11-01T07:41:15Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2022-11-01T09:51:42Z

    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

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

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-11-01T15:45:45Z

    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!

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

    Michael Paquier <michael@paquier.xyz> — 2022-11-02T01:15:37Z

    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