Report error position in partition bound check

Forgot to run make installcheck. Here's the new version of the patch that
updated the test answer file.

On Wed, Apr 08, 2020 at 05:15:57PM -0700, Alexandra Wang wrote:

I have attached a patch to pass in a ParseState to
check_new_partition_bound() to enable the reporting of the error
position. Below is what the error message looks like before and after
applying the patch.

Another option is to not pass the parser_errposition() argument at all
to ereport() in this function, since the query is relatively short and
the error message is already descriptive enough.

It depends on the complexity of the relation definition, so adding a
position looks like a good idea to me. Anyway, even if this looks
like an oversight to me, we are post feature freeze for 13 and that's
an improvement, so this looks like material for PG14 to me. Are there
more opinions on the matter?

Please note that you forgot to update the regression test output.
--
Michael

On Wed, Apr 8, 2020 at 6:11 PM Michael Paquier <michael@paquier.xyz> wrote:

Please note that you forgot to update the regression test output.

Yep thanks! Please see my previous email for the updated patch.

On Wed, Apr 08, 2020 at 08:17:55PM -0700, Alexandra Wang wrote:

On Wed, Apr 8, 2020 at 6:11 PM Michael Paquier <michael@paquier.xyz> wrote:

Please note that you forgot to update the regression test output.

Yep thanks! Please see my previous email for the updated patch.

Thanks, I saw the update. It looks like my email was a couple of
minutes too late :)

Could you add this patch to the next commit fest [1]https://commitfest.postgresql.org/28/ -- Michael?

[1]: https://commitfest.postgresql.org/28/ -- Michael
--
Michael

Hi Alexandra,
As Michael said it will be considered for the next commitfest. But
from a quick glance, a suggestion.
Instead of passing NULL parsestate from ATExecAttachPartition, pass
make_parsestate(NULL). parse_errorposition() takes care of NULL parse
state input, but it might be safer this way. Better if we could cook
up a parse state with the query text available in
AlterTableUtilityContext available in ATExecCmd().

On Thu, Apr 9, 2020 at 6:36 AM Alexandra Wang <lewang@pivotal.io> wrote:

Forgot to run make installcheck. Here's the new version of the patch that updated the test answer file.

--
Best Wishes,
Ashutosh Bapat

While I'm quite on board with providing useful error cursors,
the example cases in this patch don't seem all that useful:

 -- trying to create range partition with empty range
 CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (0);
 ERROR:  empty range bound specified for partition "fail_part"
+LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) T...
+                                                             ^
 DETAIL:  Specified lower bound (1) is greater than or equal to upper bound (0).

As best I can tell from these examples, the cursor will always
point at the FROM keyword, making it pretty unhelpful. It seems
like in addition to getting the query string passed down, you
need to do some work on the code that's actually reporting the
error position. I'd expect at a minimum that the pointer allows
identifying which column of a multi-column partition key is
giving trouble. The phrasing of this particular message, for
example, suggests that it ought to point at the "1" expression.

regards, tom lane

On Thu, Apr 9, 2020 at 10:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi Alexandra,
As Michael said it will be considered for the next commitfest. But
from a quick glance, a suggestion.
Instead of passing NULL parsestate from ATExecAttachPartition, pass
make_parsestate(NULL). parse_errorposition() takes care of NULL parse
state input, but it might be safer this way. Better if we could cook
up a parse state with the query text available in
AlterTableUtilityContext available in ATExecCmd().

+1. Maybe pass the *context* down to ATExecAttachPartition() from
ATExecCmd() rather than a ParseState.

--

Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 9, 2020 at 11:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

While I'm quite on board with providing useful error cursors,
the example cases in this patch don't seem all that useful:

-- trying to create range partition with empty range
CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (0);
ERROR:  empty range bound specified for partition "fail_part"
+LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) T...
+                                                             ^
DETAIL:  Specified lower bound (1) is greater than or equal to upper bound (0).

As best I can tell from these examples, the cursor will always
point at the FROM keyword, making it pretty unhelpful. It seems
like in addition to getting the query string passed down, you
need to do some work on the code that's actually reporting the
error position. I'd expect at a minimum that the pointer allows
identifying which column of a multi-column partition key is
giving trouble. The phrasing of this particular message, for
example, suggests that it ought to point at the "1" expression.

I agree with that. Tried that in the attached 0002, although trying
to get the cursor to point to exactly the offending column seems a bit
tough for partition overlap errors. The patch does allow to single
out which one of the lower and upper bounds is causing the overlap
with an existing partition, which is better than now and seems helpful
enough.

Also, updated Alexandra's patch to incorporate Ashutosh's comment such
that we get the same output with ATTACH PARTITION commands too.

--

Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Fri, 10 Apr 2020 at 14:31, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Apr 9, 2020 at 11:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

While I'm quite on board with providing useful error cursors,
the example cases in this patch don't seem all that useful:

-- trying to create range partition with empty range
CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1)

TO (0);

ERROR: empty range bound specified for partition "fail_part"
+LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1)

T...

+ ^
DETAIL: Specified lower bound (1) is greater than or equal to upper

bound (0).

As best I can tell from these examples, the cursor will always
point at the FROM keyword, making it pretty unhelpful. It seems
like in addition to getting the query string passed down, you
need to do some work on the code that's actually reporting the
error position. I'd expect at a minimum that the pointer allows
identifying which column of a multi-column partition key is
giving trouble. The phrasing of this particular message, for
example, suggests that it ought to point at the "1" expression.

I agree with that. Tried that in the attached 0002, although trying
to get the cursor to point to exactly the offending column seems a bit
tough for partition overlap errors. The patch does allow to single
out which one of the lower and upper bounds is causing the overlap
with an existing partition, which is better than now and seems helpful
enough.

Also, updated Alexandra's patch to incorporate Ashutosh's comment such
that we get the same output with ATTACH PARTITION commands too.

I looked at this briefly. It looks good, but I will review more in the next
CF. Do we have entry there yet? To nit-pick: for a multi-key value the ^
points to the first column and the reader may think that that's the
problematci column. Should it instead point to ( ?

--
Best Wishes,
Ashutosh

On Fri, 10 Apr 2020 at 14:31, Amit Langote <amitlangote09@gmail.com> wrote:

I agree with that. Tried that in the attached 0002, although trying
to get the cursor to point to exactly the offending column seems a bit
tough for partition overlap errors. The patch does allow to single
out which one of the lower and upper bounds is causing the overlap
with an existing partition, which is better than now and seems helpful
enough.

Also, updated Alexandra's patch to incorporate Ashutosh's comment such
that we get the same output with ATTACH PARTITION commands too.

Thank you Amit for updating the patches, the cursor looks much helpful now.
I
created the commitfest entry https://commitfest.postgresql.org/28/2533/

On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <
ashutosh.bapat@2ndquadrant.com> wrote:

for a multi-key value the ^
points to the first column and the reader may think that that's the
problematci column. Should it instead point to ( ?

I attached a v2 of Amit's 0002 patch to also report the exact column
for the partition overlap errors.

On 10 Apr 2020, at 23:50, Alexandra Wang <lewang@pivotal.io> wrote:

On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com <mailto:ashutosh.bapat@2ndquadrant.com>> wrote:

for a multi-key value the ^
points to the first column and the reader may think that that's the
problematci column. Should it instead point to ( ?

I attached a v2 of Amit's 0002 patch to also report the exact column
for the partition overlap errors.

This patch fails to apply to HEAD due to conflicts in the create_table expected
output. Can you please submit a rebased version? I'm marking the CF entry
Waiting on Author in the meantime.

cheers ./daniel

On 2 July 2020, at 06:39, Daniel Gustafsson <daniel@yesql.se> wrote:

On 10 Apr 2020, at 23:50, Alexandra Wang <lewang@pivotal.io> wrote:

On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com <mailto:ashutosh.bapat@2ndquadrant.com>> wrote:

for a multi-key value the ^
points to the first column and the reader may think that that's the
problematci column. Should it instead point to ( ?

I attached a v2 of Amit's 0002 patch to also report the exact column
for the partition overlap errors.

This patch fails to apply to HEAD due to conflicts in the create_table expected
output. Can you please submit a rebased version? I'm marking the CF entry
Waiting on Author in the meantime.

Thank you Daniel. Here's the rebased patch. I also squashed the two
patches into one so it's easier to review.

--
Alex