Message-ID: <768032ee-fb57-494b-b674-1ccb65b6f969@oss.nttdata.com>
Date: Fri, 26 Jul 2024 14:09:26 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: 'Katsuragi Yuta' <katsuragiy@oss.nttdata.com>,
 'vignesh C' <vignesh21@gmail.com>, 'Ted Yu' <yuzhihong@gmail.com>,
 'Tom Lane' <tgl@sss.pgh.pa.us>,
 "'pgsql-hackers@lists.postgresql.org'" <pgsql-hackers@lists.postgresql.org>,
 =?UTF-8?B?J8OWbmRlciBLYWxhY8SxJw==?= <onderkalaci@gmail.com>,
 "'Shinya11.Kato@oss.nttdata.com'" <Shinya11.Kato@oss.nttdata.com>,
 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>,
 'Shubham Khanna' <khannashubham1197@gmail.com>
References: 
 <TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB5866DBFEC61E81CCE9F67142F5B99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <b492a935-6c7e-8c08-e485-3c1d64d7d10f@oss.nttdata.com>
 <TYAPR01MB5866D498F593B4AD7D63C148F5939@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CAHv8RjJvxDc2U3xUfvBwirw9Wk4oq8FVggv1VUhXNw6zQU8VZQ@mail.gmail.com>
 <OS3PR01MB9882E5D54A472AB0A5A098D8F58EA@OS3PR01MB9882.jpnprd01.prod.outlook.com>
 <be9382f7-5072-4760-8b3f-31d6dffa8d62@oss.nttdata.com>
 <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com>
 <613adbf9-c86e-41dc-ab97-27dc4e7281c7@oss.nttdata.com>
 <TYCPR01MB56933FCF37A676B868362575F5AA2@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <d04aae8d-05f5-42f4-a263-b962334d9f75@oss.nttdata.com>
 <TYCPR01MB569331643314B5129312C332F5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <TYCPR01MB56934DE73B0AF7A37D8CF7B2F5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: 
 <TYCPR01MB56934DE73B0AF7A37D8CF7B2F5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/768032ee-fb57-494b-b674-1ccb65b6f969%40oss.nttdata.com>
Precedence: bulk


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