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]/messages/by-id/20221116201202.3k74ajawyom2c3eq@awork3.anarazel.de.
[1]: /messages/by-id/20221116201202.3k74ajawyom2c3eq@awork3.anarazel.de
/messages/by-id/20221116201202.3k74ajawyom2c3eq@awork3.anarazel.de
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchtext/plain; charset=UTF-8; name=v1-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchDownload
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 55a355f583..a2a4fc6e2c 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -25,6 +25,7 @@
#include "utils/pgstat_internal.h"
#include "utils/rel.h"
#include "utils/timestamp.h"
+#include "catalog/catalog.h"
/* Record that's written to 2PC state file when pgstat state is persisted */
@@ -439,14 +440,8 @@ pgstat_fetch_stat_tabentry(Oid relid)
{
PgStat_StatTabEntry *tabentry;
- tabentry = pgstat_fetch_stat_tabentry_ext(false, relid);
- if (tabentry != NULL)
- return tabentry;
+ tabentry = pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid);
- /*
- * If we didn't find it, maybe it's a shared table.
- */
- tabentry = pgstat_fetch_stat_tabentry_ext(true, relid);
return tabentry;
}
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]:
/messages/by-id/20221116201202.3k74ajawyom2c3eq@awork3.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]:
/messages/by-id/20221116201202.3k74ajawyom2c3eq@awork3.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
Attachments:
v2-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchtext/plain; charset=UTF-8; name=v2-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchDownload
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..fde8458f5e 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2101,8 +2101,8 @@ do_autovacuum(void)
/* Fetch reloptions and the pgstat entry for this table */
relopts = extract_autovac_opts(tuple, pg_class_desc);
- tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared,
- relid);
+ tabentry = pgstat_fetch_stat_tabentry(classForm->relisshared,
+ relid);
/* Check if it needs vacuum or analyze */
relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
@@ -2185,8 +2185,8 @@ do_autovacuum(void)
}
/* Fetch the pgstat entry for this table */
- tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared,
- relid);
+ tabentry = pgstat_fetch_stat_tabentry(classForm->relisshared,
+ relid);
relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
effective_multixact_freeze_max_age,
@@ -2913,8 +2913,8 @@ recheck_relation_needs_vacanalyze(Oid relid,
PgStat_StatTabEntry *tabentry;
/* fetch the pgstat table entry */
- tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared,
- relid);
+ tabentry = pgstat_fetch_stat_tabentry(classForm->relisshared,
+ relid);
relation_needs_vacanalyze(relid, avopts, classForm, tabentry,
effective_multixact_freeze_max_age,
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 55a355f583..266fa0c15e 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -61,8 +61,8 @@ pgstat_copy_relation_stats(Relation dst, Relation src)
PgStatShared_Relation *dstshstats;
PgStat_EntryRef *dst_ref;
- srcstats = pgstat_fetch_stat_tabentry_ext(src->rd_rel->relisshared,
- RelationGetRelid(src));
+ srcstats = pgstat_fetch_stat_tabentry(src->rd_rel->relisshared,
+ RelationGetRelid(src));
if (!srcstats)
return;
@@ -435,27 +435,7 @@ pgstat_update_heap_dead_tuples(Relation rel, int delta)
* caller is better off to report ZERO instead.
*/
PgStat_StatTabEntry *
-pgstat_fetch_stat_tabentry(Oid relid)
-{
- PgStat_StatTabEntry *tabentry;
-
- tabentry = pgstat_fetch_stat_tabentry_ext(false, relid);
- if (tabentry != NULL)
- return tabentry;
-
- /*
- * If we didn't find it, maybe it's a shared table.
- */
- tabentry = pgstat_fetch_stat_tabentry_ext(true, relid);
- return tabentry;
-}
-
-/*
- * More efficient version of pgstat_fetch_stat_tabentry(), allowing to specify
- * whether the to-be-accessed table is a shared relation or not.
- */
-PgStat_StatTabEntry *
-pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
+pgstat_fetch_stat_tabentry(bool shared, Oid reloid)
{
Oid dboid = (shared ? InvalidOid : MyDatabaseId);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ae3365d917..8eed606702 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -31,6 +31,7 @@
#include "utils/builtins.h"
#include "utils/inet.h"
#include "utils/timestamp.h"
+#include "catalog/catalog.h"
#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var))))
@@ -43,7 +44,7 @@ pg_stat_get_numscans(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->numscans);
@@ -59,7 +60,7 @@ pg_stat_get_lastscan(PG_FUNCTION_ARGS)
TimestampTz result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = tabentry->lastscan;
@@ -78,7 +79,7 @@ pg_stat_get_tuples_returned(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_returned);
@@ -94,7 +95,7 @@ pg_stat_get_tuples_fetched(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_fetched);
@@ -110,7 +111,7 @@ pg_stat_get_tuples_inserted(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_inserted);
@@ -126,7 +127,7 @@ pg_stat_get_tuples_updated(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_updated);
@@ -142,7 +143,7 @@ pg_stat_get_tuples_deleted(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_deleted);
@@ -158,7 +159,7 @@ pg_stat_get_tuples_hot_updated(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->tuples_hot_updated);
@@ -174,7 +175,7 @@ pg_stat_get_live_tuples(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->n_live_tuples);
@@ -190,7 +191,7 @@ pg_stat_get_dead_tuples(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->n_dead_tuples);
@@ -206,7 +207,7 @@ pg_stat_get_mod_since_analyze(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->changes_since_analyze);
@@ -222,7 +223,7 @@ pg_stat_get_ins_since_vacuum(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->inserts_since_vacuum);
@@ -238,7 +239,7 @@ pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->blocks_fetched);
@@ -254,7 +255,7 @@ pg_stat_get_blocks_hit(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->blocks_hit);
@@ -269,7 +270,7 @@ pg_stat_get_last_vacuum_time(PG_FUNCTION_ARGS)
TimestampTz result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = tabentry->vacuum_timestamp;
@@ -287,7 +288,7 @@ pg_stat_get_last_autovacuum_time(PG_FUNCTION_ARGS)
TimestampTz result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = tabentry->autovac_vacuum_timestamp;
@@ -305,7 +306,7 @@ pg_stat_get_last_analyze_time(PG_FUNCTION_ARGS)
TimestampTz result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = tabentry->analyze_timestamp;
@@ -323,7 +324,7 @@ pg_stat_get_last_autoanalyze_time(PG_FUNCTION_ARGS)
TimestampTz result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = tabentry->autovac_analyze_timestamp;
@@ -341,7 +342,7 @@ pg_stat_get_vacuum_count(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->vacuum_count);
@@ -356,7 +357,7 @@ pg_stat_get_autovacuum_count(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->autovac_vacuum_count);
@@ -371,7 +372,7 @@ pg_stat_get_analyze_count(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->analyze_count);
@@ -386,7 +387,7 @@ pg_stat_get_autoanalyze_count(PG_FUNCTION_ARGS)
int64 result;
PgStat_StatTabEntry *tabentry;
- if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
+ if ((tabentry = pgstat_fetch_stat_tabentry(IsSharedRelation(relid), relid)) == NULL)
result = 0;
else
result = (int64) (tabentry->autovac_analyze_count);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 9e2ce6f011..10ae025988 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -569,9 +569,8 @@ extern void pgstat_twophase_postcommit(TransactionId xid, uint16 info,
extern void pgstat_twophase_postabort(TransactionId xid, uint16 info,
void *recdata, uint32 len);
-extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(Oid relid);
-extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry_ext(bool shared,
- Oid reloid);
+extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(bool shared,
+ Oid reloid);
extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id);
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 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
Attachments:
v3-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchtext/plain; charset=UTF-8; name=v3-0001-pgstat_fetch_stat_tabentry-avoid_double_lookup.patchDownload
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 55a355f583..f92e16e7af 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -25,6 +25,7 @@
#include "utils/pgstat_internal.h"
#include "utils/rel.h"
#include "utils/timestamp.h"
+#include "catalog/catalog.h"
/* Record that's written to 2PC state file when pgstat state is persisted */
@@ -437,17 +438,7 @@ pgstat_update_heap_dead_tuples(Relation rel, int delta)
PgStat_StatTabEntry *
pgstat_fetch_stat_tabentry(Oid relid)
{
- PgStat_StatTabEntry *tabentry;
-
- tabentry = pgstat_fetch_stat_tabentry_ext(false, relid);
- if (tabentry != NULL)
- return tabentry;
-
- /*
- * If we didn't find it, maybe it's a shared table.
- */
- tabentry = pgstat_fetch_stat_tabentry_ext(true, relid);
- return tabentry;
+ return pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid);
}
/*