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