[Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
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
Attachments:
vn-0001-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patchapplication/octet-stream; name=vn-0001-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patch; x-unix-mode=0644Download
From 2a31dd49ff313914f39b56756103031259c0fe80 Mon Sep 17 00:00:00 2001
From: Mingli Zhang <avamingli@gmail.com>
Date: Tue, 26 Jul 2022 23:38:56 +0800
Subject: [PATCH vn] Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when
COPY TO.
FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.
Remove unnecessary codes when COPY TO.
---
src/backend/commands/copyto.c | 46 -----------------------------------
1 file changed, 46 deletions(-)
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..64422ab9b9 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -591,52 +591,6 @@ BeginCopyTo(ParseState *pstate,
}
}
- /* Convert FORCE_NOT_NULL name list to per-column flags, check validity */
- cstate->opts.force_notnull_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_notnull)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NOT_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_notnull_flags[attnum - 1] = true;
- }
- }
-
- /* Convert FORCE_NULL name list to per-column flags, check validity */
- cstate->opts.force_null_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_null)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_null_flags[attnum - 1] = true;
- }
- }
-
/* Use client encoding when ENCODING option is not specified. */
if (cstate->opts.file_encoding < 0)
cstate->file_encoding = pg_get_client_encoding();
--
2.34.1
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
Show quoted text
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
Attachments:
vn-0002-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patchapplication/octet-streamDownload
From 6b4621d50fdad5f07d9a90abb4e21c180b553ce4 Mon Sep 17 00:00:00 2001
From: Mingli Zhang <avamingli@gmail.com>
Date: Tue, 26 Jul 2022 23:38:56 +0800
Subject: [PATCH vn] Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when
COPY TO.
FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.
Remove unnecessary codes when COPY TO.
---
src/backend/commands/copyfrom.c | 3 ++
src/backend/commands/copyto.c | 49 ++-------------------------------
2 files changed, 6 insertions(+), 46 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a976008b3d..12e0b6ed1e 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1234,6 +1234,9 @@ BeginCopyFrom(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
+ /* force_quote can't be used in COPY FROM */
+ Assert(cstate->opts.force_quote == NULL);
+ Assert(!cstate->opts.force_quote_all);
/* Process the target relation */
cstate->rel = rel;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..d7558318a0 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -415,6 +415,9 @@ BeginCopyTo(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
+ /* force_null and force_notnull can't be used in COPY TO */
+ Assert(cstate->opts.force_null == NULL);
+ Assert(cstate->opts.force_notnull== NULL);
/* Process the source/target relation or query */
if (rel)
@@ -591,52 +594,6 @@ BeginCopyTo(ParseState *pstate,
}
}
- /* Convert FORCE_NOT_NULL name list to per-column flags, check validity */
- cstate->opts.force_notnull_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_notnull)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NOT_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_notnull_flags[attnum - 1] = true;
- }
- }
-
- /* Convert FORCE_NULL name list to per-column flags, check validity */
- cstate->opts.force_null_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_null)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_null_flags[attnum - 1] = true;
- }
- }
-
/* Use client encoding when ENCODING option is not specified. */
if (cstate->opts.file_encoding < 0)
cstate->file_encoding = pg_get_client_encoding();
--
2.34.1
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
HI,
More assertions added.
Thanks.
Regards,
Zhang Mingli
Show quoted text
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
Attachments:
vn-0003-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patchapplication/octet-streamDownload
From 9b4d44b37414e6471b9452c6a33ff7eb565651e5 Mon Sep 17 00:00:00 2001
From: Mingli Zhang <avamingli@gmail.com>
Date: Tue, 26 Jul 2022 23:38:56 +0800
Subject: [PATCH vn] Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when
COPY TO.
FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.
Remove unnecessary codes when COPY TO.
Add assertions.
---
src/backend/commands/copyfrom.c | 5 ++++
src/backend/commands/copyto.c | 49 ++-------------------------------
2 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a976008b3d..086a05ef52 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1234,6 +1234,11 @@ BeginCopyFrom(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
+ /* force_null, force_notnull avaliable only in CSV mode */
+ Assert((cstate->opts.force_null != NIL || cstate->opts.force_notnull != NIL) ? cstate->opts.csv_mode : true);
+ /* force_quote can't be used in COPY FROM */
+ Assert(cstate->opts.force_quote == NULL);
+ Assert(!cstate->opts.force_quote_all);
/* Process the target relation */
cstate->rel = rel;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..d7558318a0 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -415,6 +415,9 @@ BeginCopyTo(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
+ /* force_null and force_notnull can't be used in COPY TO */
+ Assert(cstate->opts.force_null == NULL);
+ Assert(cstate->opts.force_notnull== NULL);
/* Process the source/target relation or query */
if (rel)
@@ -591,52 +594,6 @@ BeginCopyTo(ParseState *pstate,
}
}
- /* Convert FORCE_NOT_NULL name list to per-column flags, check validity */
- cstate->opts.force_notnull_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_notnull)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NOT_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_notnull_flags[attnum - 1] = true;
- }
- }
-
- /* Convert FORCE_NULL name list to per-column flags, check validity */
- cstate->opts.force_null_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_null)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_null_flags[attnum - 1] = true;
- }
- }
-
/* Use client encoding when ENCODING option is not specified. */
if (cstate->opts.file_encoding < 0)
cstate->file_encoding = pg_get_client_encoding();
--
2.34.1
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
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.
Show quoted text
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
vn-0004-Avoid-to-handle-FORCE_NOT_NULL-FORCE_NULL-options.patchapplication/octet-streamDownload
From 05a689bb19c03eb50ec40e764d9d7843e0df819e Mon Sep 17 00:00:00 2001
From: Mingli Zhang <avamingli@gmail.com>
Date: Tue, 26 Jul 2022 23:38:56 +0800
Subject: [PATCH vn] Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when
COPY TO.
FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM.
Remove unnecessary codes when COPY TO.
---
src/backend/commands/copyfrom.c | 5 ++++
src/backend/commands/copyto.c | 49 ++-------------------------------
2 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a976008b3d..add6e54f24 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1234,6 +1234,11 @@ BeginCopyFrom(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
+ /* force_null, force_notnull avaliable only in CSV mode */
+ Assert((cstate->opts.force_null != NIL || cstate->opts.force_notnull != NIL) ? cstate->opts.csv_mode : true);
+ /* force_quote can't be used in COPY FROM */
+ Assert(cstate->opts.force_quote == NIL);
+ Assert(!cstate->opts.force_quote_all);
/* Process the target relation */
cstate->rel = rel;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..1bae279292 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -415,6 +415,9 @@ BeginCopyTo(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
+ /* force_null and force_notnull can't be used in COPY TO */
+ Assert(cstate->opts.force_null == NIL);
+ Assert(cstate->opts.force_notnull== NIL);
/* Process the source/target relation or query */
if (rel)
@@ -591,52 +594,6 @@ BeginCopyTo(ParseState *pstate,
}
}
- /* Convert FORCE_NOT_NULL name list to per-column flags, check validity */
- cstate->opts.force_notnull_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_notnull)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_notnull);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NOT_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_notnull_flags[attnum - 1] = true;
- }
- }
-
- /* Convert FORCE_NULL name list to per-column flags, check validity */
- cstate->opts.force_null_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- if (cstate->opts.force_null)
- {
- List *attnums;
- ListCell *cur;
-
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->opts.force_null);
-
- foreach(cur, attnums)
- {
- int attnum = lfirst_int(cur);
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (!list_member_int(cstate->attnumlist, attnum))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("FORCE_NULL column \"%s\" not referenced by COPY",
- NameStr(attr->attname))));
- cstate->opts.force_null_flags[attnum - 1] = true;
- }
- }
-
/* Use client encoding when ENCODING option is not specified. */
if (cstate->opts.file_encoding < 0)
cstate->file_encoding = pg_get_client_encoding();
--
2.34.1
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 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
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!
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