public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] Clarify that COMMENT and SECURITY LABEL can be set to NULL in the synopses
20+ messages / 5 participants
[nested] [flat]
* [PATCH v2] Clarify that COMMENT and SECURITY LABEL can be set to NULL in the synopses
@ 2023-01-31 16:05 Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2023-01-31 16:05 UTC (permalink / raw)
This was only mentioned in the description of the text/label, which
are marked as being in quotes in the synopsis, which can cause
confusion (as witnessed on IRC).
Also separate the literal and NULL case in the parameter list, per
suggestion from Tom Lane.
---
doc/src/sgml/ref/comment.sgml | 16 ++++++++++++----
doc/src/sgml/ref/security_label.sgml | 16 ++++++++++++----
2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml
index 7499da1d62..470a6d0b5c 100644
--- a/doc/src/sgml/ref/comment.sgml
+++ b/doc/src/sgml/ref/comment.sgml
@@ -66,7 +66,7 @@
TRIGGER <replaceable class="parameter">trigger_name</replaceable> ON <replaceable class="parameter">table_name</replaceable> |
TYPE <replaceable class="parameter">object_name</replaceable> |
VIEW <replaceable class="parameter">object_name</replaceable>
-} IS '<replaceable class="parameter">text</replaceable>'
+} IS { '<replaceable class="parameter">text</replaceable>' | NULL }
<phrase>where <replaceable>aggregate_signature</replaceable> is:</phrase>
@@ -263,11 +263,19 @@
</varlistentry>
<varlistentry>
- <term><replaceable class="parameter">text</replaceable></term>
+ <term><literal>'</literal><replaceable class="parameter">text</replaceable><literal>'</literal></term>
<listitem>
<para>
- The new comment, written as a string literal; or <literal>NULL</literal>
- to drop the comment.
+ The new comment, written as a string literal.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>NULL</literal></term>
+ <listitem>
+ <para>
+ Write <literal>NULL</literal> to drop the comment.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/security_label.sgml b/doc/src/sgml/ref/security_label.sgml
index 20a839ff0c..e4eee77932 100644
--- a/doc/src/sgml/ref/security_label.sgml
+++ b/doc/src/sgml/ref/security_label.sgml
@@ -44,7 +44,7 @@
TABLESPACE <replaceable class="parameter">object_name</replaceable> |
TYPE <replaceable class="parameter">object_name</replaceable> |
VIEW <replaceable class="parameter">object_name</replaceable>
-} IS '<replaceable class="parameter">label</replaceable>'
+} IS { '<replaceable class="parameter">label</replaceable>' | NULL }
<phrase>where <replaceable>aggregate_signature</replaceable> is:</phrase>
@@ -178,11 +178,19 @@
</varlistentry>
<varlistentry>
- <term><replaceable class="parameter">label</replaceable></term>
+ <term><literal>'</literal><replaceable class="parameter">label</replaceable><literal>'</literal></term>
<listitem>
<para>
- The new security label, written as a string literal; or <literal>NULL</literal>
- to drop the security label.
+ The new security label, written as a string literal.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>NULL</literal>></term>
+ <listitem>
+ <para>
+ Write <literal>NULL</literal> to drop the security label.
</para>
</listitem>
</varlistentry>
--
2.34.1
--=-=-=--
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-03-16 10:32 Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Drouvot, Bertrand @ 2023-03-16 10:32 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 3/16/23 7:29 AM, Michael Paquier wrote:
> On Mon, Mar 06, 2023 at 08:33:15AM +0100, Drouvot, Bertrand wrote:
>> Thanks for having looked at it!
>
> Looking at that, I have a few comments.
>
> + tabentry = (PgStat_TableStatus *) entry_ref->pending;
> + tablestatus = palloc(sizeof(PgStat_TableStatus));
> + *tablestatus = *tabentry;
> +
> [...]
> + for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
> + {
> + tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
> + tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
> + tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
> + }
>
> - if (entry_ref)
> - return entry_ref->pending;
> - return NULL;
> + return tablestatus;
>
> From what I get with this change, the number of tuples changed by DMLs
> have their computations done a bit earlier,
Thanks for looking at it!
Right, but note this is in a dedicated new tablestatus (created within find_tabstat_entry()).
> meaning that it would make
> all the callers of find_tabstat_entry() pay the computation cost.
Right. Another suggested approach was to add a flag but then we'd not really
hide the internal from pgstatfuncs.
> Still it is not really going to matter, because we will just do the
> computation once when looking at any pending changes of
> pg_stat_xact_all_tables for each entry.
Yes.
> There are 9 callers of
> find_tabstat_entry, with 7 being used for pg_stat_xact_all_tables.
Right, those are:
pg_stat_get_xact_numscans()
pg_stat_get_xact_tuples_returned()
pg_stat_get_xact_tuples_fetched()
pg_stat_get_xact_tuples_inserted()
pg_stat_get_xact_tuples_updated()
pg_stat_get_xact_tuples_deleted()
pg_stat_get_xact_tuples_hot_updated()
> How much do we need to care about the remaining two callers
> pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()?
Regarding pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()
the callers (if any) are outside of the core PG (as from what I can see they are not used
at all).
I don't think we should pay any particular attention to those 2 ones as anyway nothing
prevent the 7 others to be called outside of the pg_stat_xact_all_tables view.
> Could it be a problem if these two also pay the extra computation cost
> if a transaction with many subtransactions (aka )needs to look at their
> data? These two are used nowhere, they have pg_proc entries and they
> are undocumented, so it is hard to say the impact of this change on
> them..
>
Right, and that's the same for the 7 others as nothing prevent them to be called outside
of the pg_stat_xact_all_tables view.
Do you think it would be better to add the extra flag then?
> Second question: once the data from the subtransactions is copied,
> would it be cleaner to set trans to NULL after the data copy is done?
>
That would not hurt but I'm not sure it's worth it (well, it's currently
not done in pg_stat_get_xact_tuples_inserted() for example).
> It would feel a bit safer to me to document that find_tabstat_entry()
> is currently only used for this xact system view.. The extra
> computation could lead to surprises, actually, if this routine is used
> outside this context? Perhaps that's OK, but it does not give me a
> warm feeling, just to reshape three functions of pgstatfuncs.c with
> macros.
That's a fair point. On the other hand those 9 functions (which can all be used outside
of the pg_stat_xact_all_tables view) are not documented, so I'm not sure this is that much of
a concern (and if we think it is we still gave the option to add an extra flag to indicate whether
or not the extra computation is needed.)
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-03-16 11:46 ` Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-03-16 11:46 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Mar 16, 2023 at 11:32:56AM +0100, Drouvot, Bertrand wrote:
> On 3/16/23 7:29 AM, Michael Paquier wrote:
>> From what I get with this change, the number of tuples changed by DMLs
>> have their computations done a bit earlier,
>
> Thanks for looking at it!
>
> Right, but note this is in a dedicated new tablestatus (created
> within find_tabstat_entry()).
Sure, however it copies the pointer of the PgStat_TableXactStatus from
tabentry, isn't it? This means that it keeps a reference of the chain
of subtransactions. It does not matter for the functions but it could
for out-of-core callers of find_tabstat_entry(), no? Perhaps you are
right and that's not worth worrying, still I don't feel particularly
confident that this is the best approach we can take.
>> How much do we need to care about the remaining two callers
>> pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()?
>
> Regarding pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()
> the callers (if any) are outside of the core PG (as from what I can
> see they are not used at all).
>
> I don't think we should pay any particular attention to those 2 ones
> as anyway nothing prevent the 7 others to be called outside of the
> pg_stat_xact_all_tables view.
I am not quite sure, TBH. Did you look at the difference with a long
chain of subtrans, like savepoints? The ODBC driver "loves" producing
a lot of savepoints, for example.
>> It would feel a bit safer to me to document that find_tabstat_entry()
>> is currently only used for this xact system view.. The extra
>> computation could lead to surprises, actually, if this routine is used
>> outside this context? Perhaps that's OK, but it does not give me a
>> warm feeling, just to reshape three functions of pgstatfuncs.c with
>> macros.
>
> That's a fair point. On the other hand those 9 functions (which can
> all be used outside of the pg_stat_xact_all_tables view) are not
> documented, so I'm not sure this is that much of a concern (and if
> we think it is we still gave the option to add an extra flag to
> indicate whether or not the extra computation is needed.)
That's not quite exact, I think. The first 7 functions are used in a
system catalog that is documented. Still we have a problem here. I
can actually see a few projects relying on these two functions while
looking a bit around, so they are used. And the issue comes from
ddfc2d9, that has removed these functions from the documentation
ignoring that they are used in no system catalogs. I think that we
should fix that and re-add the two missing functions with a proper
description in the docs, at least? There is no trace of them.
Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
not listed.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZBMBh5ve3tyqH5%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-03-16 13:13 ` Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Drouvot, Bertrand @ 2023-03-16 13:13 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On 3/16/23 12:46 PM, Michael Paquier wrote:
> On Thu, Mar 16, 2023 at 11:32:56AM +0100, Drouvot, Bertrand wrote:
>> On 3/16/23 7:29 AM, Michael Paquier wrote:
>>> From what I get with this change, the number of tuples changed by DMLs
>>> have their computations done a bit earlier,
>>
>> Thanks for looking at it!
>>
>> Right, but note this is in a dedicated new tablestatus (created
>> within find_tabstat_entry()).
>
> Sure, however it copies the pointer of the PgStat_TableXactStatus from
> tabentry, isn't it?
Oh I see what you mean, yeah, the pointer is copied.
> This means that it keeps a reference of the chain
> of subtransactions. It does not matter for the functions but it could
> for out-of-core callers of find_tabstat_entry(), no?
Yeah, maybe.
> Perhaps you are
> right and that's not worth worrying, still I don't feel particularly
> confident that this is the best approach we can take.
>
due to what potential out-of-core callers could do with it?
>>> How much do we need to care about the remaining two callers
>>> pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()?
>>
>> Regarding pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()
>> the callers (if any) are outside of the core PG (as from what I can
>> see they are not used at all).
>>
>> I don't think we should pay any particular attention to those 2 ones
>> as anyway nothing prevent the 7 others to be called outside of the
>> pg_stat_xact_all_tables view.
>
> I am not quite sure, TBH. Did you look at the difference with a long
> chain of subtrans, like savepoints? The ODBC driver "loves" producing
> a lot of savepoints, for example.
>
No, I did not measure the impact.
>>> It would feel a bit safer to me to document that find_tabstat_entry()
>>> is currently only used for this xact system view.. The extra
>>> computation could lead to surprises, actually, if this routine is used
>>> outside this context? Perhaps that's OK, but it does not give me a
>>> warm feeling, just to reshape three functions of pgstatfuncs.c with
>>> macros.
>>
>> That's a fair point. On the other hand those 9 functions (which can
>> all be used outside of the pg_stat_xact_all_tables view) are not
>> documented, so I'm not sure this is that much of a concern (and if
>> we think it is we still gave the option to add an extra flag to
>> indicate whether or not the extra computation is needed.)
>
> That's not quite exact, I think. The first 7 functions are used in a
> system catalog that is documented.
Right.
> Still we have a problem here. I
> can actually see a few projects relying on these two functions while
> looking a bit around, so they are used. And the issue comes from
> ddfc2d9, that has removed these functions from the documentation
> ignoring that they are used in no system catalogs. I think that we
> should fix that and re-add the two missing functions with a proper
> description in the docs, at least?
As they could be/are used outside of the xact view, yes I think the same.
> There is no trace of them.
> Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
> not listed.
I'd be tempted to add documentation for all of them, I can look at it.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-03-19 23:43 ` Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-03-19 23:43 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote:
> On 3/16/23 12:46 PM, Michael Paquier wrote:
>> There is no trace of them.
>> Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
>> not listed.
>
> I'd be tempted to add documentation for all of them, I can look at it.
I am not sure that there is any need to completely undo ddfc2d9, later
simplified by 5f2b089, so my opinion would be to just add
documentation for the functions that can be used but are used in none
of the system functions.
Anyway, double-checking, I only see an inconsistency for these two,
confirming my first impression:
- pg_stat_get_xact_blocks_fetched
- pg_stat_get_xact_blocks_hit
There may be a point in having them in some of the system views, but
the non-xact flavors are only used in the statio views, which don't
really need xact versions AFAIK. I am not sure that it makes much
sense to add them in pg_stat_xact_all_tables, either. Another view is
just remove them, though some could rely on them externally. At the
end, documenting both still sounds like the best move to me.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-03-20 10:57 ` Drouvot, Bertrand <[email protected]>
2023-03-22 01:16 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-22 18:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Drouvot, Bertrand @ 2023-03-20 10:57 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 3/20/23 12:43 AM, Michael Paquier wrote:
> At the
> end, documenting both still sounds like the best move to me.
Agree.
Please find attached v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch doing so.
I did not put the exact same wording as the one being removed in ddfc2d9, as:
- For pg_stat_get_xact_blocks_hit(), I think it's better to be closer to say the
pg_statio_all_tables.heap_blks_hit definition.
- For pg_stat_get_xact_blocks_fetched(), I think that using "buffer" is better (than block) as at the
end it's related to pgstat_count_buffer_read().
At the end there is a choice to be made for both for the wording between block and buffer. Indeed their
counters get incremented in "buffer" macros while retrieved in those "blocks" functions.
"Buffer" sounds more appropriate to me, so the attached has been done that way.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6249bb50d0..fba8e892cf 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5697,6 +5697,32 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</para></entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_stat_get_xact_blocks_fetched</primary>
+ </indexterm>
+ <function>pg_stat_get_xact_blocks_fetched</function> ( <type>oid</type> )
+ <returnvalue>bigint</returnvalue>
+ </para>
+ <para>
+ Returns the number of buffer fetches for table or index, in the current transaction
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_stat_get_xact_blocks_hit</primary>
+ </indexterm>
+ <function>pg_stat_get_xact_blocks_hit</function> ( <type>oid</type> )
+ <returnvalue>bigint</returnvalue>
+ </para>
+ <para>
+ Returns the number of buffer hits for table or index, in the current transaction
+ </para></entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
Attachments:
[text/plain] v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch (1.3K, ../../[email protected]/2-v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch)
download | inline diff:
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6249bb50d0..fba8e892cf 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5697,6 +5697,32 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</para></entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_stat_get_xact_blocks_fetched</primary>
+ </indexterm>
+ <function>pg_stat_get_xact_blocks_fetched</function> ( <type>oid</type> )
+ <returnvalue>bigint</returnvalue>
+ </para>
+ <para>
+ Returns the number of buffer fetches for table or index, in the current transaction
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_stat_get_xact_blocks_hit</primary>
+ </indexterm>
+ <function>pg_stat_get_xact_blocks_hit</function> ( <type>oid</type> )
+ <returnvalue>bigint</returnvalue>
+ </para>
+ <para>
+ Returns the number of buffer hits for table or index, in the current transaction
+ </para></entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-03-22 01:16 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Michael Paquier @ 2023-03-22 01:16 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Mon, Mar 20, 2023 at 11:57:31AM +0100, Drouvot, Bertrand wrote:
> "Buffer" sounds more appropriate to me, so the attached has been done that way.
This choice is OK for me.
> + <indexterm>
> + <primary>pg_stat_get_xact_blocks_fetched</primary>
> + </indexterm>
> + <function>pg_stat_get_xact_blocks_fetched</function> ( <type>oid</type> )
> + <returnvalue>bigint</returnvalue>
> + </para>
> + <para>
> + Returns the number of buffer fetches for table or index, in the current transaction
This should be "number of buffer fetched", no?
> + </indexterm>
> + <function>pg_stat_get_xact_blocks_hit</function> ( <type>oid</type> )
> + <returnvalue>bigint</returnvalue>
> + </para>
> + <para>
> + Returns the number of buffer hits for table or index, in the current transaction
> + </para></entry>
This one looks OK to me too.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-03-22 18:21 ` Melanie Plageman <[email protected]>
2023-03-22 22:42 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Melanie Plageman @ 2023-03-22 18:21 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Mon, Mar 20, 2023 at 6:58 AM Drouvot, Bertrand
<[email protected]> wrote:
>
> Hi,
>
> On 3/20/23 12:43 AM, Michael Paquier wrote:
> > At the
> > end, documenting both still sounds like the best move to me.
>
> Agree.
>
> Please find attached v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch doing so.
>
> I did not put the exact same wording as the one being removed in ddfc2d9, as:
>
> - For pg_stat_get_xact_blocks_hit(), I think it's better to be closer to say the
> pg_statio_all_tables.heap_blks_hit definition.
>
> - For pg_stat_get_xact_blocks_fetched(), I think that using "buffer" is better (than block) as at the
> end it's related to pgstat_count_buffer_read().
>
> At the end there is a choice to be made for both for the wording between block and buffer. Indeed their
> counters get incremented in "buffer" macros while retrieved in those "blocks" functions.
>
> "Buffer" sounds more appropriate to me, so the attached has been done that way.
Apologies as I know this docs update has already been committed, but
buffers fetched and blocks fetched both feel weird to me. If you have a
cache hit, you don't end up really "fetching" anything at all (since
pgstat_count_buffer_read() is called before ReadBuffer_common() and we
don't know if it is a hit or miss yet). And, I would normally associate
fetching with fetching a block into a buffer. It seems like this counter
is really reflecting the number of buffers acquired or used.
tuples_fetched makes more sense because a tuple is "fetched" into a
slot.
This isn't really the fault of this patch since that member was already
called blocks_fetched.
- Melanie
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-22 18:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
@ 2023-03-22 22:42 ` Michael Paquier <[email protected]>
2023-03-27 14:24 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-03-22 22:42 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Wed, Mar 22, 2023 at 02:21:12PM -0400, Melanie Plageman wrote:
> Apologies as I know this docs update has already been committed, but
> buffers fetched and blocks fetched both feel weird to me. If you have a
> cache hit, you don't end up really "fetching" anything at all (since
> pgstat_count_buffer_read() is called before ReadBuffer_common() and we
> don't know if it is a hit or miss yet). And, I would normally associate
> fetching with fetching a block into a buffer. It seems like this counter
> is really reflecting the number of buffers acquired or used.
Well, it is the number of times we've requested a block read, though
it may not actually be a read if the block was in the cache already.
> This isn't really the fault of this patch since that member was already
> called blocks_fetched.
The original documentation of these functions added by 46aa77c refers
to "block fetch requests" and "block requests found in cache", so that
would not be right either based on your opinion here. If you find
"fetch" to be incorrect in this context, here is another idea:
- "block read requests" for blocks_fetched().
- "block read requested but actually found in cache" for blocks_hit().
All the system views care only about the difference between both
counters to count the number of physical reads really done.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-22 18:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
2023-03-22 22:42 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-03-27 14:24 ` Melanie Plageman <[email protected]>
2023-03-27 22:38 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Melanie Plageman @ 2023-03-27 14:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Wed, Mar 22, 2023 at 6:42 PM Michael Paquier <[email protected]> wrote:
>
> On Wed, Mar 22, 2023 at 02:21:12PM -0400, Melanie Plageman wrote:
> > Apologies as I know this docs update has already been committed, but
> > buffers fetched and blocks fetched both feel weird to me. If you have a
> > cache hit, you don't end up really "fetching" anything at all (since
> > pgstat_count_buffer_read() is called before ReadBuffer_common() and we
> > don't know if it is a hit or miss yet). And, I would normally associate
> > fetching with fetching a block into a buffer. It seems like this counter
> > is really reflecting the number of buffers acquired or used.
>
> Well, it is the number of times we've requested a block read, though
> it may not actually be a read if the block was in the cache already.
>
> > This isn't really the fault of this patch since that member was already
> > called blocks_fetched.
>
> The original documentation of these functions added by 46aa77c refers
> to "block fetch requests" and "block requests found in cache", so that
> would not be right either based on your opinion here. If you find
> "fetch" to be incorrect in this context, here is another idea:
> - "block read requests" for blocks_fetched().
> - "block read requested but actually found in cache" for blocks_hit().
I do like/prefer "block read requests" and
"blocks requested found in cache"
Though, now I fear my initial complaint may have been a bit pedantic.
- Melanie
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-22 18:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
2023-03-22 22:42 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-27 14:24 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
@ 2023-03-27 22:38 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Michael Paquier @ 2023-03-27 22:38 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Mon, Mar 27, 2023 at 10:24:46AM -0400, Melanie Plageman wrote:
> I do like/prefer "block read requests" and
> "blocks requested found in cache"
> Though, now I fear my initial complaint may have been a bit pedantic.
That's fine. Let's ask for extra opinions, then.
So, have others an opinion to share here?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-03-25 02:56 ` Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-03-25 02:56 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote:
> On 3/16/23 12:46 PM, Michael Paquier wrote:
>>> I don't think we should pay any particular attention to those 2 ones
>>> as anyway nothing prevent the 7 others to be called outside of the
>>> pg_stat_xact_all_tables view.
>>
>> I am not quite sure, TBH. Did you look at the difference with a long
>> chain of subtrans, like savepoints? The ODBC driver "loves" producing
>> a lot of savepoints, for example.
>
> No, I did not measure the impact.
I have been thinking again about this particular point, and I would be
fine with an additional boolean flag to compute the subtrans data in
the global counter when only necessary. This would not make the
macros for the stat functions that much more complicated, while being
sure that we don't do unnecessary computations when we know that we
don't need them..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZB5i1lt3v%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-03-25 03:00 ` Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2023-03-25 03:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 2023-03-25 11:56:22 +0900, Michael Paquier wrote:
> On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote:
> > On 3/16/23 12:46 PM, Michael Paquier wrote:
> >>> I don't think we should pay any particular attention to those 2 ones
> >>> as anyway nothing prevent the 7 others to be called outside of the
> >>> pg_stat_xact_all_tables view.
> >>
> >> I am not quite sure, TBH. Did you look at the difference with a long
> >> chain of subtrans, like savepoints? The ODBC driver "loves" producing
> >> a lot of savepoints, for example.
> >
> > No, I did not measure the impact.
>
> I have been thinking again about this particular point, and I would be
> fine with an additional boolean flag to compute the subtrans data in
> the global counter when only necessary. This would not make the
> macros for the stat functions that much more complicated, while being
> sure that we don't do unnecessary computations when we know that we
> don't need them..
I don't understand what we're optimizing for here. These functions are very
very very far from being a hot path. The xact functions are barely ever
used. Compared to the cost of query evaluation the cost of iterating throught
he subxacts is neglegible.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
@ 2023-03-27 06:35 ` Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-03-27 06:35 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Fri, Mar 24, 2023 at 08:00:44PM -0700, Andres Freund wrote:
> I don't understand what we're optimizing for here. These functions are very
> very very far from being a hot path. The xact functions are barely ever
> used. Compared to the cost of query evaluation the cost of iterating throught
> he subxacts is neglegible.
I was wondering about that, and I see why I'm wrong. I have quickly
gone up to 10k subtransactions, and while I was seeing what looks like
difference of 8~10% in runtime when looking at
pg_stat_xact_all_tables, the overval runtime was still close enough
(5.8ms vs 6.4ms). At this scale, possible that it was some noise,
these seemed repeatable still not to worry about.
Anyway, I was looking at this patch, and I still feel that it is a bit
incorrect to have the copy of PgStat_TableStatus returned by
find_tabstat_entry() to point to the same list of subtransaction data
as the pending entry found, while the counters are incremented. This
could lead to mistakes if the copy from find_tabstat_entry() is used
in an unexpected way in the future. The current callers are OK, but
this does not give me a warm feeling :/
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-10-26 08:04 ` Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Drouvot, Bertrand @ 2023-10-26 08:04 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 3/27/23 8:35 AM, Michael Paquier wrote:
> On Fri, Mar 24, 2023 at 08:00:44PM -0700, Andres Freund wrote:
>> I don't understand what we're optimizing for here. These functions are very
>> very very far from being a hot path. The xact functions are barely ever
>> used. Compared to the cost of query evaluation the cost of iterating throught
>> he subxacts is neglegible.
>
> I was wondering about that, and I see why I'm wrong. I have quickly
> gone up to 10k subtransactions, and while I was seeing what looks like
> difference of 8~10% in runtime when looking at
> pg_stat_xact_all_tables, the overval runtime was still close enough
> (5.8ms vs 6.4ms). At this scale, possible that it was some noise,
> these seemed repeatable still not to worry about.
>
> Anyway, I was looking at this patch, and I still feel that it is a bit
> incorrect to have the copy of PgStat_TableStatus returned by
> find_tabstat_entry() to point to the same list of subtransaction data
> as the pending entry found, while the counters are incremented. This
> could lead to mistakes if the copy from find_tabstat_entry() is used
> in an unexpected way in the future. The current callers are OK, but
> this does not give me a warm feeling :/
FWIW, please find attached V7 (mandatory rebase).
It would allow to also define:
- pg_stat_get_xact_tuples_inserted
- pg_stat_get_xact_tuples_updated
- pg_stat_get_xact_tuples_deleted
as macros, joining others pg_stat_get_xact_*() that are already
defined as macros.
The concern you raised above has not been addressed, meaning that
find_tabstat_entry() still return a copy of PgStat_TableStatus.
By "used in an unexpected way in the future", what do you mean exactly? Do you mean
the caller forgetting it is working on a copy and then could work with
"stale" counters?
Trying to understand to see if I should invest time to try to address your concern
or leave those 3 functions as they are currently before moving back to the
"Split index and table statistics into different types of stats" work [1].
[1]: https://www.postgresql.org/message-id/flat/[email protected]
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From ea025820b30f8421ebe4ea9cf9c19f6fe653118a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v7] Reconcile stats in find_tabstat_entry()
---
src/backend/utils/activity/pgstat_relation.c | 32 +++++++++-
src/backend/utils/adt/pgstatfuncs.c | 66 ++------------------
2 files changed, 35 insertions(+), 63 deletions(-)
38.8% src/backend/utils/activity/
61.1% src/backend/utils/adt/
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 9876e0c1e8..8ebef877d0 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -478,20 +478,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->counts.tuples_inserted += trans->tuples_inserted;
+ tablestatus->counts.tuples_updated += trans->tuples_updated;
+ tablestatus->counts.tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3b44af8006..01362ae688 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1600,68 +1600,14 @@ 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)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
+/* pg_stat_get_xact_tuples_inserted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted)
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_inserted;
- /* live subtransactions' counts aren't in tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+/* pg_stat_get_xact_tuples_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated)
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_updated;
- /* live subtransactions' counts aren't in tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
-
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_deleted;
- /* live subtransactions' counts aren't in tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
-
- PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_deleted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted)
Datum
pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
--
2.34.1
Attachments:
[text/plain] v7-0001-Reconcile-stats-in-find_tabstat_entry.patch (4.6K, ../../[email protected]/2-v7-0001-Reconcile-stats-in-find_tabstat_entry.patch)
download | inline diff:
From ea025820b30f8421ebe4ea9cf9c19f6fe653118a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v7] Reconcile stats in find_tabstat_entry()
---
src/backend/utils/activity/pgstat_relation.c | 32 +++++++++-
src/backend/utils/adt/pgstatfuncs.c | 66 ++------------------
2 files changed, 35 insertions(+), 63 deletions(-)
38.8% src/backend/utils/activity/
61.1% src/backend/utils/adt/
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 9876e0c1e8..8ebef877d0 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -478,20 +478,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->counts.tuples_inserted += trans->tuples_inserted;
+ tablestatus->counts.tuples_updated += trans->tuples_updated;
+ tablestatus->counts.tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3b44af8006..01362ae688 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1600,68 +1600,14 @@ 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)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
+/* pg_stat_get_xact_tuples_inserted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted)
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_inserted;
- /* live subtransactions' counts aren't in tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+/* pg_stat_get_xact_tuples_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated)
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_updated;
- /* live subtransactions' counts aren't in tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
-
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_deleted;
- /* live subtransactions' counts aren't in tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
-
- PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_deleted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted)
Datum
pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
--
2.34.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-10-27 06:07 ` Michael Paquier <[email protected]>
2023-10-27 07:45 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-10-27 06:07 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Oct 26, 2023 at 10:04:25AM +0200, Drouvot, Bertrand wrote:
> By "used in an unexpected way in the future", what do you mean exactly? Do you mean
> the caller forgetting it is working on a copy and then could work with
> "stale" counters?
(Be careful about the code indentation.)
The part that I found disturbing is here:
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
This causes tablestatus->trans to point to the same location as
tabentry->trans, but wouldn't it be better to set tablestatus->trans
to NULL instead for the copy returned to the caller?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-10-27 07:45 ` Drouvot, Bertrand <[email protected]>
2023-10-27 07:50 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-29 23:25 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Drouvot, Bertrand @ 2023-10-27 07:45 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 10/27/23 8:07 AM, Michael Paquier wrote:
>
> The part that I found disturbing is here:
> + tabentry = (PgStat_TableStatus *) entry_ref->pending;
> + tablestatus = palloc(sizeof(PgStat_TableStatus));
> + *tablestatus = *tabentry;
>
> This causes tablestatus->trans to point to the same location as
> tabentry->trans, but wouldn't it be better to set tablestatus->trans
> to NULL instead for the copy returned to the caller?
Oh I see, yeah I do agree to set tablestatus->trans to NULL to avoid
any undesired interference with tabentry->trans.
Done in V8 attached (pgindent has been run on pgstatfuncs.c and
pgstat_relation.c).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 9f69363296058bd473efa50f8d8b995627f0dbdf Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v8] Reconcile stats in find_tabstat_entry()
---
src/backend/utils/activity/pgstat_relation.c | 38 ++++++++++-
src/backend/utils/adt/pgstatfuncs.c | 66 ++------------------
2 files changed, 41 insertions(+), 63 deletions(-)
42.3% src/backend/utils/activity/
57.6% src/backend/utils/adt/
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 9876e0c1e8..4c694d925d 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -478,20 +478,52 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if (!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * We don't want tablestatus->trans to point to the same location as
+ * tabentry->trans as that could lead to undesired behavior.
+ */
+ tablestatus->trans = NULL;
+
+ /*
+ * Live subtransactions' counts aren't in counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->counts.tuples_inserted += trans->tuples_inserted;
+ tablestatus->counts.tuples_updated += trans->tuples_updated;
+ tablestatus->counts.tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 6468b6a805..998c69e241 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1588,68 +1588,14 @@ 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)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
+/* pg_stat_get_xact_tuples_inserted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted)
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_inserted;
- /* live subtransactions' counts aren't in tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+/* pg_stat_get_xact_tuples_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated)
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_updated;
- /* live subtransactions' counts aren't in tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
-
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_deleted;
- /* live subtransactions' counts aren't in tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
-
- PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_deleted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted)
Datum
pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
--
2.34.1
Attachments:
[text/plain] v8-0001-Reconcile-stats-in-find_tabstat_entry.patch (4.7K, ../../[email protected]/2-v8-0001-Reconcile-stats-in-find_tabstat_entry.patch)
download | inline diff:
From 9f69363296058bd473efa50f8d8b995627f0dbdf Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v8] Reconcile stats in find_tabstat_entry()
---
src/backend/utils/activity/pgstat_relation.c | 38 ++++++++++-
src/backend/utils/adt/pgstatfuncs.c | 66 ++------------------
2 files changed, 41 insertions(+), 63 deletions(-)
42.3% src/backend/utils/activity/
57.6% src/backend/utils/adt/
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 9876e0c1e8..4c694d925d 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -478,20 +478,52 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if (!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * We don't want tablestatus->trans to point to the same location as
+ * tabentry->trans as that could lead to undesired behavior.
+ */
+ tablestatus->trans = NULL;
+
+ /*
+ * Live subtransactions' counts aren't in counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->counts.tuples_inserted += trans->tuples_inserted;
+ tablestatus->counts.tuples_updated += trans->tuples_updated;
+ tablestatus->counts.tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 6468b6a805..998c69e241 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1588,68 +1588,14 @@ 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)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
+/* pg_stat_get_xact_tuples_inserted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted)
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_inserted;
- /* live subtransactions' counts aren't in tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+/* pg_stat_get_xact_tuples_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated)
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_updated;
- /* live subtransactions' counts aren't in tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
-
- PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
-{
- Oid relid = PG_GETARG_OID(0);
- int64 result;
- PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
-
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
- result = 0;
- else
- {
- result = tabentry->counts.tuples_deleted;
- /* live subtransactions' counts aren't in tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
-
- PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_deleted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted)
Datum
pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
--
2.34.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-27 07:45 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-10-27 07:50 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Michael Paquier @ 2023-10-27 07:50 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote:
> Oh I see, yeah I do agree to set tablestatus->trans to NULL to avoid
> any undesired interference with tabentry->trans.
>
> Done in V8 attached (pgindent has been run on pgstatfuncs.c and
> pgstat_relation.c).
LGTM.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-27 07:45 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
@ 2023-10-29 23:25 ` Michael Paquier <[email protected]>
2023-11-08 04:24 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2023-10-29 23:25 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote:
> Done in V8 attached (pgindent has been run on pgstatfuncs.c and
> pgstat_relation.c).
And applied that after editing a bit the comments.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-27 07:45 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-29 23:25 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
@ 2023-11-08 04:24 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Andres Freund @ 2023-11-08 04:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On 2023-10-30 08:25:17 +0900, Michael Paquier wrote:
> On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote:
> > Done in V8 attached (pgindent has been run on pgstatfuncs.c and
> > pgstat_relation.c).
>
> And applied that after editing a bit the comments.
Thank you both!
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2023-11-08 04:24 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 16:05 [PATCH v2] Clarify that COMMENT and SECURITY LABEL can be set to NULL in the synopses Dagfinn Ilmari Mannsåker <[email protected]>
2023-03-16 10:32 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 11:46 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-16 13:13 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-19 23:43 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-20 10:57 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-22 01:16 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-22 18:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
2023-03-22 22:42 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-27 14:24 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Melanie Plageman <[email protected]>
2023-03-27 22:38 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 02:56 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-03-25 03:00 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-27 06:35 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-26 08:04 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 06:07 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-27 07:45 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-10-27 07:50 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-10-29 23:25 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2023-11-08 04:24 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox