Horton

Thread

Commits

  1. Multirange datatypes

  1. range_agg

    Zhihong Yu <zyu@yugabyte.com> — 2020-12-16T21:54:45Z

    Hi,
w.r.t. patch v27.

+    * The idea is to prepend underscores as needed until we make a name
that
+    * doesn't collide with anything ...

I wonder if other characters (e.g. [a-z0-9]) can be used so that name
without collision can be found without calling truncate_identifier().

+       else if (strcmp(defel->defname, "multirange_type_name") == 0)
+       {
+           if (multirangeTypeName != NULL)
+               ereport(ERROR,
+                       (errcode(ERRCODE_SYNTAX_ERROR),
+                        errmsg("conflicting or redundant options")));

Maybe make the error message a bit different from occurrences of similar
error message (such as including multirangeTypeName) ?

Thanks

  2. Re: range_agg

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-12-16T22:03:38Z

    On Thu, Dec 17, 2020 at 12:54 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> +    * The idea is to prepend underscores as needed until we make a name that
> +    * doesn't collide with anything ...
>
> I wonder if other characters (e.g. [a-z0-9]) can be used so that name without collision can be found without calling truncate_identifier().

Probably.  But multiranges just shares naming logic already existing
in arrays.  If we're going to change this, I think we should change
this for arrays too.  And this change shouldn't be part of multirange
patch.

> +       else if (strcmp(defel->defname, "multirange_type_name") == 0)
> +       {
> +           if (multirangeTypeName != NULL)
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_SYNTAX_ERROR),
> +                        errmsg("conflicting or redundant options")));
>
> Maybe make the error message a bit different from occurrences of similar error message (such as including multirangeTypeName) ?

This is again isn't an invention of multirange.  We use this message
many times in DefineRange() and other places.  From the first glance,
I've nothing against changing this to a more informative message, but
that should be done globally.  And this change isn't directly related
to multirage.  Feel free to propose a patch improving this.

------
Regards,
Alexander Korotkov

  3. Re: range_agg

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-12-16T23:34:39Z

    On Thu, Dec 17, 2020 at 1:03 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Thu, Dec 17, 2020 at 12:54 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> > +    * The idea is to prepend underscores as needed until we make a name that
> > +    * doesn't collide with anything ...
> >
> > I wonder if other characters (e.g. [a-z0-9]) can be used so that name without collision can be found without calling truncate_identifier().
>
> Probably.  But multiranges just shares naming logic already existing
> in arrays.  If we're going to change this, I think we should change
> this for arrays too.  And this change shouldn't be part of multirange
> patch.

I gave this another thought.  Now we have facility to name multirange
types manually.  I think we should give up with underscore naming
completely.  If both replacing "range" with "mutlirange" in the
typename and appending "_multirange" to the type name failed (very
unlikely), then let user manually name the multirange.  Any thoughts?

------
Regards,
Alexander Korotkov

  4. Re: range_agg

    Zhihong Yu <zyu@yugabyte.com> — 2020-12-16T23:37:50Z

    Letting user manually name the multirange (after a few automatic attempts)
seems reasonable.

Cheers

On Wed, Dec 16, 2020 at 3:34 PM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> On Thu, Dec 17, 2020 at 1:03 AM Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> > On Thu, Dec 17, 2020 at 12:54 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> > > +    * The idea is to prepend underscores as needed until we make a
> name that
> > > +    * doesn't collide with anything ...
> > >
> > > I wonder if other characters (e.g. [a-z0-9]) can be used so that name
> without collision can be found without calling truncate_identifier().
> >
> > Probably.  But multiranges just shares naming logic already existing
> > in arrays.  If we're going to change this, I think we should change
> > this for arrays too.  And this change shouldn't be part of multirange
> > patch.
>
> I gave this another thought.  Now we have facility to name multirange
> types manually.  I think we should give up with underscore naming
> completely.  If both replacing "range" with "mutlirange" in the
> typename and appending "_multirange" to the type name failed (very
> unlikely), then let user manually name the multirange.  Any thoughts?
>
> ------
> Regards,
> Alexander Korotkov
>

  5. Re: range_agg

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-12-16T23:41:17Z

    On Thu, Dec 17, 2020 at 2:37 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> Letting user manually name the multirange (after a few automatic attempts) seems reasonable.

Accepted.  Thank you for your feedback.

------
Regards,
Alexander Korotkov