Horton

Thread

Commits

  1. pgstat: replace double lookup with IsSharedRelation()

  1. Avoid double lookup in pgstat_fetch_stat_tabentry()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-11-18T05:01:12Z

    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

  2. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-11-18T06:06:34Z

    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

  3. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-11-18T10:09:43Z

    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

  4. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-11-18T11:38:28Z

    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

  5. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Andres Freund <andres@anarazel.de> — 2022-11-18T17:32:10Z

    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

  6. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-11-19T08:38:26Z

    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 of the 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

  7. Re: Avoid double lookup in pgstat_fetch_stat_tabentry()

    Andres Freund <andres@anarazel.de> — 2022-11-20T19:00:12Z

    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