naming of async_mode parameter

Started by Zhihong Yuover 4 years ago4 messages
#1Zhihong Yu
zyu@yugabyte.com

Hi, Etsuro-san:
I was looking at
Fix EXPLAIN ANALYZE for async-capable nodes.

which adds the following parameter / field:

+ bool async_mode; /* true if node is in async mode */

async_mode implies an enum: {sync, async}
Since there are only two values, the data type is bool. I think it should
be named is_async.

What do you think?

Cheers

#2Etsuro Fujita
etsuro.fujita@gmail.com
In reply to: Zhihong Yu (#1)
Re: naming of async_mode parameter

Hi,

On Thu, May 13, 2021 at 2:23 AM Zhihong Yu <zyu@yugabyte.com> wrote:

I was looking at
Fix EXPLAIN ANALYZE for async-capable nodes.

Thanks for that!

which adds the following parameter / field:

+ bool async_mode; /* true if node is in async mode */

async_mode implies an enum: {sync, async}
Since there are only two values, the data type is bool. I think it should be named is_async.

By async_mode, I mean "is in async mode?", as commented above. I
thought the naming is_in_async_mode would be a bit long, so I
shortened it to async_mode. IIUC, I think another example in our
codebase would be the hash_spill_mode parameter in the AggState
struct. So I think async_mode would be acceptable IMO.

Best regards,
Etsuro Fujita

#3Zhihong Yu
zyu@yugabyte.com
In reply to: Etsuro Fujita (#2)
Re: naming of async_mode parameter

On Fri, May 14, 2021 at 1:05 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

Hi,

On Thu, May 13, 2021 at 2:23 AM Zhihong Yu <zyu@yugabyte.com> wrote:

I was looking at
Fix EXPLAIN ANALYZE for async-capable nodes.

Thanks for that!

which adds the following parameter / field:

+ bool async_mode; /* true if node is in async mode */

async_mode implies an enum: {sync, async}
Since there are only two values, the data type is bool. I think it

should be named is_async.

By async_mode, I mean "is in async mode?", as commented above. I
thought the naming is_in_async_mode would be a bit long, so I
shortened it to async_mode. IIUC, I think another example in our
codebase would be the hash_spill_mode parameter in the AggState
struct. So I think async_mode would be acceptable IMO.

Best regards,
Etsuro Fujita

Hi,
Searching postgres codebase reveals the following (partial) examples:

bool is_varlena
bool is_leaf

I think these are more intuitive.

If you think is_in_async_mode is too long, how about naming the parameter
is_async ?

If you agree, I can send out a patch.

Cheers

#4Etsuro Fujita
etsuro.fujita@gmail.com
In reply to: Zhihong Yu (#3)
Re: naming of async_mode parameter

Hi,

On Fri, May 14, 2021 at 9:00 PM Zhihong Yu <zyu@yugabyte.com> wrote:

Searching postgres codebase reveals the following (partial) examples:

bool is_varlena
bool is_leaf

I think these are more intuitive.

If you think is_in_async_mode is too long, how about naming the parameter is_async ?

Sorry, I don’t think we need to rename the parameter, because I think
async_mode is also a good name for it, and the naming follows that of
existing parameters in our codebase, such as hash_spill_mode and
csv_mode in the CopyFormatOptions struct.

I think this would be just a matter of preference.

Best regards,
Etsuro Fujita