Message-ID: <83f7982d-272a-b064-8a0f-8853354edf10@gmail.com>
Date: Fri, 13 Jan 2023 10:36:49 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Generate pg_stat_get_xact*() functions with Macros
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael.paquier@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <89606d96-cd94-af74-18f3-c7ab2b684ba2@gmail.com>
 <20230111225907.6el6c5j3hukizqxc@awork3.anarazel.de>
 <e6191040-40c4-6849-7b5d-bf3c227f1755@gmail.com>
 <20230112182434.ejffbun4plv2g37k@awork3.anarazel.de>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <20230112182434.ejffbun4plv2g37k@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/83f7982d-272a-b064-8a0f-8853354edf10%40gmail.com>
Precedence: bulk

Hi,

On 1/12/23 7:24 PM, Andres Freund wrote:
> Hi,
> 
> On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote:
>> On 1/11/23 11:59 PM, Andres Freund wrote:
>>>> Now that this patch renames some fields
>>>
>>> I don't mind renaming the fields - the prefixes really don't provide anything
>>> useful. But it's not clear why this is related to this patch? You could just
>>> include the f_ prefix in the macro, no?
>>>
>>>
>>
>> Right, but the idea is to take the same approach that the one used in 8018ffbf58 (where placing the prefixes in the macro
>> would have been possible too).
> 
> I'm not super happy about that patch tbh.
> 
> 
>>> Probably should remove PgStat_BackendFunctionEntry.
>>
>> I think that would be a 3rd patch, agree?
> 
> Yep.
> 
> 
> 
>>> I now see that PG_STAT_GET_DBENTRY_FLOAT8 already exists, defined the same
>>> way. But the name fields misleading enough that I'd be inclined to rename it?
>>>
>>
>> PG_STAT_GET_FUNCENTRY_FLOAT8_MS looks good by me. Waiting on what we'll decide for
>> the existing PG_STAT_GET_DBENTRY_FLOAT8 (so that I can align for the PG_STAT_GET_FUNCENTRY_FLOAT8).
> 
> +1
> 
> 
> 
>>> Although I suspect this actually hints at an architectural thing that could be
>>> fixed better: Perhaps we should replace find_tabstat_entry() with a version
>>> returning a fully "reconciled" PgStat_StatTabEntry?  It feels quite wrong to
>>> have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c
>>> and about how the different counts get combined.
>>>
>>> I think that'd allow us to move the definition of PgStat_TableStatus to
>>> PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a
>>> heck of a lot cleaner.
>>
>> Yeah, I think that would be for a 4th patch, agree?
> 
> Yea. I am of multiple minds about the ordering. I can see benefits on fixing
> the architectural issue before reducing duplication in the accessor with a
> macro. The reason is that if we addressed the architectural issue, the
> difference between the xact and non-xact version will be very minimal, and
> could even be generated by the same macro.
> 

Yeah, I do agree and I'm in favor of this ordering:

1) replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry
2) remove prefixes
3) Introduce the new macros

And it looks to me that removing PgStat_BackendFunctionEntry can be done independently

I'll first look at 1).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com