Subject: Re: Generate pg_stat_get_xact*() functions with Macros

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 2 Mar 2023 08:39:14 +0100

Hi,

On 3/1/23 8:54 PM, Gregory Stark (as CFM) wrote:
> Looks like you have a path forward on this and it's not ready to
> commit yet? In which case I'll mark it Waiting on Author?
> 

Yeah, there is some dependencies around this one.

[1]: depends on it
Current one depends of [2], [3] and [4]

Waiting on Author is then the right state, thanks for having moved it to that state.

[1]: https://www.postgresql.org/message-id/flat/[email-masked]
[2]: https://www.postgresql.org/message-id/flat/[email-masked]
[3]: https://www.postgresql.org/message-id/flat/[email-masked]
[4]: https://www.postgresql.org/message-id/flat/[email-masked]

Regards,

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

---

From: Michael Paquier <[email-masked]>
Date: Fri, 24 Mar 2023 09:04:31 +0900

> [4]: https://www.postgresql.org/message-id/flat/[email-masked]

[3] and [4] have been applied.  [2] is more sensitive than it looks,
and [1] for the split of index and table stats can feed on the one of
this thread.

Roma wasn't built in one day, and from what I can see you can still do
some progress with the refactoring of pgstatfuncs.c with what's
already on HEAD.  So how about handling doing that first as much as we
can based on the state of HEAD?  That looks like 50~60% (?) of the
original goal to switch pgstatfuncs.c to use more macros to generate
the definition of all these SQL functions.
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Fri, 24 Mar 2023 06:58:30 +0100

Hi,

On 3/24/23 1:04 AM, Michael Paquier wrote:
> [3] and [4] have been applied.

Thanks for your help on this!

>  [2] is more sensitive than it looks,

I think that's a good idea, so please find enclosed V2 which as compare to V1:

- Does not include the refactoring for pg_stat_get_xact_tuples_inserted(), pg_stat_get_xact_tuples_updated()
and pg_stat_get_xact_tuples_deleted() (as they depend of [2] mentioned above)

- Does not include the refactoring for pg_stat_get_xact_function_total_time(), pg_stat_get_xact_function_self_time(),
pg_stat_get_function_total_time() and pg_stat_get_function_self_time(). I think they can be done in a dedicated commit once
we agree on the renaming for PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that the new macros can match the future agreement.

- Does include the refactoring of the new pg_stat_get_xact_tuples_newpage_updated() function (added in ae4fdde135)

Regards,

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index c7ba86b3dc..e1dd1e0ad3 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1498,50 +1498,42 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
 	return (Datum) 0;
 }
 
-Datum
-pg_stat_get_xact_numscans(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.numscans);
-
-	PG_RETURN_INT64(result);
+#define PG_STAT_GET_XACT_RELENTRY_INT64(stat)			\
+Datum													\
+CppConcat(pg_stat_get_xact_,stat)(PG_FUNCTION_ARGS)		\
+{														\
+	Oid         relid = PG_GETARG_OID(0);				\
+	int64       result;									\
+	PgStat_TableStatus *tabentry;						\
+														\
+	if ((tabentry = find_tabstat_entry(relid)) == NULL)	\
+		result = 0;										\
+	else												\
+		result = (int64) (tabentry->counts.stat);		\
+														\
+	PG_RETURN_INT64(result);							\
 }
 
-Datum
-pg_stat_get_xact_tuples_returned(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
+/* pg_stat_get_xact_numscans */
+PG_STAT_GET_XACT_RELENTRY_INT64(numscans)
 
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.tuples_returned);
+/* pg_stat_get_xact_tuples_returned */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_returned)
 
-	PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_fetched */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_fetched)
 
-Datum
-pg_stat_get_xact_tuples_fetched(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
+/* pg_stat_get_xact_tuples_hot_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_hot_updated)
 
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.tuples_fetched);
+/* pg_stat_get_xact_tuples_newpage_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_newpage_updated)
 
-	PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_blocks_fetched */
+PG_STAT_GET_XACT_RELENTRY_INT64(blocks_fetched)
+
+/* pg_stat_get_xact_blocks_hit */
+PG_STAT_GET_XACT_RELENTRY_INT64(blocks_hit)
 
 Datum
 pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
@@ -1606,66 +1598,6 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64(result);
 }
 
-Datum
-pg_stat_get_xact_tuples_hot_updated(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.tuples_hot_updated);
-
-	PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_newpage_updated(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.tuples_newpage_updated);
-
-	PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_blocks_fetched(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.blocks_fetched);
-
-	PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_blocks_hit(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-		result = (int64) (tabentry->counts.blocks_hit);
-
-	PG_RETURN_INT64(result);
-}
-
 Datum
 pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
 {

---

From: Michael Paquier <[email-masked]>
Date: Sat, 25 Mar 2023 11:50:50 +0900

On Fri, Mar 24, 2023 at 06:58:30AM +0100, Drouvot, Bertrand wrote:
> - Does not include the refactoring for pg_stat_get_xact_tuples_inserted(),
> pg_stat_get_xact_tuples_updated() and pg_stat_get_xact_tuples_deleted() (as
> they depend of [2] mentioned above) 
> 
> - Does not include the refactoring for
> pg_stat_get_xact_function_total_time(),
> pg_stat_get_xact_function_self_time(), 
> pg_stat_get_function_total_time() and
> pg_stat_get_function_self_time(). I think they can be done in a
> dedicated commit once we agree on the renaming for
> PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that
> the new macros can match the future agreement.
> 
> - Does include the refactoring of the new
> - pg_stat_get_xact_tuples_newpage_updated() function (added in
> - ae4fdde135) 

Fine by me.  One step is better than no steps, and this helps:
 1 file changed, 29 insertions(+), 97 deletions(-)

I'll go apply that if there are no objections.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 27 Mar 2023 10:20:34 +0900

>> the new macros can match the future agreement.

Thanks for the reminder.  I have completely missed that this is
mentioned in [1], and that it is all about 8018ffb.  The suggestion to
prefix the macro names with a "_MS" to outline the conversion sounds
like a good one seen from here.  So please find attached a patch to do
this adjustment, completed with a similar switch for the two counters
of the function entries.

> I'll go apply that if there are no objections.

Just did this part to shave a bit more code.

[1]: https://www.postgresql.org/message-id/[email-masked]
--
Michael
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index e1dd1e0ad3..ff155e003f 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -152,29 +152,26 @@ pg_stat_get_function_calls(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64(funcentry->numcalls);
 }
 
-Datum
-pg_stat_get_function_total_time(PG_FUNCTION_ARGS)
-{
-	Oid			funcid = PG_GETARG_OID(0);
-	PgStat_StatFuncEntry *funcentry;
-
-	if ((funcentry = pgstat_fetch_stat_funcentry(funcid)) == NULL)
-		PG_RETURN_NULL();
-	/* convert counter from microsec to millisec for display */
-	PG_RETURN_FLOAT8(((double) funcentry->total_time) / 1000.0);
+/* convert counter from microsec to millisec for display */
+#define PG_STAT_GET_FUNCENTRY_FLOAT8_MS(stat)						\
+Datum																\
+CppConcat(pg_stat_get_function_,stat)(PG_FUNCTION_ARGS)				\
+{																	\
+	Oid			funcid = PG_GETARG_OID(0);							\
+	double		result;												\
+	PgStat_StatFuncEntry *funcentry;								\
+																	\
+	if ((funcentry = pgstat_fetch_stat_funcentry(funcid)) == NULL)	\
+		PG_RETURN_NULL();											\
+	result = ((double) funcentry->stat) / 1000.0;					\
+	PG_RETURN_FLOAT8(result);										\
 }
 
-Datum
-pg_stat_get_function_self_time(PG_FUNCTION_ARGS)
-{
-	Oid			funcid = PG_GETARG_OID(0);
-	PgStat_StatFuncEntry *funcentry;
+/* pg_stat_get_function_total_time */
+PG_STAT_GET_FUNCENTRY_FLOAT8_MS(total_time)
 
-	if ((funcentry = pgstat_fetch_stat_funcentry(funcid)) == NULL)
-		PG_RETURN_NULL();
-	/* convert counter from microsec to millisec for display */
-	PG_RETURN_FLOAT8(((double) funcentry->self_time) / 1000.0);
-}
+/* pg_stat_get_function_self_time */
+PG_STAT_GET_FUNCENTRY_FLOAT8_MS(self_time)
 
 Datum
 pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
@@ -1147,7 +1144,8 @@ pg_stat_get_db_checksum_last_failure(PG_FUNCTION_ARGS)
 		PG_RETURN_TIMESTAMPTZ(result);
 }
 
-#define PG_STAT_GET_DBENTRY_FLOAT8(stat)						\
+/* convert counter from microsec to millisec for display */
+#define PG_STAT_GET_DBENTRY_FLOAT8_MS(stat)						\
 Datum															\
 CppConcat(pg_stat_get_db_,stat)(PG_FUNCTION_ARGS)				\
 {																\
@@ -1164,19 +1162,19 @@ CppConcat(pg_stat_get_db_,stat)(PG_FUNCTION_ARGS)				\
 }
 
 /* pg_stat_get_db_active_time */
-PG_STAT_GET_DBENTRY_FLOAT8(active_time)
+PG_STAT_GET_DBENTRY_FLOAT8_MS(active_time)
 
 /* pg_stat_get_db_blk_read_time */
-PG_STAT_GET_DBENTRY_FLOAT8(blk_read_time)
+PG_STAT_GET_DBENTRY_FLOAT8_MS(blk_read_time)
 
 /* pg_stat_get_db_blk_write_time */
-PG_STAT_GET_DBENTRY_FLOAT8(blk_write_time)
+PG_STAT_GET_DBENTRY_FLOAT8_MS(blk_write_time)
 
 /* pg_stat_get_db_idle_in_transaction_time */
-PG_STAT_GET_DBENTRY_FLOAT8(idle_in_transaction_time)
+PG_STAT_GET_DBENTRY_FLOAT8_MS(idle_in_transaction_time)
 
 /* pg_stat_get_db_session_time */
-PG_STAT_GET_DBENTRY_FLOAT8(session_time)
+PG_STAT_GET_DBENTRY_FLOAT8_MS(session_time)
 
 Datum
 pg_stat_get_bgwriter_timed_checkpoints(PG_FUNCTION_ARGS)

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 27 Mar 2023 07:45:26 +0200

Hi,

> this adjustment, completed with a similar switch for the two counters

Thanks! LGTM, but what about also taking care of pg_stat_get_xact_function_total_time()
and pg_stat_get_xact_function_self_time() while at it?

> Just did this part to shave a bit more code.

Thanks!

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Mon, 27 Mar 2023 15:40:31 +0900

> and pg_stat_get_xact_function_self_time() while at it?

With a macro that uses INSTR_TIME_GET_MILLISEC() to cope with
instr_time?  Why not, that's one duplication less.
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 27 Mar 2023 08:54:13 +0200

> instr_time?  Why not, that's one duplication less.

Yes, something like V1 up-thread was doing. I think it can be added with your current proposal.

Regards,