Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-18 Thread Dean Rasheed
On Sat, 17 Jul 2021 at 05:24, Bharath Rupireddy wrote: > > I also think that if it is specified as CREATE FUNCTION ... STRICT > STRICT, CREATE FUNCTION ... CALLED ON NULL INPUT RETURNS NULL ON NULL > INPUT etc. isn't the syntaxer catching that error while parsing the > SQL text, similar to CREATE

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-18 Thread Dean Rasheed
On Fri, 16 Jul 2021 at 12:17, Dean Rasheed wrote: > > On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy > wrote: > > > > Thanks. The v5 patch LGTM. > > OK, I'll push that in a while. > Pushed, with some additional tidying up. In particular, I decided it was neater to follow the style of the code

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-16 Thread Bharath Rupireddy
On Fri, Jul 16, 2021 at 4:47 PM Dean Rasheed wrote: > > On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy > wrote: > > > > Thanks. The v5 patch LGTM. > > OK, I'll push that in a while. Thanks. > There are a few cases where def->defname isn't necessarily the name > that was specified by the user

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-16 Thread Dean Rasheed
On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy wrote: > > Thanks. The v5 patch LGTM. OK, I'll push that in a while. > Comment on errorConflictingDefElem: > I think that the message in errorConflictingDefElem should say > <>. I'm not sure why it > wasn't done. Almost, all the cases where

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-16 Thread Bharath Rupireddy
On Fri, Jul 16, 2021 at 1:32 PM Dean Rasheed wrote: > > On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy > wrote: > > > > 1) Duplicate option check for "FROM" option is unnecessary and will be > > a dead code. Because the syntaxer anyways would catch if FROM is > > specified more than once,

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-16 Thread Dean Rasheed
On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy wrote: > > 1) Duplicate option check for "FROM" option is unnecessary and will be > a dead code. Because the syntaxer anyways would catch if FROM is > specified more than once, something like CREATE COLLATION mycoll1 FROM > FROM "C";. Hmm, it is

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-15 Thread Bharath Rupireddy
On Fri, Jul 16, 2021 at 1:04 AM Dean Rasheed wrote: > Having pushed [1], I started looking at this, and I think it's mostly > in good shape. Thanks a lot for taking a look at this. > Since we're improving the CREATE COLLATION errors, I think it's also > worth splitting out the error for LOCALE

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-07-15 Thread Dean Rasheed
On Mon, 31 May 2021 at 15:10, vignesh C wrote: > > On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy > wrote: > > > > Thanks. PSA v3 patch. > > Thanks for the updated patch, the changes look good to me. > Hi, Having pushed [1], I started looking at this, and I think it's mostly in good shape.

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-31 Thread vignesh C
On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy wrote: > > On Sat, May 29, 2021 at 9:08 PM vignesh C wrote: > > One minor comment: > > You can remove the brackets around errcode, You could change: > > + if (localeEl) > > + ereport(ERROR, > > + (errcode(ERRCODE_SYNTAX_ERROR), > > +

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-29 Thread Bharath Rupireddy
On Sat, May 29, 2021 at 9:08 PM vignesh C wrote: > One minor comment: > You can remove the brackets around errcode, You could change: > + if (localeEl) > + ereport(ERROR, > + (errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("option \"%s\" specified more than once", defel->defname), > +

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-29 Thread vignesh C
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy wrote: > > On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > > +1 for fixing this issue, we have handled this error in other places. > > The patch does not apply on head, could you rebase the patch on head > > and post a new patch. > > Thanks. I

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-27 Thread Bharath Rupireddy
On Thu, May 27, 2021 at 8:36 PM vignesh C wrote: > > On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy > wrote: > > > > On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > > > +1 for fixing this issue, we have handled this error in other places. > > > The patch does not apply on head, could you

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-27 Thread vignesh C
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy wrote: > > On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > > +1 for fixing this issue, we have handled this error in other places. > > The patch does not apply on head, could you rebase the patch on head > > and post a new patch. > > Thanks. I

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-26 Thread Bharath Rupireddy
On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > +1 for fixing this issue, we have handled this error in other places. > The patch does not apply on head, could you rebase the patch on head > and post a new patch. Thanks. I thought of rebasing once the other patch (which reorganizes

Re: CREATE COLLATION - check for duplicate options and error out if found one

2021-05-26 Thread vignesh C
On Tue, Apr 27, 2021 at 3:21 PM Bharath Rupireddy wrote: > > Hi, > > While reviewing [1], I found that the CREATE COLLATION doesn't throw an error > if duplicate options are specified, see [2] for testing a few cases on HEAD. > This may end up accepting some of the weird cases, see [2]. It's

CREATE COLLATION - check for duplicate options and error out if found one

2021-04-27 Thread Bharath Rupireddy
Hi, While reviewing [1], I found that the CREATE COLLATION doesn't throw an error if duplicate options are specified, see [2] for testing a few cases on HEAD. This may end up accepting some of the weird cases, see [2]. It's against other option checking code in the server where the duplicate