Enhanced error message to include hint messages for redundant options error

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> 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 will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

+1. A more detailed error will be useful in a rare scenario like users
have specified duplicate options along with a lot of other options,
they will know for which option the error is thrown by the server.

Instead of adding errhint or errdetail, how about just changing the
error message itself to something like "option \"%s\" specified more
than once" or "parameter \"%s\" specified more than once" like we have
in other places in the code?

Comments on the patch:

1) generally errhint and errdetail messages should end with a period
".", please see their usage in other places.
+ errhint("Option \"streaming\" specified more
than once")));

2) I think it should be errdetail instead of errhint, because you are
giving more information about the error, but not hinting user how to
overcome it. If you had to say something like "Remove duplicate
\"password\" option." or "The \"password\" option is specified more
than once. Remove all the duplicates.", then it makes sense to use
errhint.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> 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 will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

+1 for improving the error

Comments on the patch:

1) generally errhint and errdetail messages should end with a period
".", please see their usage in other places.
+ errhint("Option \"streaming\" specified more
than once")));

2) I think it should be errdetail instead of errhint, because you are
giving more information about the error, but not hinting user how to
overcome it. If you had to say something like "Remove duplicate
\"password\" option." or "The \"password\" option is specified more
than once. Remove all the duplicates.", then it makes sense to use
errhint.

I agree this should be errdetail.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> 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 will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

+1. A more detailed error will be useful in a rare scenario like users
have specified duplicate options along with a lot of other options,
they will know for which option the error is thrown by the server.

Instead of adding errhint or errdetail, how about just changing the
error message itself to something like "option \"%s\" specified more
than once" or "parameter \"%s\" specified more than once" like we have
in other places in the code?

Both seemed fine but I preferred using errdetail as I felt it is
slightly better for the details to appear in a new line.

Comments on the patch:

1) generally errhint and errdetail messages should end with a period
".", please see their usage in other places.
+ errhint("Option \"streaming\" specified more
than once")));

Modified it.

2) I think it should be errdetail instead of errhint, because you are
giving more information about the error, but not hinting user how to
overcome it. If you had to say something like "Remove duplicate
\"password\" option." or "The \"password\" option is specified more
than once. Remove all the duplicates.", then it makes sense to use
errhint.

Modified it.

Attached patch for the same.

Regards,
Vignesh

On Mon, Apr 26, 2021 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> 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 will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

+1 for improving the error

Comments on the patch:

1) generally errhint and errdetail messages should end with a period
".", please see their usage in other places.
+ errhint("Option \"streaming\" specified more
than once")));

2) I think it should be errdetail instead of errhint, because you are
giving more information about the error, but not hinting user how to
overcome it. If you had to say something like "Remove duplicate
\"password\" option." or "The \"password\" option is specified more
than once. Remove all the duplicates.", then it makes sense to use
errhint.

I agree this should be errdetail.

Thanks for the comment, I have modified and shared the v2 patch
attached in the previous mail.

Regards,
Vignesh

On Mon, Apr 26, 2021 at 7:02 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> 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 will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

+1. A more detailed error will be useful in a rare scenario like users
have specified duplicate options along with a lot of other options,
they will know for which option the error is thrown by the server.

Instead of adding errhint or errdetail, how about just changing the
error message itself to something like "option \"%s\" specified more
than once" or "parameter \"%s\" specified more than once" like we have
in other places in the code?

Both seemed fine but I preferred using errdetail as I felt it is
slightly better for the details to appear in a new line.

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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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 parser_errposition() shows the problem option. That seems
pretty unhelpful, so whenever the problem is the redundancy I would have
the message be explicit about that, and about which option is the
problem:
errmsg("option \"%s\" specified more than once", "someopt")
Do note that wording it this way means only one translatable message,
not dozens.

In some cases it is possible that you'd end up with two messages, one
for "redundant" and one for the other ways for options to conflict with
others; for example collationcmds.c has one that's not as obvious, and
force_quote/force_quote_all in COPY have their own thing too.

I think we should do parser_errposition() wherever possible, in
addition to the wording change.

--
�lvaro Herrera Valdivia, Chile
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
more or less, right?
<crab> i.e., "deadly poison"

On Mon, Apr 26, 2021 at 8:06 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 places have the message "conflicting or redundant options",
and then parser_errposition() shows the problem option. That seems
pretty unhelpful, so whenever the problem is the redundancy I would have
the message be explicit about that, and about which option is the
problem:
errmsg("option \"%s\" specified more than once", "someopt")
Do note that wording it this way means only one translatable message,
not dozens.

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\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location 2) change the message to something like "option
\"%s\" is conflicting with option \"%s\"".

In some cases it is possible that you'd end up with two messages, one
for "redundant" and one for the other ways for options to conflict with
others; for example collationcmds.c has one that's not as obvious, and

And yes, we need to divide up the message for conflicting and
redundant options on a case-to-case basis.

In createdb: we just need to modify the error message to "conflicting
options" or we could just get rid of errdetail and have the error
message directly saying "LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE". Redundant options are just caught in the
above for loop in createdb.
if (dlocale && (dcollate || dctype))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
errdetail("LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE.")));

In AlterDatabase: we can remove parser_errposition because the option
name in the error message would give the right information.
if (list_length(stmt->options) != 1)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("option \"%s\" cannot be specified with
other options",
dtablespace->defname),
parser_errposition(pstate, dtablespace->location)));

In compute_common_attribute: we can remove goto duplicate_error and
have the message change to "option \"%s\" specified more than once".

In DefineType: we need to rework for loop.
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.
CREATE COLLATION case_insensitive (provider = icu, provider =
someother locale = '@colStrength=secondary', deterministic = false,
deterministic = true);

force_quote/force_quote_all in COPY have their own thing too.

We can remove the errhint for force_not_null and force_null along with
the error message wording change to "option \"%s\" specified more than
once".

Upon looking at error "conflicting or redundant options" instances, to
do the above we need a lot of code changes, I'm not sure that will be
acceptable.

One thing is that all the option checking for loops are doing these
things in common: 1) fetching the values bool, int, float, string of
the options 2) redundant checking. I feel we need to invent a common
API to which we pass in 1) a list of allowed options for a particular
command, we can have these as static data structure
{allowed_option_name, data_type}, 2) a list of user specified options
3) the API will return a list of fetched i.e. parsed values
{user_specified_option_name, data_type, value}. Maybe the API can
return a hash table of these values so that the callers can look up
faster for the required option. The advantage of this API is that we
don't need to have many for-loops for options checking in the code.
I'm not sure it is worth doing though. Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location

Hmm, I would keep the parser_errposition() even if the option name is
mentioned in the error message. There's no harm in being a little
redundant, with both the option name and the error cursor showing the
same thing.

2) change the message to something like "option \"%s\" is conflicting
with option \"%s\"".

Maybe, but since these would all be special cases, I think we'd need to
discuss them individually. I would suggest that in order not to stall
this patch, these cases should all stay as "redundant or conflicting
options" -- that is, avoid any further change apart from exactly the
thing you came here to change. You can submit a 0002 patch to change
those other errors. That way, even if those changes end up rejected for
whatever reason, you still got your 0001 done (which would change the
bulk of "conflicting or redundant" error to the "option %s already
specified" error). Some progress is better than none.

--
�lvaro Herrera 39�49'30"S 73�17'W
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 redundant options"
exists: 1) change the message to "option \"%s\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location

Hmm, I would keep the parser_errposition() even if the option name is
mentioned in the error message. There's no harm in being a little
redundant, with both the option name and the error cursor showing the
same thing.

Agreed.

2) change the message to something like "option \"%s\" is conflicting
with option \"%s\"".

Maybe, but since these would all be special cases, I think we'd need to
discuss them individually. I would suggest that in order not to stall
this patch, these cases should all stay as "redundant or conflicting
options" -- that is, avoid any further change apart from exactly the
thing you came here to change. You can submit a 0002 patch to change
those other errors. That way, even if those changes end up rejected for
whatever reason, you still got your 0001 done (which would change the
bulk of "conflicting or redundant" error to the "option %s already
specified" error). Some progress is better than none.

+1 to have all the conflicting options error message changes as 0002
patch or I'm okay even if we discuss those changes after the 0001
patch goes in.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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.
CREATE COLLATION case_insensitive (provider = icu, provider =
someother locale = '@colStrength=secondary', deterministic = false,
deterministic = true);

I'm thinking that the above problem should be discussed separately. I
will start a new thread soon on this.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 27, 2021 at 6:23 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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.
CREATE COLLATION case_insensitive (provider = icu, provider =
someother locale = '@colStrength=secondary', deterministic = false,
deterministic = true);

I'm thinking that the above problem should be discussed separately. I
will start a new thread soon on this.

I started a separate thread -
/messages/by-id/CALj2ACWtL6fTLdyF4R_YkPtf1YEDb6FUoD5DGAki3rpD+sWqiA@mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 redundant options"
exists: 1) change the message to "option \"%s\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location

Hmm, I would keep the parser_errposition() even if the option name is
mentioned in the error message. There's no harm in being a little
redundant, with both the option name and the error cursor showing the
same thing.

2) change the message to something like "option \"%s\" is conflicting
with option \"%s\"".

Maybe, but since these would all be special cases, I think we'd need to
discuss them individually. I would suggest that in order not to stall
this patch, these cases should all stay as "redundant or conflicting
options" -- that is, avoid any further change apart from exactly the
thing you came here to change. You can submit a 0002 patch to change
those other errors. That way, even if those changes end up rejected for
whatever reason, you still got your 0001 done (which would change the
bulk of "conflicting or redundant" error to the "option %s already
specified" error). Some progress is better than none.

Thanks for the comments, please find the attached v3 patch which has
the change for the first part. I will make changes for 002 and post it
soon.
Thoughts?

Regards,
Vignesh

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 Herrera 39�49'30"S 73�17'W
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 that.

Yes, we need to add parser_errposition as agreed in [1]/messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com.

I think we will have to make changes in compute_common_attribute as
well because the error in the duplicate_error goto statement is
actually for the duplicate option specified more than once, we can do
something like the attached. If it seems okay, it can be merged with
the main patch.

[1]: /messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 8:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 that.

Yes, we need to add parser_errposition as agreed in [1].

I think we will have to make changes in compute_common_attribute as
well because the error in the duplicate_error goto statement is
actually for the duplicate option specified more than once, we can do
something like the attached. If it seems okay, it can be merged with
the main patch.

+ DefElem *duplicate_item = NULL;
+
if (strcmp(defel->defname, "volatility") == 0)
{
if (is_procedure)
goto procedure_error;
if (*volatility_item)
- goto duplicate_error;
+ duplicate_item = defel;

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?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbalaut@gmail.com> 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?

Yeah. I can do that. Thanks for the comment.

While on this, I also removed the duplicate_error and procedure_error
goto statements, because IMHO, using goto statements is not an elegant
way. I used boolean flags to do the job instead. See the attached and
let me know what you think.

Just for completion, I also attached Vignesh's latest patch v3 as-is,
in case anybody wants to review it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbalaut@gmail.com> 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?

Yeah. I can do that. Thanks for the comment.

While on this, I also removed the duplicate_error and procedure_error
goto statements, because IMHO, using goto statements is not an elegant
way. I used boolean flags to do the job instead. See the attached and
let me know what you think.

Okay, but I see one side effect of this, basically earlier on
procedure_error and duplicate_error we were not assigning anything to
output parameters, e.g. volatility_item, but now those values will be
assigned with defel even if there is an error. So I think we should
better avoid such change. But even if you want to do then better
check for any impacts on the caller.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbalaut@gmail.com> 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?

Yeah. I can do that. Thanks for the comment.

While on this, I also removed the duplicate_error and procedure_error
goto statements, because IMHO, using goto statements is not an elegant
way. I used boolean flags to do the job instead. See the attached and
let me know what you think.

Okay, but I see one side effect of this, basically earlier on
procedure_error and duplicate_error we were not assigning anything to
output parameters, e.g. volatility_item, but now those values will be
assigned with defel even if there is an error.

Yes, but on ereport(ERROR, we don't come back right? The txn gets
aborted and the control is not returned to the caller instead it will
go to sigjmp_buf of the backend.

So I think we should
better avoid such change. But even if you want to do then better
check for any impacts on the caller.

AFAICS, there will not be any impact on the caller, as the control
doesn't return to the caller on error.

And another good reason to remove the goto statements is that they
have return false; statements just to suppress the compiler and having
them after ereport(ERROR, doesn't make any sense to me.

duplicate_error:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
return false; /* keep compiler quiet */

procedure_error:
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("invalid attribute in procedure definition"),
parser_errposition(pstate, defel->location)));
return false;

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbalaut@gmail.com> 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?

Yeah. I can do that. Thanks for the comment.

While on this, I also removed the duplicate_error and procedure_error
goto statements, because IMHO, using goto statements is not an elegant
way. I used boolean flags to do the job instead. See the attached and
let me know what you think.

Okay, but I see one side effect of this, basically earlier on
procedure_error and duplicate_error we were not assigning anything to
output parameters, e.g. volatility_item, but now those values will be
assigned with defel even if there is an error.

Yes, but on ereport(ERROR, we don't come back right? The txn gets
aborted and the control is not returned to the caller instead it will
go to sigjmp_buf of the backend.

So I think we should
better avoid such change. But even if you want to do then better
check for any impacts on the caller.

AFAICS, there will not be any impact on the caller, as the control
doesn't return to the caller on error.

I see.

other comments

 if (strcmp(defel->defname, "volatility") == 0)
  {
  if (is_procedure)
- goto procedure_error;
+ is_procedure_error =  true;
  if (*volatility_item)
- goto duplicate_error;
+ is_duplicate_error = true;

Another side effect I see is, in the above check earlier if
is_procedure was true it was directly goto procedure_error, but now it
will also check the if (*volatility_item) and it may set
is_duplicate_error also true, which seems wrong to me. I think you
can change it to

if (is_procedure)
is_procedure_error = true;
else if (*volatility_item)
is_duplicate_error = true;

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com