Thread: redundant check of msg in does_not_exist_skipping
Hi,
I was looking at commit aca992040951c7665f1701cd25d48808eda7a809
I think the check of msg after the switch statement is not necessary. The variable msg is used afterward.
If there is (potential) missing case in switch statement, the compiler would warn.
How about removing the check ?
Thanks
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index db906f530e..55996940eb 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -518,9 +518,6 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
/* no default, to let compiler warn about missing case */
}
- if (!msg)
- elog(ERROR, "unrecognized object type: %d", (int) objtype);
-
if (!args)
ereport(NOTICE, (errmsg(msg, name)));
else
index db906f530e..55996940eb 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -518,9 +518,6 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
/* no default, to let compiler warn about missing case */
}
- if (!msg)
- elog(ERROR, "unrecognized object type: %d", (int) objtype);
-
if (!args)
ereport(NOTICE, (errmsg(msg, name)));
else
On Thu, 17 Nov 2022 at 20:12, Ted Yu <yuzhihong@gmail.com> wrote:
> Hi,
> I was looking at commit aca992040951c7665f1701cd25d48808eda7a809
>
> I think the check of msg after the switch statement is not necessary. The
> variable msg is used afterward.
> If there is (potential) missing case in switch statement, the compiler
> would warn.
>
> How about removing the check ?
>
I think we cannot remove the check, for example, if objtype is OBJECT_OPFAMILY,
and schema_does_not_exist_skipping() returns true, the so the msg keeps NULL,
if we remove this check, a sigfault might be occurd in ereport().
case OBJECT_OPFAMILY:
{
List *opfname = list_copy_tail(castNode(List, object), 1);
if (!schema_does_not_exist_skipping(opfname, &msg, &name))
{
msg = gettext_noop("operator family \"%s\" does not exist for access method \"%s\", skipping");
name = NameListToString(opfname);
args = strVal(linitial(castNode(List, object)));
}
}
break;
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Thu, 17 Nov 2022 at 23:06, Japin Li <japinli@hotmail.com> wrote:
> On Thu, 17 Nov 2022 at 20:12, Ted Yu <yuzhihong@gmail.com> wrote:
>> Hi,
>> I was looking at commit aca992040951c7665f1701cd25d48808eda7a809
>>
>> I think the check of msg after the switch statement is not necessary. The
>> variable msg is used afterward.
>> If there is (potential) missing case in switch statement, the compiler
>> would warn.
>>
>> How about removing the check ?
>>
>
> I think we cannot remove the check, for example, if objtype is OBJECT_OPFAMILY,
> and schema_does_not_exist_skipping() returns true, the so the msg keeps NULL,
> if we remove this check, a sigfault might be occurd in ereport().
>
> case OBJECT_OPFAMILY:
> {
> List *opfname = list_copy_tail(castNode(List, object), 1);
>
> if (!schema_does_not_exist_skipping(opfname, &msg, &name))
> {
> msg = gettext_noop("operator family \"%s\" does not exist for access method \"%s\", skipping");
> name = NameListToString(opfname);
> args = strVal(linitial(castNode(List, object)));
> }
> }
> break;
Sorry, I didn't look into schema_does_not_exist_skipping(), and after look
into schema_does_not_exist_skipping function and others, all paths that go
out switch branch has non-NULL for msg, so we can remove this check safely.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Japin Li <japinli@hotmail.com> writes:
> On Thu, 17 Nov 2022 at 23:06, Japin Li <japinli@hotmail.com> wrote:
>> I think we cannot remove the check, for example, if objtype is OBJECT_OPFAMILY,
>> and schema_does_not_exist_skipping() returns true, the so the msg keeps NULL,
>> if we remove this check, a sigfault might be occurd in ereport().
> Sorry, I didn't look into schema_does_not_exist_skipping(), and after look
> into schema_does_not_exist_skipping function and others, all paths that go
> out switch branch has non-NULL for msg, so we can remove this check safely.
This is a completely bad idea. If it takes that level of analysis
to see that msg can't be null, we should leave the test in place.
Any future modification of either this code or what it calls could
break the conclusion.
regards, tom lane
On Thu, Nov 17, 2022 at 10:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > This is a completely bad idea. If it takes that level of analysis > to see that msg can't be null, we should leave the test in place. > Any future modification of either this code or what it calls could > break the conclusion. +1. Also, even if the check were quite obviously useless, it's cheap insurance. It's difficult to believe that it hurts performance in any measurable way. If anything, we would benefit from having more sanity checks in our code, rather than fewer. -- Robert Haas EDB: http://www.enterprisedb.com
> On Nov 18, 2022, at 4:04 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Nov 17, 2022 at 10:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> This is a completely bad idea. If it takes that level of analysis >> to see that msg can't be null, we should leave the test in place. >> Any future modification of either this code or what it calls could >> break the conclusion. > > +1. Also, even if the check were quite obviously useless, it's cheap > insurance. It's difficult to believe that it hurts performance in any > measurable way. If anything, we would benefit from having more sanity > checks in our code, rather than fewer. > Thanks for the explanation! Got it.