Re: [Proposal] Add foreign-server health checks infrastructure

On 2024/07/26 12:15, Hayato Kuroda (Fujitsu) wrote:
> Agreed to add the table. I ran a proofreading tool, and it said below points.
> You can revise if they are acceptable.

Yes, I'm okay with these changes. Thanks for the review!


>> I’ve also made several code improvements, for example adding a typedef for
>> pgfdwVersion to simplify its usage, and updated typedefs.list.
>>
>> +enum pgfdwVersion
>> +{
>> +    PGFDW_V1_0 = 0,
>> +    PGFDW_V1_2,
>> +}            pgfdwVersion;
> 
> It was intentionally not added, because while developing pg_createsubscriber,
> I got a comment that local-use data have not have to be typedef'd [1]:

I didn't know about the rule regarding local-use enums without typedef,
as there are examples like pgssVersion and pgssStoreKind in pg_stat_statements.c.
However, I guess that keeping typedefs.list small is better.
So, I'm fine with removing the typedef from that enum definition
and updating typedefs.list accordingly.


> ```
> +Datum
> +postgres_fdw_get_connections_1_2(PG_FUNCTION_ARGS)
> +{
> +    postgres_fdw_get_connections_internal(fcinfo, PGFDW_V1_2);
> +
> +    return (Datum) 0;
> +}
> ```
> 
> I know they have a same meaning, but can you clarify why (Datun) 0
> is returned instead of PG_RETURN_VOID()?

You're right. I made the change for readability, as similar
functions in pg_stat_statements are implemented that way.
However, it's not essential. I'm okay with reverting it.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION