Wrong comment in statscmds.c/CreateStatistics?

Started by Peter Smithover 3 years ago2 messages
#1Peter Smith
smithpb2250@gmail.com

I happened to notice the following code in
src/backend/commands/statscmds.c, CreateStatistics:

======
/*
* Parse the statistics kinds.
*
* First check that if this is the case with a single expression, there
* are no statistics kinds specified (we don't allow that for the simple
* CREATE STATISTICS form).
*/
if ((list_length(stmt->exprs) == 1) && (list_length(stxexprs) == 1))
{
/* statistics kinds not specified */
if (list_length(stmt->stat_types) > 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("when building statistics on a single expression, statistics
kinds may not be specified")));
}
======

AFAICT that one-line comment (/* statistics kinds not specified */) is
wrong because at that point we don't yet know if kinds are specified
or not.

SUGGESTION-1
Change the comment to /* Check there are no statistics kinds specified */

SUGGESTION-2
Simply remove that one-line comment because the larger comment seems
to be saying the same thing anyhow.

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

#2Junwang Zhao
zhjwpku@gmail.com
In reply to: Peter Smith (#1)
Re: Wrong comment in statscmds.c/CreateStatistics?

Yeah, the comments are kind of confusing, see some comments inline.

On Tue, Aug 16, 2022 at 8:47 AM Peter Smith <smithpb2250@gmail.com> wrote:

I happened to notice the following code in
src/backend/commands/statscmds.c, CreateStatistics:

======
/*
* Parse the statistics kinds.
*
* First check that if this is the case with a single expression, there
* are no statistics kinds specified (we don't allow that for the simple

maybe change to *there should be no* is better?

* CREATE STATISTICS form).
*/
if ((list_length(stmt->exprs) == 1) && (list_length(stxexprs) == 1))
{
/* statistics kinds not specified */

remove this line or change to *statistics kinds should not be specified*,
I prefer just removing it.

if (list_length(stmt->stat_types) > 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("when building statistics on a single expression, statistics
kinds may not be specified")));

change *may* to *should*?

}
======

AFAICT that one-line comment (/* statistics kinds not specified */) is
wrong because at that point we don't yet know if kinds are specified
or not.

SUGGESTION-1
Change the comment to /* Check there are no statistics kinds specified */

SUGGESTION-2
Simply remove that one-line comment because the larger comment seems
to be saying the same thing anyhow.

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

--
Regards
Junwang Zhao