Horton

Thread

Commits

  1. Improve error cursor positions for problems with partition bounds.

  1. Report error position in partition bound check

    Alexandra Wang <lewang@pivotal.io> — 2020-04-09T00:15:57Z

    Hi,

I'm playing with partitioned tables and found a minor thing with the
error reporting of bounds checking when create partitions.

In function check_new_partition_bound(), there are three places where
we call ereport() with a parser_errposition(pstate, spec->location)
argument.  However, that pstate is a dummy ParseState made from NULL,
so the error message never reports the position of the error in the
source query line.


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.

-- Create parent table
create table foo (a int, b date) partition by range (b);

-- Before:
create table foo_part_1 partition of foo for values from (date
'2007-01-01') to (date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
DETAIL:  Specified lower bound ('2007-01-01') is greater than or equal to
upper bound ('2006-01-01').

-- After:
create table foo_part_1 partition of foo for values from (date
'2007-01-01') to (date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
LINE 1: ...eate table foo_part_1 partition of foo for values from (date...
                                                             ^
DETAIL:  Specified lower bound ('2007-01-01') is greater than or equal to
upper bound ('2006-01-01').

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.

Alex and Ashwin

  2. Re: Report error position in partition bound check

    Alexandra Wang <lewang@pivotal.io> — 2020-04-09T01:05:58Z

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

  3. Re: Report error position in partition bound check

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T01:11:03Z

    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

  4. Re: Report error position in partition bound check

    Alexandra Wang <lewang@pivotal.io> — 2020-04-09T03:17:55Z

    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.

  5. Re: Report error position in partition bound check

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T03:40:35Z

    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]?

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

  6. Re: Report error position in partition bound check

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2020-04-09T13:51:15Z

    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

  7. Re: Report error position in partition bound check

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-09T14:03:55Z

    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

  8. Re: Report error position in partition bound check

    Amit Langote <amitlangote09@gmail.com> — 2020-04-09T14:08:36Z

    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

  9. Re: Report error position in partition bound check

    Amit Langote <amitlangote09@gmail.com> — 2020-04-10T09:01:37Z

    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

  10. Re: Report error position in partition bound check

    Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com> — 2020-04-10T15:37:25Z

    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

  11. Re: Report error position in partition bound check

    Alexandra Wang <lewang@pivotal.io> — 2020-04-10T16:59:49Z

    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/

  12. Re: Report error position in partition bound check

    Alexandra Wang <lewang@pivotal.io> — 2020-04-10T21:50:17Z

    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.

  13. Re: Report error position in partition bound check

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-02T13:39:09Z

    > 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

  14. Re: Report error position in partition bound check

    Alexandra Wang <walexandra@vmware.com> — 2020-07-13T17:53:46Z

    > 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