Re: Additional Chapter for Tutorial

2021-05-20 Thread Jürgen Purtz

Hi Jürgen,

What's going to happen with this work?

If you intend to have it eventually committed, I think it will be 
necessary to make the patches smaller, and bring them into the 
commitfest app, so that others can follow progress.


I for one, cannot see/remember/understand what has been done, or even 
whether you intend to continue with it.


Thanks,

Erik


Peter changed the status to 'Returned with feedback' at the end of the 
last commit fest. I'm not absolutely sure, but my understanding is that 
the patch is rejected.


--

Jürgen Purtz






Re: Additional Chapter for Tutorial

2021-05-20 Thread Erik Rijkers

Hi Jürgen,

What's going to happen with this work?

If you intend to have it eventually committed, I think it will be 
necessary to make the patches smaller, and bring them into the 
commitfest app, so that others can follow progress.


I for one, cannot see/remember/understand what has been done, or even 
whether you intend to continue with it.


Thanks,

Erik




Re: pg_monitor role description

2021-05-20 Thread Pavel Luzanov



On 20.05.2021 21:10, Tom Lane wrote:

Pavel Luzanov  writes:

So, is it correct to change description of pg_monitor role from:
"Read/execute various monitoring views and functions. This role is a member 
of|pg_read_all_settings|,|pg_read_all_stats|  and|pg_stat_scan_tables|."
to
"Read/execute various monitoring views and functions. The roles 
pg_read_all_settings, pg_read_all_stats and pg_stat_scan_tables are members of this 
role."

No, it is not.  That wording implies that the built-in grants are like

GRANT pg_monitor TO pg_read_all_settings

and so on, where the truth is the opposite.


I'm totally confused. I'm taking timeout to think about it.

--

Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company





Re: pg_monitor role description

2021-05-20 Thread Tom Lane
Pavel Luzanov  writes:
> So, is it correct to change description of pg_monitor role from:
> "Read/execute various monitoring views and functions. This role is a member 
> of|pg_read_all_settings|,|pg_read_all_stats|  and|pg_stat_scan_tables|."
> to
> "Read/execute various monitoring views and functions. The roles 
> pg_read_all_settings, pg_read_all_stats and pg_stat_scan_tables are members 
> of this role."

No, it is not.  That wording implies that the built-in grants are like

GRANT pg_monitor TO pg_read_all_settings

and so on, where the truth is the opposite.

regards, tom lane




Re: pg_monitor role description

2021-05-20 Thread Pavel Luzanov

Hello,

On 20.05.2021 20:27, Pavel Luzanov wrote:

So, is it correct to change description of pg_monitor role from:
"Read/execute various monitoring views and functions. This role is a member 
of|pg_read_all_settings|,|pg_read_all_stats|  and|pg_stat_scan_tables|."
to
"Read/execute various monitoring views and functions. The roles 
pg_read_all_settings, pg_read_all_stats and pg_stat_scan_tables are members of this 
role."

I can prepare a simple patch.


Just in case, patch attached.


--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index fe0bdb7599..00338bbebd 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -554,9 +554,9 @@ DROP ROLE doomed_role;
   
pg_monitor
Read/execute various monitoring views and functions.
-   This role is a member of pg_read_all_settings,
-   pg_read_all_stats and
-   pg_stat_scan_tables.
+   The roles pg_read_all_settings,
+   pg_read_all_stats and pg_stat_scan_tables
+   are members of this role.
   
   
pg_database_owner


Re: pg_monitor role description

2021-05-20 Thread Pavel Luzanov

Hello,

On 20.05.2021 19:03, Tom Lane wrote:


Pavel Luzanov  writes:

Let me try one more time.
What is correct from the English language point of view:
1. Julien Rouhaud is a member of PostgreSQL Community.
or
2. PostgreSQL Community is a member of Julien Rouhaud, Michael Paquier.
Or both forms are correct?
I think that 1 is correct.

You're right about that ...


So, is it correct to change description of pg_monitor role from:

"Read/execute various monitoring views and functions. This role is a member 
of|pg_read_all_settings|,|pg_read_all_stats|  and|pg_stat_scan_tables|."

to

"Read/execute various monitoring views and functions. The roles 
pg_read_all_settings, pg_read_all_stats and pg_stat_scan_tables are members of this 
role."

I can prepare a simple patch.


And column header in a \du output must be something like 'members' instead of 
'member of'.

... but this does not follow, because it's a poor analogy.  "Member of"
means "these role(s) have been GRANT'ed to pg_monitor".


Yes, I understood this point and agree.


--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company



Re: pg_monitor role description

2021-05-20 Thread Laurenz Albe
On Thu, 2021-05-20 at 12:03 -0400, Tom Lane wrote:
>  List of roles
>  Role name |  Attributes  |  Member of  
> ---+--+-
>  bob   |  | {sysadmins}
>  joe   |  | {sysadmins}
>  sysadmins | Cannot login | {}
> 
> and I think most would agree that titling the column "Members" would
> be backwards.

Right.  you have to read that like:

Role name (bob) with attributes () is a member of (sysadmins).

Yours,
Laurenz Albe





Re: pg_monitor role description

2021-05-20 Thread Tom Lane
Pavel Luzanov  writes:
> Let me try one more time.
> What is correct from the English language point of view:
> 1. Julien Rouhaud is a member of PostgreSQL Community.
> or
> 2. PostgreSQL Community is a member of Julien Rouhaud, Michael Paquier.
> Or both forms are correct?
> I think that 1 is correct.

You're right about that ...

> And column header in a \du output must be something like 'members' instead of 
> 'member of'.

... but this does not follow, because it's a poor analogy.  "Member of"
means "these role(s) have been GRANT'ed to pg_monitor".

As a more typical use-case, there might be a role "sysadmins" that holds
assorted privileges, and then certain individual users are granted that
role.  Nobody would quibble with seeing

 List of roles
 Role name |  Attributes  |  Member of  
---+--+-
 bob   |  | {sysadmins}
 joe   |  | {sysadmins}
 sysadmins | Cannot login | {}

and I think most would agree that titling the column "Members" would
be backwards.

regards, tom lane




Re: pg_monitor role description

2021-05-20 Thread Pavel Luzanov

On 20.05.2021 11:54, Julien Rouhaud wrote:


On Thu, May 20, 2021 at 3:01 PM Michael Paquier  wrote:

On Thu, May 20, 2021 at 06:11:40AM +, PG Doc comments form wrote:

"This role is a member of pg_read_all_settings, pg_read_all_stats and
pg_stat_scan_tables."
Is it correct sentence?
It seems for me that pg_read_all_stats is a member of pg_monitor. But not
vice versa.

Here is what I am getting:
=# \dgS pg_monitor
   List of roles
  Role name  |  Attributes  | Member of
+--+--
  pg_monitor | Cannot login | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}

=# \dgS pg_read_all_data
 List of roles

 Role name |  Attributes  | Member of
--+--+---
  pg_read_all_data | Cannot login | {}

So the docs look correct to me.

Indeed.  In other words pg_monitor is the sum of the authorizations
given by all those roles.


Let me try one more time.
What is correct from the English language point of view:

1. Julien Rouhaud is a member of PostgreSQL Community.

or

2. PostgreSQL Community is a member of Julien Rouhaud, Michael Paquier.

Or both forms are correct?

I think that 1 is correct. And column header in a \du output must be something 
like 'members' instead of 'member of'.

--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company



Re: pg_monitor role description

2021-05-20 Thread Julien Rouhaud
On Thu, May 20, 2021 at 3:01 PM Michael Paquier  wrote:
>
> On Thu, May 20, 2021 at 06:11:40AM +, PG Doc comments form wrote:
> > "This role is a member of pg_read_all_settings, pg_read_all_stats and
> > pg_stat_scan_tables."
> > Is it correct sentence?
> > It seems for me that pg_read_all_stats is a member of pg_monitor. But not
> > vice versa.
>
> Here is what I am getting:
> =# \dgS pg_monitor
>   List of roles
>  Role name  |  Attributes  | Member of
> +--+--
>  pg_monitor | Cannot login | 
> {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
>
> =# \dgS pg_read_all_data
> List of roles
>
> Role name |  Attributes  | Member of
> --+--+---
>  pg_read_all_data | Cannot login | {}
>
> So the docs look correct to me.

Indeed.  In other words pg_monitor is the sum of the authorizations
given by all those roles.




Re: more detailed description of tup_returned and tup_fetched

2021-05-20 Thread Masahiro Ikeda


On 2021/05/20 17:00, Fujii Masao wrote:
> On 2021/05/20 9:46, Masahiro Ikeda wrote:
>> On 2021/05/18 20:10, Fujii Masao wrote:
> pg_stat_database.tup_fetched:
> Number of index entries returned by scans on indexes in this database
 Is this the sum of pg_stat_all_indexes.idx_tup_read? This is accounted to
 pg_stat_database.tup_returned.
>>>
>>> I was thinking that pg_stat_database.tup_fetched is the same as
>>> the sum of pg_stat_all_tables.idx_tup_fetch. Because they both
>>> are incremented by bitmap index scans, but pg_stat_all_indexes.idx_tup_read
>>> is not.
>>
>> Yes. So, "Number of index entries returned by scans on indexes in this
>> database" is incorrect, and "Number of live rows fetched by index scans in
>> this database" is correct?
> 
> Yes, I think so!

Thanks!
I updated the patch for summarizing this thread.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index dcbb10fb6f..09a22a43ac 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3712,7 +3712,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
tup_returned bigint
   
   
-   Number of rows returned by queries in this database
+   Number of live rows fetched by sequential scans and index entries returned by index scans in this database
   
  
 
@@ -3721,7 +3721,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
tup_fetched bigint
   
   
-   Number of rows fetched by queries in this database
+   Number of live rows fetched by index scans in this database
   
  
 


Re: more detailed description of tup_returned and tup_fetched

2021-05-20 Thread Fujii Masao




On 2021/05/20 9:46, Masahiro Ikeda wrote:



On 2021/05/18 20:10, Fujii Masao wrote:



On 2021/05/18 18:23, Masahiro Ikeda wrote:



On 2021/05/18 16:01, Fujii Masao wrote:

On 2021/05/18 13:20, Masahiro Ikeda wrote:

Tid Range Scan increments the tup_returned, and
pg_stat_all_tables.seq_tup_read is also incremented. I thought it's ok
because
Tid Range Scan is like sequential scan.


Yes, you're right. One interesting thing I found is;
when Tid Range Scan happens, seq_tup_read is incremented
but seq_scan is not. I'm not sure if this is expected behavior or not.


The following comment says that this behavior is expected. But, I agree it's
odd and it's natural both seq_tup_read and seq_scan are incremented at the
same time or not...

/*
   * Currently, we only have a stats counter for sequential heap scans (but
   * e.g for bitmap scans the underlying bitmap index scans will be counted,
   * and for sample scans we update stats for tuple fetches).
   */
if (scan->rs_base.rs_flags & SO_TYPE_SEQSCAN)
 pgstat_count_heap_scan(scan->rs_base.rs_rd);



That's the reason why the document of
pg_stat_all_tables.seq_tup_read says "Number of live rows fetched by
sequential scans"


Regarding the original issue, as far as I understand correctly,

* pg_stat_database.tup_returned = sum(pg_stat_all_tables.seq_tup_read) +
sum(pg_stat_all_indexes.idx_tup_read)
* pg_stat_database.tup_fetched = sum(pg_stat_all_tables.idx_tup_fetch)

But the counters for some system catalogs like pg_database shared
across all databases of a cluster are excluded from that calculation.
Is this my understanding right? If right, probably we can reuse
the existing descriptions for those counters to document
pg_stat_database counters. For example,


Yes, my understanding is same now.



pg_stat_database.tup_returned:> Number of live rows fetched by sequential
and index scans in this database


I wonder "live rows fetched by index scans" may mislead. I think "live" means
it's not dead tuple and "rows" mean the tuple user want to get.

But, pg_stat_all_indexes.idx_tup_read says that "index entires returned by
scans on this index". There is no meaning of "live" and "rows", so I thought
it's better to distinguish them.

So, why don't you change to "Number of live rows fetched by sequential scans
and index entries returned by index scans in this database"?


Yes, LGTM.



pg_stat_database.tup_fetched:
Number of index entries returned by scans on indexes in this database

Is this the sum of pg_stat_all_indexes.idx_tup_read? This is accounted to
pg_stat_database.tup_returned.


I was thinking that pg_stat_database.tup_fetched is the same as
the sum of pg_stat_all_tables.idx_tup_fetch. Because they both
are incremented by bitmap index scans, but pg_stat_all_indexes.idx_tup_read
is not.


Yes. So, "Number of index entries returned by scans on indexes in this
database" is incorrect, and "Number of live rows fetched by index scans in
this database" is correct?


Yes, I think so!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION




Re: pg_monitor role description

2021-05-20 Thread Michael Paquier
On Thu, May 20, 2021 at 06:11:40AM +, PG Doc comments form wrote:
> "This role is a member of pg_read_all_settings, pg_read_all_stats and
> pg_stat_scan_tables."
> Is it correct sentence?
> It seems for me that pg_read_all_stats is a member of pg_monitor. But not
> vice versa.

Here is what I am getting:
=# \dgS pg_monitor
  List of roles
 Role name  |  Attributes  | Member of
+--+--
 pg_monitor | Cannot login | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}

=# \dgS pg_read_all_data
List of roles

Role name |  Attributes  | Member of
--+--+---
 pg_read_all_data | Cannot login | {}

So the docs look correct to me.
--
Michael


signature.asc
Description: PGP signature