On Thu, Jul 15, 2021 at 5:12 PM vignesh C wrote:
>
> On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed wrote:
> >
> > On Tue, 13 Jul 2021 at 15:30, vignesh C wrote:
> > >
> > > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed
> > > wrote:
> > > >
> > > > As it stands, the improvements from (3) seem
On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed wrote:
>
> On Tue, 13 Jul 2021 at 15:30, vignesh C wrote:
> >
> > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed
> > wrote:
> > >
> > > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > > the patch saves a couple of hundred
On Tue, 13 Jul 2021 at 15:30, vignesh C wrote:
>
> On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed wrote:
> >
> > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > the patch saves a couple of hundred lines of code, and for me an
> > optimised executable is around 30 kB
On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed wrote:
>
> On Mon, 12 Jul 2021 at 17:39, vignesh C wrote:
> >
> > Thanks for your comments, I have made the changes for the same in the
> > V10 patch attached.
> > Thoughts?
> >
>
> I'm still not happy about changing so many error messages.
>
> Some
On Mon, 12 Jul 2021 at 17:39, vignesh C wrote:
>
> Thanks for your comments, I have made the changes for the same in the
> V10 patch attached.
> Thoughts?
>
I'm still not happy about changing so many error messages.
Some of the changes might be OK, but aren't strictly necessary. For example:
On Sun, Jul 11, 2021 at 3:23 PM Dean Rasheed wrote:
>
> On Sat, 10 Jul 2021 at 18:09, vignesh C wrote:
> >
> > I'm planning to handle conflicting errors separately after this
> > current work is done, once the patch is changed to have just the valid
> > scenarios(removing the scenarios you have
.On Sun, Jul 11, 2021 at 2:57 PM Dean Rasheed wrote:
>
> On Sat, 10 Jul 2021 at 17:03, vignesh C wrote:
> >
> > > Also, I don't think this function should be marked inline -- using a
> > > normal function ought to help make the compiled code smaller.
> >
> > inline instructs the compiler to
On Sat, 10 Jul 2021 at 18:09, vignesh C wrote:
>
> I'm planning to handle conflicting errors separately after this
> current work is done, once the patch is changed to have just the valid
> scenarios(removing the scenarios you have pointed out) existing
> function can work as is without any
On Sat, 10 Jul 2021 at 17:03, vignesh C wrote:
>
> > Also, I don't think this function should be marked inline -- using a
> > normal function ought to help make the compiled code smaller.
>
> inline instructs the compiler to attempt to embed the function content
> into the calling code instead of
On Sat, Jul 10, 2021 at 4:31 PM Dean Rasheed wrote:
>
> On Sat, 10 Jul 2021 at 11:44, Dean Rasheed wrote:
> >
> > I'm inclined to think that it isn't worth the effort trying to
> > distinguish between conflicting options, options specified more than
> > once and faked-up options that weren't
On Sat, Jul 10, 2021 at 4:14 PM Dean Rasheed wrote:
>
> On Thu, 8 Jul 2021 at 14:40, vignesh C wrote:
> >
> > On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson wrote:
> > >
> > > I sort of like the visual cue of seeing ereport(ERROR .. since it makes
> > > it
> > > clear it will break
On Sat, 10 Jul 2021 at 11:44, Dean Rasheed wrote:
>
> I'm inclined to think that it isn't worth the effort trying to
> distinguish between conflicting options, options specified more than
> once and faked-up options that weren't really specified. If we just
> make errorConflictingDefElem() report
On Thu, 8 Jul 2021 at 14:40, vignesh C wrote:
>
> On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson wrote:
> >
> > I sort of like the visual cue of seeing ereport(ERROR .. since it makes it
> > clear it will break execution then and there, this will require a lookup for
> > anyone who don't know
On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson wrote:
>
> > On 6 Jul 2021, at 17:08, vignesh C wrote:
>
> > The patch was not applying on the head because of the recent commit
> > "8aafb02616753f5c6c90bbc567636b73c0cbb9d4", attached patch which is
> > rebased on HEAD.
>
> I sort of like the
> On 6 Jul 2021, at 17:08, vignesh C wrote:
> The patch was not applying on the head because of the recent commit
> "8aafb02616753f5c6c90bbc567636b73c0cbb9d4", attached patch which is
> rebased on HEAD.
I sort of like the visual cue of seeing ereport(ERROR .. since it makes it
clear it will
On Wed, Jun 30, 2021 at 7:48 PM vignesh C wrote:
>
> On Thu, May 13, 2021 at 8:09 PM vignesh C wrote:
> >
> > On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera
> > wrote:
> > >
> >
> > Thanks for the comments, Attached patch has the changes for the same.
> >
>
> The Patch was not applying on
On Thu, May 13, 2021 at 8:09 PM vignesh C wrote:
>
> On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera
> wrote:
> >
>
> Thanks for the comments, Attached patch has the changes for the same.
>
The Patch was not applying on Head, the attached patch is rebased on
top of Head.
Regards,
Vignesh
From
On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera wrote:
>
> You can avoid duplicating the ereport like this:
>
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("option \"%s\" specified more than
> once",
You can avoid duplicating the ereport like this:
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("option \"%s\" specified more than
once", defel->defname),
+parser ?
On Tue, May 11, 2021 at 6:54 PM vignesh C wrote:
>
> On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera
> wrote:
> >
> > On 2021-May-10, vignesh C wrote:
> >
> > > That sounds fine to me, Attached v6 patch which has the changes for the
> > > same.
> >
> > What about defining a function (maybe a
On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera wrote:
>
> On 2021-May-10, vignesh C wrote:
>
> > That sounds fine to me, Attached v6 patch which has the changes for the
> > same.
>
> What about defining a function (maybe a static inline function in
> defrem.h) that is marked noreturn and
On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera wrote:
>
> On 2021-May-10, vignesh C wrote:
>
> > That sounds fine to me, Attached v6 patch which has the changes for the
> > same.
>
> What about defining a function (maybe a static inline function in
> defrem.h) that is marked noreturn and
On 2021-May-10, vignesh C wrote:
> That sounds fine to me, Attached v6 patch which has the changes for the same.
What about defining a function (maybe a static inline function in
defrem.h) that is marked noreturn and receives the DefElem and
optionally pstate, and throws the error? I think that
On Mon, May 10, 2021 at 6:00 AM houzj.f...@fujitsu.com
wrote:
>
> > > > > > > > Thanks! The v5 patch looks good to me. Let's see if all
> > > > > > > > agree on the goto duplicate_error approach which could reduce
> > the LOC by ~80.
> > > > > > >
> > > > > > > I think the "goto duplicate_error"
> > > > > > > Thanks! The v5 patch looks good to me. Let's see if all
> > > > > > > agree on the goto duplicate_error approach which could reduce
> the LOC by ~80.
> > > > > >
> > > > > > I think the "goto duplicate_error" approach looks good, it
> > > > > > avoids duplicating the same error code
On Sat, May 8, 2021 at 7:06 PM vignesh C wrote:
>
> On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy
> wrote:
> >
> > On Sat, May 8, 2021 at 12:01 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > > > goto
On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy
wrote:
>
> On Sat, May 8, 2021 at 12:01 PM houzj.f...@fujitsu.com
> wrote:
> >
> > > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > > goto duplicate_error approach which could reduce the LOC by ~80.
> > > >
> > >
On Sat, May 8, 2021 at 12:01 PM houzj.f...@fujitsu.com
wrote:
>
> > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > goto duplicate_error approach which could reduce the LOC by ~80.
> > >
> > > I think the "goto duplicate_error" approach looks good, it avoids
> > >
> > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > goto duplicate_error approach which could reduce the LOC by ~80.
> >
> > I think the "goto duplicate_error" approach looks good, it avoids
> > duplicating the same error code multiple times.
>
> Thanks. I will mark
On Mon, May 3, 2021 at 1:41 PM Dilip Kumar wrote:
>
> On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
> wrote:
> >
> > On Sun, May 2, 2021 at 8:44 PM vignesh C wrote:
> > >
> > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > > wrote:
> > > >
> > > > On Sat, May 1, 2021 at 7:25 PM
On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
wrote:
>
> On Sun, May 2, 2021 at 8:44 PM vignesh C wrote:
> >
> > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > wrote:
> > >
> > > On Sat, May 1, 2021 at 7:25 PM vignesh C wrote:
> > > > > > I'm not attaching above one line change as a
On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
wrote:
>
> On Sun, May 2, 2021 at 8:44 PM vignesh C wrote:
> >
> > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > wrote:
> > >
> > > On Sat, May 1, 2021 at 7:25 PM vignesh C wrote:
> > > > > > I'm not attaching above one line change as a
On Sun, May 2, 2021 at 8:44 PM vignesh C wrote:
>
> On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> wrote:
> >
> > On Sat, May 1, 2021 at 7:25 PM vignesh C wrote:
> > > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > > merge this into the main patch.
> > >
>
On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
wrote:
>
> On Sat, May 1, 2021 at 7:25 PM vignesh C wrote:
> > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > merge this into the main patch.
> >
> > Thanks for the comments. I have merged the change into the
On Sat, May 1, 2021 at 10:54 PM Alvaro Herrera wrote:
>
> On 2021-May-01, vignesh C wrote:
>
> > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera
> > wrote:
> > >
> > > On 2021-Apr-29, vignesh C wrote:
> > >
> > > > Thanks for the comments, please find the attached v3 patch which has
> > > > the
Le dim. 2 mai 2021 à 18:31, vignesh C a écrit :
> On Sun, May 2, 2021 at 4:27 AM Alvaro Herrera
> wrote:
> >
> > On 2021-May-01, Bharath Rupireddy wrote:
> >
> > > IMO, it's not good to change the function API just for showing up
> > > parse_position (which is there for cosmetic reasons I feel)
On Sun, May 2, 2021 at 4:27 AM Alvaro Herrera wrote:
>
> On 2021-May-01, Bharath Rupireddy wrote:
>
> > IMO, it's not good to change the function API just for showing up
> > parse_position (which is there for cosmetic reasons I feel) in an error
> > which actually has the option name clearly
On 2021-May-01, Bharath Rupireddy wrote:
> IMO, it's not good to change the function API just for showing up
> parse_position (which is there for cosmetic reasons I feel) in an error
> which actually has the option name clearly mentioned in the error message.
Why not? We've done it before, I'm
On Sat, May 1, 2021, 10:54 PM Alvaro Herrera
wrote:
> On 2021-May-01, vignesh C wrote:
> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera
> wrote:
> > >
> > > On 2021-Apr-29, vignesh C wrote:
> > >
> > > > Thanks for the comments, please find the attached v3 patch which has
> > > > the change
On 2021-May-01, vignesh C wrote:
> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera
> wrote:
> >
> > On 2021-Apr-29, vignesh C wrote:
> >
> > > Thanks for the comments, please find the attached v3 patch which has
> > > the change for the first part.
> >
> > Looks good to me. I would only add
On Sat, May 1, 2021 at 7:25 PM vignesh C wrote:
> > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > merge this into the main patch.
>
> Thanks for the comments. I have merged the change into the attached patch.
> Thoughts?
Thanks! v4 basically LGTM. Can we park this
On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera wrote:
>
> On 2021-Apr-29, vignesh C wrote:
>
> > Thanks for the comments, please find the attached v3 patch which has
> > the change for the first part.
>
> Looks good to me. I would only add parser_errposition() to the few
> error sites missing
On Sat, May 1, 2021 at 10:47 AM Dilip Kumar wrote:
>
> On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar wrote:
> > > Looking into this again, why not as shown below? IMHO, this way the
> > > code will be logically the same as it was
On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy
wrote:
>
> On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar wrote:
> > Looking into this again, why not as shown below? IMHO, this way the
> > code will be logically the same as it was before the patch, basically
> > why to process an extra statement
On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar wrote:
> Looking into this again, why not as shown below? IMHO, this way the
> code will be logically the same as it was before the patch, basically
> why to process an extra statement ( *volatility_item = defel;) if we
> have already decided to error.
On Fri, Apr 30, 2021 at 12:36 PM Bharath Rupireddy
wrote:
> >
> > other comments
> >
> > if (strcmp(defel->defname, "volatility") == 0)
> > {
> > if (is_procedure)
> > - goto procedure_error;
> > + is_procedure_error = true;
> > if (*volatility_item)
> > - goto duplicate_error;
> > +
On Fri, Apr 30, 2021 at 5:06 PM Bharath Rupireddy
wrote:
>
> On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar wrote:
> >
> > On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
> > wrote:
> > >
> > > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar
> > > wrote:
> > > >
> > > > On Fri, Apr 30, 2021 at
On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar wrote:
>
> On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote:
> > >
> > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> > > wrote:
> > > >
> > > > On Fri, Apr 30, 2021 at
On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
wrote:
>
> On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote:
> >
> > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> > wrote:
> > >
> > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar
> > > wrote:
> > > > In this function, we already
On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote:
>
> On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote:
> > > In this function, we already have the "defel" variable then I do not
> > > understand why you are using one extra
On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
wrote:
>
> On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote:
> > In this function, we already have the "defel" variable then I do not
> > understand why you are using one extra variable and assigning defel to
> > that?
> > If the goal is to
On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote:
> In this function, we already have the "defel" variable then I do not
> understand why you are using one extra variable and assigning defel to
> that?
> If the goal is to just improve the error message then you can simply
> use defel->defname?
On Fri, Apr 30, 2021 at 8:16 AM Bharath Rupireddy
wrote:
>
> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera
> wrote:
> >
> > On 2021-Apr-29, vignesh C wrote:
> >
> > > Thanks for the comments, please find the attached v3 patch which has
> > > the change for the first part.
> >
> > Looks good
On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera wrote:
>
> On 2021-Apr-29, vignesh C wrote:
>
> > Thanks for the comments, please find the attached v3 patch which has
> > the change for the first part.
>
> Looks good to me. I would only add parser_errposition() to the few
> error sites missing
On 2021-Apr-29, vignesh C wrote:
> Thanks for the comments, please find the attached v3 patch which has
> the change for the first part.
Looks good to me. I would only add parser_errposition() to the few
error sites missing that.
--
Álvaro Herrera39°49'30"S
On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > I agree that we can just be clear about the problem. Looks like the
> > majority of the errors "conflicting or redundant options" are for
> > redundant options. So, wherever "conflicting or
On Tue, Apr 27, 2021 at 6:23 AM Bharath Rupireddy
wrote:
>
> On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
> wrote:
> >
> > I found another problem with collationcmds.c is that it doesn't error
> > out if some of the options are specified more than once, something
> > like below. I think the
On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
wrote:
>
> I found another problem with collationcmds.c is that it doesn't error
> out if some of the options are specified more than once, something
> like below. I think the option checking "for loop" in DefineCollation
> needs to be reworked.
>
On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > I agree that we can just be clear about the problem. Looks like the
> > majority of the errors "conflicting or redundant options" are for
> > redundant options. So, wherever "conflicting or
On 2021-Apr-26, Bharath Rupireddy wrote:
> I agree that we can just be clear about the problem. Looks like the
> majority of the errors "conflicting or redundant options" are for
> redundant options. So, wherever "conflicting or redundant options"
> exists: 1) change the message to "option \"%s\"
On Mon, Apr 26, 2021 at 8:06 PM Alvaro Herrera wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > Thanks! IMO, it is better to change the error message to "option
> > \"%s\" specified more than once" instead of adding an error detail.
> > Let's hear other hackers' opinions.
>
> Many other
On 2021-Apr-26, Bharath Rupireddy wrote:
> Thanks! IMO, it is better to change the error message to "option
> \"%s\" specified more than once" instead of adding an error detail.
> Let's hear other hackers' opinions.
Many other places have the message "conflicting or redundant options",
and then
On Mon, Apr 26, 2021 at 7:02 PM vignesh C wrote:
>
> On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
> wrote:
> >
> > On Mon, Apr 26, 2021 at 5:29 PM vignesh C wrote:
> > >
> > > Hi,
> > >
> > > While reviewing one of the logical replication patches, I found that
> > > we do not include hint
On Mon, Apr 26, 2021 at 6:18 PM Dilip Kumar wrote:
>
> On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
> wrote:
> >
> > On Mon, Apr 26, 2021 at 5:29 PM vignesh C wrote:
> > >
> > > Hi,
> > >
> > > While reviewing one of the logical replication patches, I found that
> > > we do not include
On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
wrote:
>
> On Mon, Apr 26, 2021 at 5:29 PM vignesh C wrote:
> >
> > Hi,
> >
> > While reviewing one of the logical replication patches, I found that
> > we do not include hint messages to display the actual option which has
> > been specified
On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
wrote:
>
> On Mon, Apr 26, 2021 at 5:29 PM vignesh C wrote:
> >
> > Hi,
> >
> > While reviewing one of the logical replication patches, I found that
> > we do not include hint messages to display the actual option which has
> > been specified
On Mon, Apr 26, 2021 at 5:29 PM vignesh C wrote:
>
> Hi,
>
> While reviewing one of the logical replication patches, I found that
> we do not include hint messages to display the actual option which has
> been specified more than once in case of redundant option error. I
> felt including this
Hi,
While reviewing one of the logical replication patches, I found that
we do not include hint messages to display the actual option which has
been specified more than once in case of redundant option error. I
felt including this will help in easily identifying the error, users
will not have to
68 matches
Mail list logo
