Thread: Avoid double lookup in pgstat_fetch_stat_tabentry()
Hi hackers, Please find attached a patch proposal to avoid 2 calls to pgstat_fetch_stat_tabentry_ext() in pgstat_fetch_stat_tabentry() in case the relation is not a shared one and no statistics are found. Thanks Andres for the suggestion done in [1]. [1]: https://www.postgresql.org/message-id/20221116201202.3k74ajawyom2c3eq%40awork3.anarazel.de Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Nov 18, 2022 at 10:32 AM Drouvot, Bertrand <bertranddrouvot.pg@gmail.com> wrote: > > Hi hackers, > > Please find attached a patch proposal to avoid 2 calls to > pgstat_fetch_stat_tabentry_ext() in pgstat_fetch_stat_tabentry() in case > the relation is not a shared one and no statistics are found. > > Thanks Andres for the suggestion done in [1]. > > [1]: > https://www.postgresql.org/message-id/20221116201202.3k74ajawyom2c3eq%40awork3.anarazel.de +1. The patch LGTM. However, I have a suggestion to simplify it further by getting rid of the local variable tabentry and just returning pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid);. Furthermore, the pgstat_fetch_stat_tabentry() can just be a static inline function. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 11/18/22 7:06 AM, Bharath Rupireddy wrote: > On Fri, Nov 18, 2022 at 10:32 AM Drouvot, Bertrand > <bertranddrouvot.pg@gmail.com> wrote: >> >> Hi hackers, >> >> Please find attached a patch proposal to avoid 2 calls to >> pgstat_fetch_stat_tabentry_ext() in pgstat_fetch_stat_tabentry() in case >> the relation is not a shared one and no statistics are found. >> >> Thanks Andres for the suggestion done in [1]. >> >> [1]: >> https://www.postgresql.org/message-id/20221116201202.3k74ajawyom2c3eq%40awork3.anarazel.de > > +1. The patch LGTM. Thanks for looking at it! > However, I have a suggestion to simplify it > further by getting rid of the local variable tabentry and just > returning pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), > relid);. Furthermore, the pgstat_fetch_stat_tabentry() can just be a > static inline function. Good point. While at it, why not completely get rid of pgstat_fetch_stat_tabentry_ext(), like in v2 the attached? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Nov 18, 2022 at 3:41 PM Drouvot, Bertrand <bertranddrouvot.pg@gmail.com> wrote: > > > However, I have a suggestion to simplify it > > further by getting rid of the local variable tabentry and just > > returning pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), > > relid);. Furthermore, the pgstat_fetch_stat_tabentry() can just be a > > static inline function. > Good point. While at it, why not completely get rid of > pgstat_fetch_stat_tabentry_ext(), like in v2 the attached? Hm. While it saves around 20 LOC, IsSharedRelation() is now spread across, but WFM. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 2022-11-18 11:09:43 +0100, Drouvot, Bertrand wrote: > > Furthermore, the pgstat_fetch_stat_tabentry() can just be a > > static inline function. I think that's just premature optimization for something like this. The function call overhead on accessing stats can't be a relevant factor - the increase in code size is more likely to matter (but still unlikely). > Good point. While at it, why not completely get rid of > pgstat_fetch_stat_tabentry_ext(), like in v2 the attached? -1, I don't think spreading the IsSharedRelation() is a good idea. It costs more code than it saves. Greetings, Andres Freund
Hi,
On 11/18/22 6:32 PM, Andres Freund wrote:
> Hi,
>
> On 2022-11-18 11:09:43 +0100, Drouvot, Bertrand wrote:
>>> Furthermore, the pgstat_fetch_stat_tabentry() can just be a
>>> static inline function.
>
> I think that's just premature optimization for something like this. The
> function call overhead on accessing stats can't be a relevant factor - the
> increase in code size is more likely to matter (but still unlikely).
>
>
>> Good point. While at it, why not completely get rid of
>> pgstat_fetch_stat_tabentry_ext(), like in v2 the attached?
>
> -1, I don't think spreading the IsSharedRelation() is a good idea. It costs
> more code than it saves.
>
Got it, please find attached V3: switching back to the initial proposal and implementing Bharath's comment (getting rid
ofthe local variable tabentry).
Out of curiosity, here are the sizes (no debug):
- Current code (no patch)
$ size ./src/backend/utils/adt/pgstatfuncs.o ./src/backend/utils/activity/pgstat_relation.o
text data bss dec hex filename
24974 0 0 24974 618e ./src/backend/utils/adt/pgstatfuncs.o
7353 64 0 7417 1cf9 ./src/backend/utils/activity/pgstat_relation.o
- IsSharedRelation() spreading
$ size ./src/backend/utils/adt/pgstatfuncs.o ./src/backend/utils/activity/pgstat_relation.o
text data bss dec hex filename
25304 0 0 25304 62d8 ./src/backend/utils/adt/pgstatfuncs.o
7249 64 0 7313 1c91 ./src/backend/utils/activity/pgstat_relation.o
- inline function
$ size ./src/backend/utils/adt/pgstatfuncs.o ./src/backend/utils/activity/pgstat_relation.o
text data bss dec hex filename
25044 0 0 25044 61d4 ./src/backend/utils/adt/pgstatfuncs.o
7249 64 0 7313 1c91 ./src/backend/utils/activity/pgstat_relation.o
- V3 attached
$ size ./src/backend/utils/adt/pgstatfuncs.o ./src/backend/utils/activity/pgstat_relation.o
text data bss dec hex filename
24974 0 0 24974 618e ./src/backend/utils/adt/pgstatfuncs.o
7323 64 0 7387 1cdb ./src/backend/utils/activity/pgstat_relation.o
I'd vote for V3 for readability, size and "backward compatibility" with current code.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment
Hi, On 2022-11-19 09:38:26 +0100, Drouvot, Bertrand wrote: > I'd vote for V3 for readability, size and "backward compatibility" with current code. Pushed that. Thanks for the patch and evaluation. Greetings, Andres Freund