Обсуждение: Small fix on query_id_enabled

Hi,

I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON.

 /* True when compute_query_id is ON, or AUTO and a module requests them */
 bool       query_id_enabled = false;

Should we fix this as following (just fixing the place of a comma) ?

/* True when compute_query_id is ON or AUTO, and a module requests them */

Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.

Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled,  instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?

I attached a patch for above fixes. 

Although renaming might  not be acceptable since changing global variables
may affect third party tools, I think the comment should be fixed at least.

IMHO, it seems better to make this variable static not to be accessed directly.
However, I left it as is because this is used in a static inline function.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

Hi,

On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
>
> I found the comment on query_id_enabled looks inaccurate because this is
> never set to true when compute_query_id is ON.
>
>  /* True when compute_query_id is ON, or AUTO and a module requests them */
>  bool       query_id_enabled = false;
>
> Should we fix this as following (just fixing the place of a comma) ?
>
> /* True when compute_query_id is ON or AUTO, and a module requests them */

Agreed.

> Also, I think the name is a bit confusing for the same reason, that is,
> query_id_enabled may be false even when query id is computed in fact.
>
> Actually, this does not matter because we use IsQueryIdEnabled to check
> if query id is enabled,  instead of referring to a global variable
> (query_id_enabled or compute_query_id). But, just for making a code a bit
> more readable, how about renaming this to query_id_required which seems to
> stand for the meaning more correctly?

-1 for renaming to avoid breaking extensions that might access it.  We should
simply document for compute_query_id and query_id_enabled declaration that one
should instead use IsQueryIdEnabled() if they're interested in whether the core
queryid are computed or not.

On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote:
> On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
>> Also, I think the name is a bit confusing for the same reason, that is,
>> query_id_enabled may be false even when query id is computed in fact.
>>
>> Actually, this does not matter because we use IsQueryIdEnabled to check
>> if query id is enabled,  instead of referring to a global variable
>> (query_id_enabled or compute_query_id). But, just for making a code a bit
>> more readable, how about renaming this to query_id_required which seems to
>> stand for the meaning more correctly?
>
> -1 for renaming to avoid breaking extensions that might access it.  We should
> simply document for compute_query_id and query_id_enabled declaration that one
> should instead use IsQueryIdEnabled() if they're interested in whether the core
> queryid are computed or not.

Agreed.  A renaming would involve more pain than gain.  Improving the
comments around how to all that would be good enough, my 2c.
--
Michael

On Sat, 10 Feb 2024 10:19:15 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote:
> > On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
> >> Also, I think the name is a bit confusing for the same reason, that is,
> >> query_id_enabled may be false even when query id is computed in fact.
> >>
> >> Actually, this does not matter because we use IsQueryIdEnabled to check
> >> if query id is enabled,  instead of referring to a global variable
> >> (query_id_enabled or compute_query_id). But, just for making a code a bit
> >> more readable, how about renaming this to query_id_required which seems to
> >> stand for the meaning more correctly?
> > 
> > -1 for renaming to avoid breaking extensions that might access it.  We should
> > simply document for compute_query_id and query_id_enabled declaration that one
> > should instead use IsQueryIdEnabled() if they're interested in whether the core
> > queryid are computed or not.
> 
> Agreed.  A renaming would involve more pain than gain.  Improving the
> comments around how to all that would be good enough, my 2c.

Thank you both for your comments.

I agreed with not renaming it.

I attached a updated patch that adds comments noting to use IsQueryIdEnabled()
instead of accessing the variables directly.

Regards,
Yugo Nagata
-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote:
> I attached an updated patch that adds comments noting to use IsQueryIdEnabled()
> instead of accessing the variables directly.

Sounds good to me, thanks.
--
Michael

On Tue, Feb 13, 2024 at 05:28:32PM +0900, Michael Paquier wrote:
> On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote:
> > I attached an updated patch that adds comments noting to use IsQueryIdEnabled()
> > instead of accessing the variables directly.
>
> Sounds good to me, thanks.

 +1!

On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote:
>  +1!

Okay, applied as-is, then.
--
Michael

On Wed, 14 Feb 2024 07:21:54 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote:
> >  +1!
> 
> Okay, applied as-is, then.

Thank you!

Regards,
Yugo Nagata

> --
> Michael


-- 
Yugo NAGATA <nagata@sraoss.co.jp>