shared-memory based stats collector

On Fri, Jun 29, 2018 at 4:34 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Nowadays PostgreSQL has dynamic shared hash (dshash) so we can
use this as the main storage of statistics. We can share data
without a stress using this.

A PoC previously posted tried to use "locally copied" dshash but
it doesn't looks fine so I steered to different direction.

With this patch dshash can create a local copy based on dynhash.

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello. Thanks for the comment.

At Mon, 2 Jul 2018 14:25:58 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoYQhr30eAcgJCi1v0FhA+3RP1FZVnXqSTLe=6fHy9e5oA@mail.gmail.com>

On Fri, Jun 29, 2018 at 4:34 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Nowadays PostgreSQL has dynamic shared hash (dshash) so we can
use this as the main storage of statistics. We can share data
without a stress using this.

A PoC previously posted tried to use "locally copied" dshash but
it doesn't looks fine so I steered to different direction.

With this patch dshash can create a local copy based on dynhash.

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

The consistency here means "repeatable read" of an object's stats
entry, not a snapshot covering all objects. We don't need to copy
all the entries at once following this definition. The attached
version makes a cache entry only for requested objects.

Addition to that vacuum doesn't require even repeatable read
consistency so we don't need to cache the entries at all.
backend_get_tab_entry now returns an isolated (that means
not-stored-in-hash) palloc'ed copy without making a local copy in
the case.

As the result, this version behaves as the follows.

- Stats collector stores the results in shared memory.

- In backend, cache is created only for requested objects and
lasts for the transaction.

- Vacuum directly reads the shared stats and doesn't create a
local copy.

The non-behavioral difference from the v1 is the follows.

- snapshot feature of dshash is removed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. This is new version fixed windows build.

At Tue, 03 Jul 2018 19:01:44 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20180703.190144.222427588.horiguchi.kyotaro@lab.ntt.co.jp>

Hello. Thanks for the comment.

At Mon, 2 Jul 2018 14:25:58 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoYQhr30eAcgJCi1v0FhA+3RP1FZVnXqSTLe=6fHy9e5oA@mail.gmail.com>

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

The consistency here means "repeatable read" of an object's stats
entry, not a snapshot covering all objects. We don't need to copy
all the entries at once following this definition. The attached
version makes a cache entry only for requested objects.

Addition to that vacuum doesn't require even repeatable read
consistency so we don't need to cache the entries at all.
backend_get_tab_entry now returns an isolated (that means
not-stored-in-hash) palloc'ed copy without making a local copy in
the case.

As the result, this version behaves as the follows.

- Stats collector stores the results in shared memory.

- In backend, cache is created only for requested objects and
lasts for the transaction.

- Vacuum directly reads the shared stats and doesn't create a
local copy.

The non-behavioral difference from the v1 is the follows.

- snapshot feature of dshash is removed.

This version includes some additional patches. 0003 removes
PG_STAT_TMP_DIR and it affects pg_stat_statements, pg_basebackup
and pg_rewind. Among them pg_stat_statements gets build failure
because it uses the directory to save query texts. 0005 is a new
patch and moves the file to the permanent stat directory. With
this change pg_basebackup and pg_rewind no longer ignore the
query text file.

I haven't explicitly mentioned that, but
dynamic_shared_memory_type = none prevents server from
starting. This patch is not providing a fallback path for the
case. I'm expecting that 'none' will be removed in v12.

v3-0001-sequential-scan-for-dshash.patch
- Functionally same with v2. got cosmetic changes.

v3-0002-Change-stats-collector-to-an-axiliary-process.patch
- Fixed for Windows build.

v3-0003-dshash-based-stats-collector.patch
- Cosmetic changes from v2.

v3-0004-Documentation-update.patch
- New patch in v3 of documentation edits.

v3-0005-Let-pg_stat_statements-not-to-use-PG_STAT_TMP_DIR.patch
- New patch of tentative change of pg_stat_statements.

v3-0006-Remove-pg_stat_tmp-exclusion-from-pg_resetwal.patch
- New patch of tentative change of pg_rewind.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Mon, 2 Jul 2018 14:25:58 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoYQhr30eAcgJCi1v0FhA+3RP1FZVnXqSTLe=6fHy9e5oA@mail.gmail.com>

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

The consistency here means "repeatable read" of an object's stats
entry, not a snapshot covering all objects. We don't need to copy
all the entries at once following this definition. The attached
version makes a cache entry only for requested objects.

Uh, what? That's basically destroying the long-standing semantics of
statistics snapshots. I do not think we can consider that acceptable.
As an example, it would mean that scan counts for indexes would not
match up with scan counts for their tables.

regards, tom lane

Hello.

At Wed, 04 Jul 2018 17:23:51 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <67470.1530739431@sss.pgh.pa.us>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Mon, 2 Jul 2018 14:25:58 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoYQhr30eAcgJCi1v0FhA+3RP1FZVnXqSTLe=6fHy9e5oA@mail.gmail.com>

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

The consistency here means "repeatable read" of an object's stats
entry, not a snapshot covering all objects. We don't need to copy
all the entries at once following this definition. The attached
version makes a cache entry only for requested objects.

Uh, what? That's basically destroying the long-standing semantics of
statistics snapshots. I do not think we can consider that acceptable.
As an example, it would mean that scan counts for indexes would not
match up with scan counts for their tables.

The current stats collector mechanism sends at most 8 table stats
in a single message. Split messages from multiple transactions
can reach to collector in shuffled order. The resulting snapshot
can be "inconsistent" if INQUIRY message comes between such split
messages. Of course a single meesage would be enough for common
transactions but not for all.

Even though the inconsistency happens more frequently with this
patch, I don't think users expect such strict consistency of
table stats, especially on a busy system. And I believe it's a
good thing if users saw more "useful" information for the relaxed
consistency. (The actual meaning of "useful" is out of the
current focus:p)

Meanwhile, if we should keep the practical-consistency, a giant
lock is out of the question. So we need a transactional stats of
any shape. It can be a whole-image snapshot or a regular MMVC
table, or maybe the current dshash with UNDO logs. Since there
are actually many states, it is inevitable to require storage to
reproduce each state.

I think the consensus is that the whole-image snapshot takes
too-much memory. MMVC is apparently too-much for the purpose.

UNDO logs seems a bit promising. If we looking stats in a long
transaction, the required memory for UNDO information easily
reaches to the same amount with the whole-image snapshot. But I
expect that it is not so common.

I'll consider that apart from the current patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Thu, 05 Jul 2018 12:04:23 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20180705.120423.49626073.horiguchi.kyotaro@lab.ntt.co.jp>

UNDO logs seems a bit promising. If we looking stats in a long
transaction, the required memory for UNDO information easily
reaches to the same amount with the whole-image snapshot. But I
expect that it is not so common.

I'll consider that apart from the current patch.

Done as a PoC. (Sorry for the format since filterdiff genearates
a garbage from the patch..)

The attached v3-0008 is that. PoC of UNDO logging of server
stats. It records undo logs only for table stats if any
transaction started acess to stats data. So the logging is rarely
performed. The undo logs are used at the first acess to each
relation's stats then cached. autovacuum and vacuum doesn't
request undoing since they just don't need it.

# v3-0007 is a trivial fix for v3-0003, which will be merged.

I see several arguable points on this feature.

- The undo logs are stored in a ring buffer with a fixed size,
currently 1000 entries. If it is filled up, the consistency
will be broken. Undo log is recorded just once after the
latest undo-recording transaction comes. It is likely to be
read in rather short-lived transactions and it's likely that
there's no more than several such transactions
simultaneously. It's possible to provide dynamic resizing
feature but it doesn't seem worth the complexity.

- Undoing runs linear search on the ring buffer. It is done at
the first time when the stats for every relation is
accessed. It can be (a bit) slow when many log entriess
resides. (Concurrent vacuum can generate many undo log
entries.)

- Undo logs for other stats doesn't seem to me to be needed,
but..

A=>: select relname, seq_scan from pg_stat_user_tables where relname = 't1';
relname | seq_scan
t1 | 0

A=> select relname, seq_scan from pg_stat_user_tables where relname = 't2';
relname | seq_scan
t2 | 0

A=> BEGIN;

-- This gives effect because no stats access has been made
B=> select * from t1;
B=> select * from t2;

A=> select relname, seq_scan from pg_stat_user_tables where relname = 't1';
relname | seq_scan
t1 | 1

-- This has no effect because undo logging is now working
B=> select * from t1;
B=> select * from t2;

<repeat two times>

-- This is the second time in this xact to request for t1,
-- just returns cached result.
A=> select relname, seq_scan from pg_stat_user_tables where relname = 't1';
relname | seq_scan
t1 | 1

-- This is the first time in this xact to request for t2. The
-- result is undoed one.
A=> select relname, seq_scan from pg_stat_user_tables where relname = 't2';
relname | seq_scan
t2 | 1
A=> COMMIT;

A=> select relname, seq_scan from pg_stat_user_tables where relname = 't1';
relname | seq_scan
t1 | 4
A=> select relname, seq_scan from pg_stat_user_tables where relname = 't2';
relname | seq_scan
t2 | 4

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jul 4, 2018 at 11:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Mon, 2 Jul 2018 14:25:58 -0400, Robert Haas <robertmhaas@gmail.com>

wrote in <CA+TgmoYQhr30eAcgJCi1v0FhA+3RP1FZVnXqSTLe=6fHy9e5oA@mail.
gmail.com>

Copying the whole hash table kinds of sucks, partly because of the
time it will take to copy it, but also because it means that memory
usage is still O(nbackends * ntables). Without looking at the patch,
I'm guessing that you're doing that because we need a way to show each
transaction a consistent snapshot of the data, and I admit that I
don't see another obvious way to tackle that problem. Still, it would
be nice if we had a better idea.

The consistency here means "repeatable read" of an object's stats
entry, not a snapshot covering all objects. We don't need to copy
all the entries at once following this definition. The attached
version makes a cache entry only for requested objects.

Uh, what? That's basically destroying the long-standing semantics of
statistics snapshots. I do not think we can consider that acceptable.
As an example, it would mean that scan counts for indexes would not
match up with scan counts for their tables.

I agree that this is definitely something that needs to be considered. I
took a look some time ago at the same thing, and ran up against exactly
that one (and at the time did not have time to fix it).

I have not yet had time to look at the downstream suggested handling
(UNDO). However, I had one other thing from my notes I wanted to mention :)

We should probably consider adding an API to fetch counters that *don't*
follow these rules, in case it's not needed. When going through files we're
still stuck at that bottleneck, but if going through shared memory it
should be possible to make it a lot cheaper by volunteering to "not need
that".

We should also consider the ability to fetch stats for a single object,
which would require no copying of the whole structure at all. I think
something like this could for example be used for autovacuum rechecks. On
top of the file based transfer that would help very little, but through
shared memory it could be a lot lighter weight.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Fri, Jul 6, 2018 at 10:29 AM, Magnus Hagander <magnus@hagander.net> wrote:

I agree that this is definitely something that needs to be considered. I
took a look some time ago at the same thing, and ran up against exactly that
one (and at the time did not have time to fix it).

I have not yet had time to look at the downstream suggested handling (UNDO).
However, I had one other thing from my notes I wanted to mention :)

We should probably consider adding an API to fetch counters that *don't*
follow these rules, in case it's not needed. When going through files we're
still stuck at that bottleneck, but if going through shared memory it should
be possible to make it a lot cheaper by volunteering to "not need that".

We should also consider the ability to fetch stats for a single object,
which would require no copying of the whole structure at all. I think
something like this could for example be used for autovacuum rechecks. On
top of the file based transfer that would help very little, but through
shared memory it could be a lot lighter weight.

I think we also have to ask ourselves in general whether snapshots of
this data are worth what they cost. I don't think anyone would doubt
that a consistent snapshot of the data is better than an inconsistent
view of the data if the costs were equal. However, if we can avoid a
huge amount of memory usage and complexity on large systems with
hundreds of backends by ditching the snapshot requirement, then we
should ask ourselves how important we think the snapshot behavior
really is.

Note that commit 3cba8999b34 relaxed the synchronization requirements
around GetLockStatusData(). In other words, since 2011, you can no
longer be certain that 'select * from pg_locks' is returning a
perfectly consistent view of the lock status. If this has caused
anybody a major problem, I'm unaware of it. Maybe the same would end
up being true here. The amount of memory we're consuming for this
data may be a bigger problem than minor inconsistencies in the view of
the data would be.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-07-06 14:49:53 -0400, Robert Haas wrote:

I think we also have to ask ourselves in general whether snapshots of
this data are worth what they cost. I don't think anyone would doubt
that a consistent snapshot of the data is better than an inconsistent
view of the data if the costs were equal. However, if we can avoid a
huge amount of memory usage and complexity on large systems with
hundreds of backends by ditching the snapshot requirement, then we
should ask ourselves how important we think the snapshot behavior
really is.

Indeed. I don't think it's worthwhile major additional memory or code
complexity in this situation. The likelihood of benefitting from more /
better stats seems far higher than a more accurate view of the stats -
which aren't particularly accurate themselves. They don't even survive
crashes right now, so I don't think the current accuracy is very high.

Greetings,

Andres Freund

On 07/06/2018 11:57 AM, Andres Freund wrote:

On 2018-07-06 14:49:53 -0400, Robert Haas wrote:

I think we also have to ask ourselves in general whether snapshots of
this data are worth what they cost. I don't think anyone would doubt
that a consistent snapshot of the data is better than an inconsistent
view of the data if the costs were equal. However, if we can avoid a
huge amount of memory usage and complexity on large systems with
hundreds of backends by ditching the snapshot requirement, then we
should ask ourselves how important we think the snapshot behavior
really is.

Indeed. I don't think it's worthwhile major additional memory or code
complexity in this situation. The likelihood of benefitting from more /
better stats seems far higher than a more accurate view of the stats -
which aren't particularly accurate themselves. They don't even survive
crashes right now, so I don't think the current accuracy is very high.

Will stats, if we move toward the suggested changes be "less" accurate
than they are now? We already know that stats are generally not accurate
but they are close enough. If we move toward this change will it still
be close enough?

JD

Greetings,

Andres Freund

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
*** A fault and talent of mine is to tell it exactly how it is. ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****

On Fri, Jul 6, 2018 at 3:02 PM, Joshua D. Drake <jd@commandprompt.com> wrote:

Will stats, if we move toward the suggested changes be "less" accurate than
they are now? We already know that stats are generally not accurate but they
are close enough. If we move toward this change will it still be close
enough?

There proposed change would have no impact at all on the long-term
accuracy of the statistics. It would just mean that there would be
race conditions when reading them, so that for example you would be
more likely to see a count of heap scans that doesn't match the count
of index scans, because an update arrives in between when you read the
first value and when you read the second one. I don't see that
mattering a whole lot, TBH, but maybe I'm missing something.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-07-06 12:02:39 -0700, Joshua D. Drake wrote:

On 07/06/2018 11:57 AM, Andres Freund wrote:

On 2018-07-06 14:49:53 -0400, Robert Haas wrote:

I think we also have to ask ourselves in general whether snapshots of
this data are worth what they cost. I don't think anyone would doubt
that a consistent snapshot of the data is better than an inconsistent
view of the data if the costs were equal. However, if we can avoid a
huge amount of memory usage and complexity on large systems with
hundreds of backends by ditching the snapshot requirement, then we
should ask ourselves how important we think the snapshot behavior
really is.

Indeed. I don't think it's worthwhile major additional memory or code
complexity in this situation. The likelihood of benefitting from more /
better stats seems far higher than a more accurate view of the stats -
which aren't particularly accurate themselves. They don't even survive
crashes right now, so I don't think the current accuracy is very high.

Will stats, if we move toward the suggested changes be "less" accurate than
they are now? We already know that stats are generally not accurate but they
are close enough. If we move toward this change will it still be close
enough?

I don't think there's a meaningful difference to before. And at the same
time less duplication / hardcoded structure will allow us to increase
the amount of stats we keep.

Greetings,

Andres Freund

On 07/06/2018 12:34 PM, Robert Haas wrote:

On Fri, Jul 6, 2018 at 3:02 PM, Joshua D. Drake <jd@commandprompt.com> wrote:

Will stats, if we move toward the suggested changes be "less" accurate than
they are now? We already know that stats are generally not accurate but they
are close enough. If we move toward this change will it still be close
enough?

There proposed change would have no impact at all on the long-term
accuracy of the statistics. It would just mean that there would be
race conditions when reading them, so that for example you would be
more likely to see a count of heap scans that doesn't match the count
of index scans, because an update arrives in between when you read the
first value and when you read the second one. I don't see that
mattering a whole lot, TBH, but maybe I'm missing something.

I agree that it probably isn't a big deal. Generally speaking when we
look at stats it is to get an "idea" of what is going on. We don't care
if we are missing an increase/decrease of 20 of any particular value
within stats. Based on this and what Andres said, it seems like a net
win to me.

JD

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
*** A fault and talent of mine is to tell it exactly how it is. ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****

On Fri, Jul 6, 2018 at 8:57 PM, Andres Freund <andres@anarazel.de> wrote:

On 2018-07-06 14:49:53 -0400, Robert Haas wrote:

I think we also have to ask ourselves in general whether snapshots of
this data are worth what they cost. I don't think anyone would doubt
that a consistent snapshot of the data is better than an inconsistent
view of the data if the costs were equal. However, if we can avoid a
huge amount of memory usage and complexity on large systems with
hundreds of backends by ditching the snapshot requirement, then we
should ask ourselves how important we think the snapshot behavior
really is.

Indeed. I don't think it's worthwhile major additional memory or code
complexity in this situation. The likelihood of benefitting from more /
better stats seems far higher than a more accurate view of the stats -
which aren't particularly accurate themselves. They don't even survive
crashes right now, so I don't think the current accuracy is very high.

Definitely agreed.

*If* we can provide the snapshots view of them without too much overhead I
think it's worth looking into that while *also* proviiding a lower overhead
interface for those that don't care about it.

If it ends up that keeping the snapshots become too much overhead in either
in performance or code-maintenance, then I agree can probably drop that.
But we should at least properly investigate the cost.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Hi,

On 2018-07-06 22:03:12 +0200, Magnus Hagander wrote:

*If* we can provide the snapshots view of them without too much overhead I
think it's worth looking into that while *also* proviiding a lower overhead
interface for those that don't care about it.

I don't see how that's possible without adding significant amounts of
complexity and probably memory / cpu overhead. The current stats already
are quite inconsistent (often outdated, partially updated, messages
dropped when busy) - I don't see what we really gain by building
something MVCC like in the "new" stats subsystem.

If it ends up that keeping the snapshots become too much overhead in either
in performance or code-maintenance, then I agree can probably drop that.
But we should at least properly investigate the cost.

I don't think it's worthwhile to more than think a bit about it. There's
fairly obvious tradeoffs in complexity here. Trying to get there seems
like a good way to make the feature too big.

Greetings,

Andres Freund

Hello. Thanks for the opinions.

At Fri, 6 Jul 2018 13:10:36 -0700, Andres Freund <andres@anarazel.de> wrote in <20180706201036.awheoi6tk556x6aj@alap3.anarazel.de>

Hi,

On 2018-07-06 22:03:12 +0200, Magnus Hagander wrote:

*If* we can provide the snapshots view of them without too much overhead I
think it's worth looking into that while *also* proviiding a lower overhead
interface for those that don't care about it.

I don't see how that's possible without adding significant amounts of
complexity and probably memory / cpu overhead. The current stats already
are quite inconsistent (often outdated, partially updated, messages
dropped when busy) - I don't see what we really gain by building
something MVCC like in the "new" stats subsystem.

If it ends up that keeping the snapshots become too much overhead in either
in performance or code-maintenance, then I agree can probably drop that.
But we should at least properly investigate the cost.

I don't think it's worthwhile to more than think a bit about it. There's
fairly obvious tradeoffs in complexity here. Trying to get there seems
like a good way to make the feature too big.

Agreed.

Well, if we allow to lose consistency in some extent for improved
performance and smaller footprint, relaxing the consistency of
database stats can reduce footprint further especially on a
cluster with so many databases. Backends are interested only in
the residing database and vacuum doesn't cache stats at all. A
possible problem is vacuum and stats collector can go into a race
condition. I'm not sure but I suppose it is not worse than being
involved in an IO congestion.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 07/10/2018 02:07 PM, Kyotaro HORIGUCHI wrote:

Hello. Thanks for the opinions.

At Fri, 6 Jul 2018 13:10:36 -0700, Andres Freund <andres@anarazel.de> wrote in <20180706201036.awheoi6tk556x6aj@alap3.anarazel.de>

Hi,

On 2018-07-06 22:03:12 +0200, Magnus Hagander wrote:

*If* we can provide the snapshots view of them without too much overhead I
think it's worth looking into that while *also* proviiding a lower overhead
interface for those that don't care about it.

I don't see how that's possible without adding significant amounts of
complexity and probably memory / cpu overhead. The current stats already
are quite inconsistent (often outdated, partially updated, messages
dropped when busy) - I don't see what we really gain by building
something MVCC like in the "new" stats subsystem.

If it ends up that keeping the snapshots become too much overhead in either
in performance or code-maintenance, then I agree can probably drop that.
But we should at least properly investigate the cost.

I don't think it's worthwhile to more than think a bit about it. There's
fairly obvious tradeoffs in complexity here. Trying to get there seems
like a good way to make the feature too big.

Agreed.

Well, if we allow to lose consistency in some extent for improved
performance and smaller footprint, relaxing the consistency of
database stats can reduce footprint further especially on a
cluster with so many databases. Backends are interested only in
the residing database and vacuum doesn't cache stats at all. A
possible problem is vacuum and stats collector can go into a race
condition. I'm not sure but I suppose it is not worse than being
involved in an IO congestion.

As someone who regularly analyzes stats collected from user systems, I
think there's certainly some value with keeping the snapshots reasonably
consistent. But I agree it doesn't need to be perfect, and some level of
inconsistency is acceptable (and the amount of complexity/overhead
needed to maintain perfect consistency seems rather excessive here).

There's one more reason why attempts to keep stats snapshots "perfectly"
consistent are likely doomed to fail - the messages are sent over UDP,
which does not guarantee delivery etc. So there's always some level of
possible inconsistency even with "perfectly consistent" snapshots.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-07-10 14:52:13 +0200, Tomas Vondra wrote:

There's one more reason why attempts to keep stats snapshots "perfectly"
consistent are likely doomed to fail - the messages are sent over UDP, which
does not guarantee delivery etc. So there's always some level of possible
inconsistency even with "perfectly consistent" snapshots.

FWIW, I don't see us continuing to do so if we go for a shared hashtable
for stats.

- Andres

I've spent some time reviewing this version.

Design
------

1. Even with your patch the stats collector still uses an UDP socket to
receive data. Now that the shared memory API is there, shouldn't the
messages be sent via shared memory queue? [1]/messages/by-id/20180711000605.sqjik3vqe5opqz33@alap3.anarazel.de That would increase the
reliability of message delivery.

I can actually imagine backends inserting data into the shared hash tables
themselves, but that might make them wait if the same entries are accessed
by another backend. It should be much cheaper just to insert message into
the queue and let the collector process it. In future version the collector
can launch parallel workers so that writes by backends do not get blocked
due to full queue.

2. I think the access to the shared hash tables introduces more contention
than necessary. For example, pgstat_recv_tabstat() retrieves "dbentry" and
leaves the containing hash table partition locked *exclusively* even if it
changes only the containing table entries, while changes of the containing
dbentry are done.

It appears that the shared hash tables are only modified by the stats
collector. The unnecessary use of the exclusive lock might be a bigger
issue in the future if the stats collector will use parallel
workers. Monitoring functions and autovacuum are affected by the locking
now.

(I see that the it's not trivial to get just-created entry locked in shared
mode: it may need a loop in which we release the exclusive lock and acquire
the shared lock unless the entry was already removed.)

3. Data in both shared_archiverStats and shared_globalStats is mostly accessed
w/o locking. Is that ok? I'd expect the StatsLock to be used for these.

Coding
------

* git apply v4-0003-dshash-based-stats-collector.patch needed manual
resolution of one conflict.

* pgstat_quickdie_handler() appears to be the only "quickdie handler" that
calls on_exit_reset(), although the comments are almost copy & pasted from
such a handler of other processes. Can you please explain what's specific
about pgstat.c?

* the variable name "area" would be sufficient if it was local to some
function, otherwise I think the name is too generic.

* likewise db_stats is too generic for a global variable. How about
"snapshot_db_stats_local"?

* backend_get_db_entry() passes 0 for handle to snapshot_statentry(). How
about DSM_HANDLE_INVALID ?

* I only see one call of snapshot_statentry_all() and it receives 0 for
handle. Thus the argument can be removed and the function does not have to
attach / detach to / from the shared hash table.

* backend_snapshot_global_stats() switches to TopMemoryContext before it calls
pgstat_attach_shared_stats(), but the latter takes care of the context
itself.

* pgstat_attach_shared_stats() - header comment should explain what the return
value means.

* reset_dbentry_counters() does more than just resetting the counters. Name
like initialize_dbentry() would be more descriptive.

* typos:

** backend_snapshot_global_stats(): "liftime" -> "lifetime"

** snapshot_statentry(): "entriy" -> "entry"

** backend_get_func_etnry(): "onshot" -> "oneshot"

** snapshot_statentry_all(): "Returns a local hash contains ..." -> "Returns a local hash containing ..."

[1]: /messages/by-id/20180711000605.sqjik3vqe5opqz33@alap3.anarazel.de

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

Hi,

On 2018-09-20 09:55:27 +0200, Antonin Houska wrote:

I've spent some time reviewing this version.

Design
------

1. Even with your patch the stats collector still uses an UDP socket to
receive data. Now that the shared memory API is there, shouldn't the
messages be sent via shared memory queue? [1] That would increase the
reliability of message delivery.

I can actually imagine backends inserting data into the shared hash tables
themselves, but that might make them wait if the same entries are accessed
by another backend. It should be much cheaper just to insert message into
the queue and let the collector process it. In future version the collector
can launch parallel workers so that writes by backends do not get blocked
due to full queue.

I don't think either of these is right. I think it's crucial to get rid
of the UDP socket, but I think using a shmem queue is the wrong
approach. Not just because postgres' shm_mq is single-reader/writer, but
also because it's plainly unnecessary. Backends should attempt to
update the shared hashtable, but acquire the necessary lock
conditionally, and leave the pending updates of the shared hashtable to
a later time if they cannot acquire the lock.

Greetings,

Andres Freund

Hello. Thank you for the comments.

At Thu, 20 Sep 2018 10:37:24 -0700, Andres Freund <andres@anarazel.de> wrote in <20180920173724.5w2n2nwkxtyi4azw@alap3.anarazel.de>

Hi,

On 2018-09-20 09:55:27 +0200, Antonin Houska wrote:

I've spent some time reviewing this version.

Design
------

1. Even with your patch the stats collector still uses an UDP socket to
receive data. Now that the shared memory API is there, shouldn't the
messages be sent via shared memory queue? [1] That would increase the
reliability of message delivery.

I can actually imagine backends inserting data into the shared hash tables
themselves, but that might make them wait if the same entries are accessed
by another backend. It should be much cheaper just to insert message into
the queue and let the collector process it. In future version the collector
can launch parallel workers so that writes by backends do not get blocked
due to full queue.

I don't think either of these is right. I think it's crucial to get rid
of the UDP socket, but I think using a shmem queue is the wrong
approach. Not just because postgres' shm_mq is single-reader/writer, but
also because it's plainly unnecessary. Backends should attempt to
update the shared hashtable, but acquire the necessary lock
conditionally, and leave the pending updates of the shared hashtable to
a later time if they cannot acquire the lock.

Ok, I just intended to avoid reading many bytes from a file and
thought that writer-side can be resolved later.

Currently locks on the shared stats table is acquired by dshash
mechanism in a partition-wise manner. The number of the
partitions is currently fixed to 2^7 = 128, but writes for the
same table confilicts each other regardless of the number of
partitions. As the first step, I'm going to add
conditional-locking capability to dsahsh_find_or_insert and each
backend holds a queue of its pending updates.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. This is a super-PoC of no-UDP stats collector.

At Wed, 26 Sep 2018 09:55:09 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20180926.095509.182252925.horiguchi.kyotaro@lab.ntt.co.jp>

I don't think either of these is right. I think it's crucial to get rid
of the UDP socket, but I think using a shmem queue is the wrong
approach. Not just because postgres' shm_mq is single-reader/writer, but
also because it's plainly unnecessary. Backends should attempt to
update the shared hashtable, but acquire the necessary lock
conditionally, and leave the pending updates of the shared hashtable to
a later time if they cannot acquire the lock.

Ok, I just intended to avoid reading many bytes from a file and
thought that writer-side can be resolved later.

Currently locks on the shared stats table is acquired by dshash
mechanism in a partition-wise manner. The number of the
partitions is currently fixed to 2^7 = 128, but writes for the
same table confilicts each other regardless of the number of
partitions. As the first step, I'm going to add
conditional-locking capability to dsahsh_find_or_insert and each
backend holds a queue of its pending updates.

I don't have more time 'til next monday so this is just a PoC
(sorry..).

- 0001 to 0006 is rebased version of v4.
- 0007 adds conditional locking to dshash

- 0008 is the no-UDP stats collector.

If required lock is not acquired for some stats items, report
funcions immediately return after storing the values locally. The
stored values are merged with later calls. Explicitly calling
pgstat_cleanup_pending_stat() at a convenient timing tries to
apply the pending values, but the function is not called anywhere
for now.

stats collector process is used only to save and load saved stats
files and create shared memory for stats. I'm going to remove
stats collector.

I'll continue working this way.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

The previous patch doesn't work...

At Thu, 27 Sep 2018 22:00:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20180927.220049.168546206.horiguchi.kyotaro@lab.ntt.co.jp>

- 0001 to 0006 is rebased version of v4.
- 0007 adds conditional locking to dshash

- 0008 is the no-UDP stats collector.

If required lock is not acquired for some stats items, report
funcions immediately return after storing the values locally. The
stored values are merged with later calls. Explicitly calling
pgstat_cleanup_pending_stat() at a convenient timing tries to
apply the pending values, but the function is not called anywhere
for now.

stats collector process is used only to save and load saved stats
files and create shared memory for stats. I'm going to remove
stats collector.

I'll continue working this way.

It doesn't work nor even compile since I failed to include some
changes. The atached v6-0008 at least compiles and words.

0001-0007 are not attached since they are still aplicable on
master head with offsets.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Tue, 02 Oct 2018 16:06:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20181002.160651.117284090.horiguchi.kyotaro@lab.ntt.co.jp>

It doesn't work nor even compile since I failed to include some
changes. The atached v6-0008 at least compiles and words.

0001-0007 are not attached since they are still aplicable on
master head with offsets.

In this patchset 0001-0007 are still the same with the previous
version. I'll reorganize the whole patchset in the next version.

This is more saner version of previous v5-0008, which didn't pass
regression test. v6-0008 to v6-0010 are attached and they are
applied on top of v5-0001-0007.

- stats collector has been removed.

- modified dshash further so that deletion is allowed during
sequential scan.

- I'm not sure about the following existing comment at the
beginning of pgstat.c

* - Add a pgstat config column to pg_database, so this
* entire thing can be enabled/disabled on a per db basis.

Some points known to need some considerations are:

1. Concurrency is controlled by per-database entry in db_stats
dshash. It has 127 lock partitions but all backends on the
same database share just one lock and only one backend takes
the right to update stats. (Every backend doesn't update stats
with the interval not shorter than 500ms, like the current
stats collector.) Table-stats can be removed by DROP DB
simultaeously with stats updates so need to block it using
per-databsae lock. Any locking means other than dshash might
be needed.

2. Since dshash cannot allow multiple locks because of resize,
pgstat_update_stat is forced to be a bit ineffecient.
It loops onver stats list twice, for shared tables and regular
tables since we can acquire lock on one database at once.

Maybe providing individual TabStatusArray for the two will fix
it, will do in the next version.

3. This adds a new timeout IDLE_STATS_UPDATE_TIMEOUT. This works
similarly to IDLE_IN_TRANSACTIION_SESSION_TIMEOUT. It fires in
at most PGSTAT_STAT_MIN_INTERVAL(500)ms to clean up pending
statistics update.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

This is more saner version of previous v5-0008, which didn't pass
regression test. v6-0008 to v6-0010 are attached and they are
applied on top of v5-0001-0007.

- stats collector has been removed.

- modified dshash further so that deletion is allowed during
sequential scan.

- I'm not sure about the following existing comment at the
beginning of pgstat.c

* - Add a pgstat config column to pg_database, so this
* entire thing can be enabled/disabled on a per db basis.

Following is the next handful of my comments:

* If you remove the stats collector, I think the remaining code in pgstat.c
does no longer fit into the backend/postmaster/ directory.

* I'm not sure it's o.k. to call pgstat_write_statsfiles() from
postmaster.c:reaper(): the function can raise ERROR (I see at least one code
path: pgstat_write_statsfile() -> get_dbstat_filename()) and, as reaper() is
a signal handler, it's hard to imagine the consequences. Maybe a reason to
leave some functionality in a separate worker, although the "stats
collector" would have to be changed.

* Question still remains whether all the statistics should be loaded into
shared memory, see the note on paging near the bottom of [1]/messages/by-id/CA+TgmobQVbz4K_+RSmiM9HeRKpy3vS5xnbkL95gSEnWijzprKQ@mail.gmail.com.

* if dshash_seq_init() is passed consistent=false, shouldn't we call
ensure_valid_bucket_pointers() also from dshash_seq_next()? If the scan
needs to access the next partition and the old partition lock got released,
the table can be resized before the next partition lock is acquired, and
thus the backend-local copy of buckets becomes obsolete.

* Neither snapshot_statentry_all() nor backend_snapshot_all_db_entries() seems
to be used in the current patch version.

* pgstat_initstats(): I think WaitLatch() should be used instead of sleep().

* pgstat_get_db_entry(): "return false" should probably be "return NULL".

* Is the PGSTAT_TABLE_WRITE flag actually used? Unlike PGSTAT_TABLE_CREATE, I
couldn't find a place where it's value is tested.

* dshash_seq_init(): does it need to be called with consistent=true from
pgstat_vacuum_stat() when the the entries returned by the scan are just
dropped?

dshash_seq_init(&dshstat, db_stats, true, true);

I suspect this is a thinko because another call from the same function looks
like

dshash_seq_init(&dshstat, dshtable, false, true);

* I'm not sure about usefulness of dshash_get_num_entries(). It passes
consistent=false to dshash_seq_init(), so the number of entries can change
during the execution. And even if the function requested a "consistent
scan", entries can be added / removed as soon as the scan is over.

If the returned value is used to decide whether the hashtable should be
scanned or not, I think you don't break anything if you simply start the
scan unconditionally and see if you find some entries.

And if you really need to count the entries, I suggest that you use the
per-partition counts (dshash_partition.count) instead of scanning individual
entries.

[1]: /messages/by-id/CA+TgmobQVbz4K_+RSmiM9HeRKpy3vS5xnbkL95gSEnWijzprKQ@mail.gmail.com

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Hi,

I've started looking at the patch over the past few days. I don't have
any deep insights at this point, but there seems to be some sort of
issue in pgstat_update_stat. When building using gcc, I do get this warning:

pgstat.c: In function ‘pgstat_update_stat’:
pgstat.c:648:18: warning: ‘now’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
oldest_pending = now;
~~~~~~~~~~~~~~~^~~~~
PostgreSQL installation complete.

which kinda makes sense, because 'now' is set only in the (!force)
branch. So if the very first call to pgstat_update_stat is with
force=true, it's not set, and the code executes this:

/* record oldest pending update time */
if (pgStatPendingTabHash == NULL)
oldest_pending = 0;
else if (oldest_pending == 0)
oldest_pending = now;

at which point we set "oldest_pending = now" with "now" containing some
random garbage.

When running this under valgrind, I get a couple of warnings in this
area of code - see the attached log with a small sample. Judging by the
locations I assume those are related to the same issue, but I have not
looked into that.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/05/2018 10:30 AM, Kyotaro HORIGUCHI wrote:

Hello.

At Tue, 02 Oct 2018 16:06:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20181002.160651.117284090.horiguchi.kyotaro@lab.ntt.co.jp>

It doesn't work nor even compile since I failed to include some
changes. The atached v6-0008 at least compiles and words.

0001-0007 are not attached since they are still aplicable on
master head with offsets.

In this patchset 0001-0007 are still the same with the previous
version. I'll reorganize the whole patchset in the next version.

This is more saner version of previous v5-0008, which didn't pass
regression test. v6-0008 to v6-0010 are attached and they are
applied on top of v5-0001-0007.

BTW one more thing - I strongly recommend always attaching the whole
patch series, even if some of the parts did not change.

Firstly, it makes the reviewer's life much easier, because it's not
necessary to hunt through past messages for all the bits and resolve
potential conflicts (e.g. there are two 0008 in recent messages).

Secondly, it makes http://commitfest.cputube.org/ work - it tries to
apply patches from a single message, which fails when some of the parts
are omitted.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thank you for the comments, Antonin, Tomas.

At Tue, 30 Oct 2018 13:35:23 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <b38591e0-54ca-7a27-813b-0bf91a204c5b@2ndquadrant.com>

On 10/05/2018 10:30 AM, Kyotaro HORIGUCHI wrote:

Hello.

At Tue, 02 Oct 2018 16:06:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20181002.160651.117284090.horiguchi.kyotaro@lab.ntt.co.jp>

It doesn't work nor even compile since I failed to include some
changes. The atached v6-0008 at least compiles and words.

0001-0007 are not attached since they are still aplicable on
master head with offsets.

In this patchset 0001-0007 are still the same with the previous
version. I'll reorganize the whole patchset in the next version.

This is more saner version of previous v5-0008, which didn't pass
regression test. v6-0008 to v6-0010 are attached and they are
applied on top of v5-0001-0007.

BTW one more thing - I strongly recommend always attaching the whole
patch series, even if some of the parts did not change.

Firstly, it makes the reviewer's life much easier, because it's not
necessary to hunt through past messages for all the bits and resolve
potential conflicts (e.g. there are two 0008 in recent messages).

Secondly, it makes http://commitfest.cputube.org/ work - it tries to
apply patches from a single message, which fails when some of the parts
are omitted.

Yeah, I know about the second point. About the second point, I'm
not sure which makes review easier confirming difference in all
files or finding not-modified portion from upthread. But ok, I'l
follow your suggestion. I'll attach the whole patches and add
note about difference.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for the comments.

This message contains the whole refactored patch set.

At Mon, 29 Oct 2018 15:10:10 +0100, Antonin Houska <ah@cybertec.at> wrote in <28855.1540822210@localhost>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

This is more saner version of previous v5-0008, which didn't pass
regression test. v6-0008 to v6-0010 are attached and they are
applied on top of v5-0001-0007.

- stats collector has been removed.

- modified dshash further so that deletion is allowed during
sequential scan.

- I'm not sure about the following existing comment at the
beginning of pgstat.c

* - Add a pgstat config column to pg_database, so this
* entire thing can be enabled/disabled on a per db basis.

Following is the next handful of my comments:

* If you remove the stats collector, I think the remaining code in pgstat.c
does no longer fit into the backend/postmaster/ directory.

I didn't condier that, but I still don't find nice place among
existing directories. backend/statistics may be that but I feel
somewhat uneasy.. Finally I split pgstat.c into two files
(suggested in the file comment) and put both of them into a new
directory backend/statmon. One part is backend status faclity,
named bestatus (BackEnd Status) and one is pgstat, access
statistics part. The last 0008 patch does that. I tried to move
it ealier but it was a bit tough.

* I'm not sure it's o.k. to call pgstat_write_statsfiles() from
postmaster.c:reaper(): the function can raise ERROR (I see at least one code
path: pgstat_write_statsfile() -> get_dbstat_filename()) and, as reaper() is
a signal handler, it's hard to imagine the consequences. Maybe a reason to
leave some functionality in a separate worker, although the "stats
collector" would have to be changed.

I was careless about that. longjmp() in signal handler is
inhibited so we mustn't emit ERROR there. In the first place they
are placed in wrong places from several perspective. I changed
the way load and store. In the attached patch (0003), load is
performed on the way initializing shared memory on postmaster and
storing is done in shutdown hook on postmaster. Since the shared
memory area is inherited to all children so no process actually
does initial attaching any longer. Addition to that archiver
process became an auxiliary process (0004) since it writes to the
shared stats.

* Question still remains whether all the statistics should be loaded into
shared memory, see the note on paging near the bottom of [1].

Even counting possible page-in latency on stats writing, I agree
to what Robert said in the message that we will win by average
regarding to users who don't create so many databases. If some
stats page to write were paged-out, also related heap page would
have been evicted out from shared buffers (or the buffer page
itself may have been paged-out) and every resources that can be
stashed out may be stashed out. So I don't think it becomes a
serious problem. On reading stats, it is currently reading a file
and sometimes waiting for maiking a up-to-date file. I think we
are needless to say about the case.

For cluster with many-many databases, a backend running on a
database will mainly see only stats for the current database (and
about shared tables) we can split stats by that criteria in the
next step.

* if dshash_seq_init() is passed consistent=false, shouldn't we call
ensure_valid_bucket_pointers() also from dshash_seq_next()? If the scan
needs to access the next partition and the old partition lock got released,
the table can be resized before the next partition lock is acquired, and
thus the backend-local copy of buckets becomes obsolete.

Oops. You're right. Addition to that, resizing can happen while
dshash_seq_next moves the lock to the next partition. Resizing
happens on the way breaks sequential scan semantics. I added
ensure_valid_bucket_pointers() after the initial acquisition of
partition lock and move the lock seamlessly during scan. (0001)

* Neither snapshot_statentry_all() nor backend_snapshot_all_db_entries() seems
to be used in the current patch version.

Thanks. This is not used since we concluded that we no longer
need strict consistency in stats numbers. Removed. (0003)

* pgstat_initstats(): I think WaitLatch() should be used instead of sleep().

Bgwriter and checkpointer waited for postmaster's loading of
stats files. But I changed the startup sequence (as mentioned
above), so the wait became useless. Most of them are reaplced
with Assert. (0003)

* pgstat_get_db_entry(): "return false" should probably be "return NULL".

I don't find that. (Isn't it caught by compiler?) Maybe it is
"found = false"? (it might be a bit tricky)

* Is the PGSTAT_TABLE_WRITE flag actually used? Unlike PGSTAT_TABLE_CREATE, I
couldn't find a place where it's value is tested.

Thank you for fiding that. As pointed, PGSTAT_TABLE_WRITE is
finally not used since WRITE is always accompanied by CREATE in
the patch. I think WRITE is more readable than CREATE there so I
removed CREATE. I renamed all PGSTAT_TABLE_ symbols as the
follows while fixing this.

PGSTAST_TABLE_READ -> PGSTAT_FETCH_SHARED
PGSTAST_TABLE_WRITE -> PGSTAT_FETCH_EXCLUSIVE
PGSTAST_TABLE_NOWAIT -> PGSTAT_FETCH_NOWAIT

PGSTAST_TABLE_NOT_FOUND -> PGSTAT_ENTRY_NOT_FOUND
PGSTAST_TABLE_FOUND -> PGSTAT_ENTRY_FOUND
PGSTAST_TABLE_LOCK_FAILED -> PGSTAT_LOCK_FAILED

* dshash_seq_init(): does it need to be called with consistent=true from
pgstat_vacuum_stat() when the the entries returned by the scan are just
dropped?

dshash_seq_init(&dshstat, db_stats, true, true);

I suspect this is a thinko because another call from the same function looks
like

dshash_seq_init(&dshstat, dshtable, false, true);

It's a left-behind, which was just migrated from the previous (in
v4) snapshot-based code. Snapshots had such consistency. But it
no longer looks useful. (0003)

As the result consistent=false in all caller site so I can remove
the parameter but I leave it alone for a while.

* I'm not sure about usefulness of dshash_get_num_entries(). It passes
consistent=false to dshash_seq_init(), so the number of entries can change
during the execution. And even if the function requested a "consistent
scan", entries can be added / removed as soon as the scan is over.

If the returned value is used to decide whether the hashtable should be
scanned or not, I think you don't break anything if you simply start the
scan unconditionally and see if you find some entries.
And if you really need to count the entries, I suggest that you use the
per-partition counts (dshash_partition.count) instead of scanning individual
entries.

It is mainly to avoid useless call to pgstat_collect_oid(). The
shortcut is useful because funcstat is not taken ususally.
Instead, I removed the function and create function stats dshash
on-demand. Then changed the condition "dshash_get_num_entries() >
0" to "dbentry->functions != DSM_HANDLE_INVALID". (0003)

[1] /messages/by-id/CA+TgmobQVbz4K_+RSmiM9HeRKpy3vS5xnbkL95gSEnWijzprKQ@mail.gmail.com

New version of this patch is attched to reply to Tomas's message.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. Thank you for looking this.

At Tue, 30 Oct 2018 01:49:59 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <5253d750-890b-069b-031f-2a9b73e47832@2ndquadrant.com>

Hi,

I've started looking at the patch over the past few days. I don't have
any deep insights at this point, but there seems to be some sort of
issue in pgstat_update_stat. When building using gcc, I do get this
warning:

pgstat.c: In function ‘pgstat_update_stat’:
pgstat.c:648:18: warning: ‘now’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
oldest_pending = now;
~~~~~~~~~~~~~~~^~~~~
PostgreSQL installation complete.

Uggh! The reason for the code is "last_report = now" comes later
than the code around... Fixed.

When running this under valgrind, I get a couple of warnings in this
area of code - see the attached log with a small sample. Judging by
the locations I assume those are related to the same issue, but I have
not looked into that.

There was several typos/thinkos related to pointers modifed from
original variables. There was a code like the following in the
original code.

memset(&shared_globalStats, 0, siazeof(shared_globalStats));

It was not fixed despite this patch changes the type of the
variable from PgStat_GlboalStats to (PgStat_GlobalStats *). As
the result major part of the varialbe remaineduninitialized.

I re-ran this version on valgrind and I didn't see such kind of
problem. Thank you for the testing.

I refactored the patch into complete set consists of 8 files.

v7-0001-sequential-scan-for-dshash.patch
- dshash sequential scan feature

v7-0002-Add-conditional-lock-feature-to-dshash.patch
- dshash contitional lock feature

v7-0003-Shared-memory-based-stats-collector.patch
- Shared memory base stats collector.
- Remove stats collector process
- Change stats collector to shared memory base
(This needs 0004 to work, but it is currently separate from
this for readability)

v7-0004-Make-archiver-process-an-auxiliary-process.patch
- Archiver process needs to touch shared memory.
(I didn't check EXEC_BACKEND case)

v7-0005-Let-pg_stat_statements-not-to-use-PG_STAT_TMP_DIR.patch
- I removed pg_stat_tmp directory so move pg_stat_statements
file stored there to pg_stat directory. (This would need fix)

v7-0006-Remove-pg_stat_tmp-exclusion-from-pg_rewind.patch
- For the same reason with 0005.

v7-0007-Documentation-update.patch
- Removes description related to pg_stat_tmp.

v7-0008-Split-out-backend-status-monitor-part-from-pgstat.patch
- Just refactoring. Splits the current postmaster/pgstat.c into
two files statmon/pgstat.c and statmon/bestatus.c.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 11/8/18 12:46 PM, Kyotaro HORIGUCHI wrote:

Hello. Thank you for looking this.

At Tue, 30 Oct 2018 01:49:59 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <5253d750-890b-069b-031f-2a9b73e47832@2ndquadrant.com>

Hi,

I've started looking at the patch over the past few days. I don't have
any deep insights at this point, but there seems to be some sort of
issue in pgstat_update_stat. When building using gcc, I do get this
warning:

pgstat.c: In function ‘pgstat_update_stat’:
pgstat.c:648:18: warning: ‘now’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
oldest_pending = now;
~~~~~~~~~~~~~~~^~~~~
PostgreSQL installation complete.

Uggh! The reason for the code is "last_report = now" comes later
than the code around... Fixed.

When running this under valgrind, I get a couple of warnings in this
area of code - see the attached log with a small sample. Judging by
the locations I assume those are related to the same issue, but I have
not looked into that.

There was several typos/thinkos related to pointers modifed from
original variables. There was a code like the following in the
original code.

memset(&shared_globalStats, 0, siazeof(shared_globalStats));

It was not fixed despite this patch changes the type of the
variable from PgStat_GlboalStats to (PgStat_GlobalStats *). As
the result major part of the varialbe remaineduninitialized.

I re-ran this version on valgrind and I didn't see such kind of
problem. Thank you for the testing.

OK, regression tests now seem to pass without any valgrind issues.

However a quite a few extensions in contrib seem are broken now. It
seems fixing it is as simple as including the new bestatus.h next to
pgstat.h.

I'm not sure splitting the headers like this is needed, actually. It's
true we're replacing pgstat.c with something else, but it's still
related to stats, backing pg_stat_* system views etc. So I'd keep as
much of the definitions in pgstat.h, so that it's enough to include that
one header file. That would "unbreak" the extensions.

Renaming pgstat_report_* functions to bestatus_report_* seems
unnecessary to me too. The original names seem quite fine to me.

BTW the updated patches no longer apply cleanly. Apparently it got
broken since Tuesday, most likely by the pread/pwrite patch.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Nov-08, Tomas Vondra wrote:

I'm not sure splitting the headers like this is needed, actually. It's true
we're replacing pgstat.c with something else, but it's still related to
stats, backing pg_stat_* system views etc. So I'd keep as much of the
definitions in pgstat.h, so that it's enough to include that one header
file. That would "unbreak" the extensions.

pgstat.h includes a lot of other stuff that presumably isn't needed if
all some .c wants is in bestatus.h, so my vote would be to make this
change *if it's actually possible to do it*: you want the affected
headers to compile standalone (use cpluspluscheck or similar to verify
this), for one thing.

Renaming pgstat_report_* functions to bestatus_report_* seems unnecessary to
me too. The original names seem quite fine to me.

Yeah, this probably keeps churn to a minimum.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello. This is rebased version.

At Thu, 8 Nov 2018 16:06:49 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com>

However a quite a few extensions in contrib seem are broken now. It
seems fixing it is as simple as including the new bestatus.h next to
pgstat.h.

The additional 0009 does that.

At Thu, 8 Nov 2018 12:39:41 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20181108153941.txjb6rg3y7q26ldm@alvherre.pgsql>

On 2018-Nov-08, Tomas Vondra wrote:

I'm not sure splitting the headers like this is needed, actually. It's true
we're replacing pgstat.c with something else, but it's still related to
stats, backing pg_stat_* system views etc. So I'd keep as much of the
definitions in pgstat.h, so that it's enough to include that one header
file. That would "unbreak" the extensions.

pgstat.h includes a lot of other stuff that presumably isn't needed if
all some .c wants is in bestatus.h, so my vote would be to make this
change *if it's actually possible to do it*: you want the affected
headers to compile standalone (use cpluspluscheck or similar to verify
this), for one thing.

cpluspluscheck doesn't complain to this change. I'm afraid that I
didn't read you cleary, but I counted up files that needs only
pgstat/ only bestatus / both in $(TOPDIR). (containing contrib)

only (new) pgstat.h : 33
only bestatus.h : 47
both : 22

Renaming pgstat_report_* functions to bestatus_report_* seems unnecessary to
me too. The original names seem quite fine to me.

Yeah, this probably keeps churn to a minimum.

Reverted the names. pgstat_intialize() and
pgstat_clear_snapshot() were splitted into two functions for each
pgstat.c and bestatus.c. (and pgstat_clear_snapstop() is calling
pgstat_bestatus_clear_snapshot()!, and the names of the functions
looks somewhat inconsistent. Will fix later.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 11/9/18 9:33 AM, Kyotaro HORIGUCHI wrote:

Hello. This is rebased version.

At Thu, 8 Nov 2018 16:06:49 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com>

However a quite a few extensions in contrib seem are broken now. It
seems fixing it is as simple as including the new bestatus.h next to
pgstat.h.

The additional 0009 does that.

That does fix it, indeed. But the break happens in 0003, so that's where
the fixes should be moved - I've tried to simply apply 0009 right after
0003, but that does not seem to work because bestatus.h does not exist
at that point yet :-/

The current split into 8 parts seems quite sensible to me, i.e. that's
how it might get committed eventually. That however means each part
needs to be correct on it's own (hence fixes in 0009 are a problem).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Fri, 9 Nov 2018 14:16:31 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <803f2d96-3b4b-f357-9a2e-45443212f13d@2ndquadrant.com>

On 11/9/18 9:33 AM, Kyotaro HORIGUCHI wrote:

Hello. This is rebased version.
At Thu, 8 Nov 2018 16:06:49 +0100, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote in
<de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com>

However a quite a few extensions in contrib seem are broken now. It
seems fixing it is as simple as including the new bestatus.h next to
pgstat.h.

The additional 0009 does that.

That does fix it, indeed. But the break happens in 0003, so that's
where the fixes should be moved - I've tried to simply apply 0009
right after 0003, but that does not seem to work because bestatus.h
does not exist at that point yet :-/

Sorry, I misunderstood you. The real reason for breaking 0003 as
you saw was the result I just removed PG_STAT_TMP_DIR. 0005 fixes
that later. I (half-intentionally) didin't keep soundness of the
source tree at v8-0003 and v8-0008.

The current split into 8 parts seems quite sensible to me, i.e. that's
how it might get committed eventually. That however means each part
needs to be correct on it's own (hence fixes in 0009 are a problem).

Thanks. I neatended up the patchset so that individual patch
keeps source buildable and doesn't break programs' behaviors.

v9-0001-sequential-scan-for-dshash.patch
v9-0002-Add-conditional-lock-feature-to-dshash.patch
same to v8

v9-0003-Make-archiver-process-an-auxiliary-process.patch
moved from v8-0004 since this is applicable independently from
v8-0003.

v9-0004-Shared-memory-based-stats-collector.patch
v8-0003 + some fixes to make contribs work and removed initdb
part.

v9-0005-Remove-statistics-temporary-directory.patch
v8-0005 + v8-0006 + initdb part of v8-0003. pg_stat_statements
may still need fix since I changed only the directory for
temporary query files.

v9-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
v8-0008 plus v8-0009

v9-0007-Documentation-update.patch
this still leaves description about UDP-based stats collector
and will need further edit.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, 2018-11-12 at 20:10 +0900, Kyotaro HORIGUCHI wrote:

At Fri, 9 Nov 2018 14:16:31 +0100, Tomas Vondra <
tomas.vondra@2ndquadrant.com> wrote in <
803f2d96-3b4b-f357-9a2e-45443212f13d@2ndquadrant.com>

On 11/9/18 9:33 AM, Kyotaro HORIGUCHI wrote:

Hello. This is rebased version.
At Thu, 8 Nov 2018 16:06:49 +0100, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote in
<de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com>

However a quite a few extensions in contrib seem are broken now.

It

seems fixing it is as simple as including the new bestatus.h

next to

pgstat.h.

The additional 0009 does that.

That does fix it, indeed. But the break happens in 0003, so that's
where the fixes should be moved - I've tried to simply apply 0009
right after 0003, but that does not seem to work because bestatus.h
does not exist at that point yet :-/

Sorry, I misunderstood you. The real reason for breaking 0003 as
you saw was the result I just removed PG_STAT_TMP_DIR. 0005 fixes
that later. I (half-intentionally) didin't keep soundness of the
source tree at v8-0003 and v8-0008.

The current split into 8 parts seems quite sensible to me, i.e.

that's

how it might get committed eventually. That however means each part
needs to be correct on it's own (hence fixes in 0009 are a

problem).

Thanks. I neatended up the patchset so that individual patch
keeps source buildable and doesn't break programs' behaviors.

OK, thanks. I'll take a look. I also plan to do much more testing, both
for correctness and performance - it's quite piece of functionality.

If everything goes well I'd like to get this committed by the end of
January CF (with some of the initial parts in this CF, possibly).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

Unfortunately, the patch does not apply anymore - it seems it got broken
by the changes to signal handling and/or removal of magic OIDs :-(

I've done a review and testing when applied on top of 10074651e335.

Firstly, the testing - I was wondering if the patch has some performance
impact, so I've done some testing with a read-only workload on large
number of tables (1k, 10k and 100k) while concurrently selecting data
from pg_stat_* catalogs at the same time.

In one case both workloads were running against the same database, in
another there were two separate databases (and the selects from stat
catalogs were running against an "empty" database with no use tables).

In both cases there were 8 clients doing selects from the user tables,
and 4 clients accessing the pg_stat_* catalogs.

For the "single database" case the results looks like this (this is just
patched / master throughput):

# of tables xact stats
------------------------------
1000 97.71% 98.76%
10000 100.38% 97.97%
100000 100.10% 98.50%

xact is throughput of the user workload (select from the large number of
tables) and stats is throughput for selects from system catalogs.

So pretty much no difference - 2% is within noise on this machine.

On two separate databases the results are a bit more interesting:

# of tables xact stats
-------------------------------
1000 100.49% 80.38%
10000 103.18% 80.28%
100000 100.85% 81.95%

For the main workload there's pretty much no difference, but for selects
from the stats catalogs there's ~20% drop in throughput. In absolute
numbers this means drop from ~670tps to ~550tps. I haven't investigated
this, but I suppose this is due to dshash seqscan being more expensive
than reading the data from file.

I don't think any of this is an issue in practice, though. The important
thing is that there's no measurable impact on the regular workload.

Now, a couple of comments regarding individual parts of the patch.

0001-0003
---------

I do think 0001 - 0003 are ready, with some minor cosmetic issues:

1) I'd rephrase the last part of dshash_seq_init comment more like this:

* If consistent is set for dshash_seq_init, the all hash table
* partitions are locked in the requested mode (as determined by the
* exclusive flag), and the locks are held until the end of the scan.
* Otherwise the partition locks are acquired and released as needed
* during the scan (up to two partitions may be locked at the same time).

Maybe it should briefly explain what the consistency guarantees are (and
aren't), but considering we're not materially changing the existing
behavior probably is not really necessary.

2) I think the dshash_find_extended() signature should be more like
dshash_find(), i.e. just appending parameters instead of moving them
around unnecessarily. Perhaps we should add

Assert(nowait || !lock_acquired);

Using nowait=false with lock_acquired!=NULL does not seem sensible.

3) I suppose this comment in postmaster.c is just copy-paste:

-#define BACKEND_TYPE_ARCHIVER	0x0010	/* bgworker process */
+#define BACKEND_TYPE_ARCHIVER	0x0010	/* archiver process */

I wonder why wasn't archiver a regular auxiliary process already? It
seems like a fairly natural thing, so why not?

0004 (+0005 and 0007)
---------------------

This seems fine, but I have my doubts about two changes - removing of
stats_temp_directory and the IDLE_STATS_UPDATE_TIMEOUT thingy.

There's a couple of issues with the stats_temp_directory. Firstly, I
don't understand why it's spread over multiple parts of the patch. The
GUC is removed in 0004, the underlying variable is removed in 0005 and
then the docs are updated in 0007. If we really want to do this, it
should happen in a single patch.

But the main question is - do we really want to do that? I understand
this directory was meant for the stats data we're moving to shared
memory, so removing it seems natural. But clearly it's used by
pg_stat_statements - 0005 fixes that, of course, but I wonder if there
are other extensions using it to store files?

It's not just about how intensive I/O to those files is, but this also
means the files will now be included in backups / pg_rewind, and maybe
that's not really desirable?

Maybe it's fine but I'm not quite convinced about it ...

I'm not sure I understand what IDLE_STATS_UPDATE_TIMEOUT does. You've
described it as

This adds a new timeout IDLE_STATS_UPDATE_TIMEOUT. This works
similarly to IDLE_IN_TRANSACTIION_SESSION_TIMEOUT. It fires in
at most PGSTAT_STAT_MIN_INTERVAL(500)ms to clean up pending
statistics update.

but I'm not sure what pending updates do you mean? Aren't we updating
the stats at the end of each transaction? At least that's what we've
been doing before, so maybe this patch changes that?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thank you very much for the testing.

At Mon, 26 Nov 2018 02:52:30 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <6c079a69-feba-e47c-7b85-8a9ff31adef3@2ndquadrant.com>

Hi,

Unfortunately, the patch does not apply anymore - it seems it got broken
by the changes to signal handling and/or removal of magic OIDs :-(

Big hit but was simple.

I've done a review and testing when applied on top of 10074651e335.

Firstly, the testing - I was wondering if the patch has some performance
impact, so I've done some testing with a read-only workload on large
number of tables (1k, 10k and 100k) while concurrently selecting data
from pg_stat_* catalogs at the same time.

In one case both workloads were running against the same database, in
another there were two separate databases (and the selects from stat
catalogs were running against an "empty" database with no use tables).

In both cases there were 8 clients doing selects from the user tables,
and 4 clients accessing the pg_stat_* catalogs.

For the "single database" case the results looks like this (this is just
patched / master throughput):

# of tables xact stats
------------------------------
1000 97.71% 98.76%
10000 100.38% 97.97%
100000 100.10% 98.50%

xact is throughput of the user workload (select from the large number of
tables) and stats is throughput for selects from system catalogs.

So pretty much no difference - 2% is within noise on this machine.

No increase by the number of tables seems suggesting that.

On two separate databases the results are a bit more interesting:

# of tables xact stats
-------------------------------
1000 100.49% 80.38%
10000 103.18% 80.28%
100000 100.85% 81.95%

For the main workload there's pretty much no difference, but for selects
from the stats catalogs there's ~20% drop in throughput. In absolute
numbers this means drop from ~670tps to ~550tps. I haven't investigated
this, but I suppose this is due to dshash seqscan being more expensive
than reading the data from file.

Thanks for finding that. The three seqscan loops in
pgstat_vacuum_stat cannot take such a long time, I think. I'll
investigate it.

I don't think any of this is an issue in practice, though. The important
thing is that there's no measurable impact on the regular workload.

Now, a couple of comments regarding individual parts of the patch.

0001-0003
---------

I do think 0001 - 0003 are ready, with some minor cosmetic issues:

1) I'd rephrase the last part of dshash_seq_init comment more like this:

* If consistent is set for dshash_seq_init, the all hash table
* partitions are locked in the requested mode (as determined by the
* exclusive flag), and the locks are held until the end of the scan.
* Otherwise the partition locks are acquired and released as needed
* during the scan (up to two partitions may be locked at the same time).

Replaced with this.

Maybe it should briefly explain what the consistency guarantees are (and
aren't), but considering we're not materially changing the existing
behavior probably is not really necessary.

Mmm. actually sequential scan is a new thing altogether, but..

2) I think the dshash_find_extended() signature should be more like
dshash_find(), i.e. just appending parameters instead of moving them
around unnecessarily. Perhaps we should add

Sure. It seems to have done by my off-lined finger;p Fixed.

Assert(nowait || !lock_acquired);

Using nowait=false with lock_acquired!=NULL does not seem sensible.

Agreed. Added.

3) I suppose this comment in postmaster.c is just copy-paste:

-#define BACKEND_TYPE_ARCHIVER	0x0010	/* bgworker process */
+#define BACKEND_TYPE_ARCHIVER	0x0010	/* archiver process */

Ugh! Fixed.

I wonder why wasn't archiver a regular auxiliary process already? It
seems like a fairly natural thing, so why not?

Perhaps it's just because it didn't need access to shared memory.

0004 (+0005 and 0007)
---------------------

This seems fine, but I have my doubts about two changes - removing of
stats_temp_directory and the IDLE_STATS_UPDATE_TIMEOUT thingy.

There's a couple of issues with the stats_temp_directory. Firstly, I
don't understand why it's spread over multiple parts of the patch. The
GUC is removed in 0004, the underlying variable is removed in 0005 and
then the docs are updated in 0007. If we really want to do this, it
should happen in a single patch.

Sure.

But the main question is - do we really want to do that? I understand
this directory was meant for the stats data we're moving to shared
memory, so removing it seems natural. But clearly it's used by
pg_stat_statements - 0005 fixes that, of course, but I wonder if there
are other extensions using it to store files?
It's not just about how intensive I/O to those files is, but this also
means the files will now be included in backups / pg_rewind, and maybe
that's not really desirable?

Maybe it's fine but I'm not quite convinced about it ...

It was also in my mind. Anyway sorry for the strange separation.

I was confused about pgstat_stat_directory (the names are
actually very confusing..). Addition to that pg_stat_statements
does *not* use the variable stats_temp_directory, but using
PG_STAT_TMP_DIR. pgstat_stat_directory was used only by
basebackup.c.

The GUC base variable pgstat_temp_directory is not extern'ed so
we can just remove it along with the GUC
definition. pgstat_stat_directory (it actually stores *temporary*
stats directory) was extern'ed in pgstat.h and PG_STAT_TMP_DIR is
defined in pgstat.h. They are not removed in the new version.
Finally 0005 no longer breaks any other bins, contribs and
externalextensions.

I'm not sure I understand what IDLE_STATS_UPDATE_TIMEOUT does. You've
described it as

This adds a new timeout IDLE_STATS_UPDATE_TIMEOUT. This works
similarly to IDLE_IN_TRANSACTIION_SESSION_TIMEOUT. It fires in
at most PGSTAT_STAT_MIN_INTERVAL(500)ms to clean up pending
statistics update.

but I'm not sure what pending updates do you mean? Aren't we updating
the stats at the end of each transaction? At least that's what we've
been doing before, so maybe this patch changes that?

Without the timeout, updates on shared memory happens at the same
rate with transaction traffic and it easily causes congestion. So
the update frequency is limited to the timtout in this patch and
the local statistics made by trasactions committed within the
timeout interval will be merged into one shared stats update. It
is the "pending statistics".

With the socket-based stats collector, it doesn't update the
temporary stats file with the interval not shorter than the
timeout. The update timeout seemingly behaves the same way with
the socket-based stats collector in the view of readers.

If local statistics is not fully processed at the end of the last
transaction. We don't have a chance to flush them before the next
transaction ends. So timeout is loaded if any "panding stats"
remains. (around postgres.c:4175) The pending stats are processed
forcibly in ProcessInterrupts().

postgres.c:4175

stats_timeout = pgstat_update_stat(false);
if (stats_timeout > 0)
{
disable_idle_stats_update_timeout = true;
enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT,
stats_timeout);

The attached are the new version addressing the all comments here
(except the 20% regression), and rebased.

v10-0001-sequential-scan-for-dshash.patch
v10-0002-Add-conditional-lock-feature-to-dshash.patch
fixed.
v10-0003-Make-archiver-process-an-auxiliary-process.patch
fixed.
v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 11/27/18 9:59 AM, Kyotaro HORIGUCHI wrote:

...>>
For the main workload there's pretty much no difference, but for selects
from the stats catalogs there's ~20% drop in throughput. In absolute
numbers this means drop from ~670tps to ~550tps. I haven't investigated
this, but I suppose this is due to dshash seqscan being more expensive
than reading the data from file.

Thanks for finding that. The three seqscan loops in
pgstat_vacuum_stat cannot take such a long time, I think. I'll
investigate it.

OK. I'm not sure this is related to pgstat_vacuum_stat - the slowdown
happens while querying the catalogs, so why would that trigger vacuum of
the stats? I may be missing something, of course.

FWIW, the "query statistics" test simply does this:

SELECT * FROM pg_stat_all_tables;
SELECT * FROM pg_stat_all_indexes;
SELECT * FROM pg_stat_user_indexes;
SELECT * FROM pg_stat_user_tables;
SELECT * FROM pg_stat_sys_tables;
SELECT * FROM pg_stat_sys_indexes;

and the slowdown happened even it was running on it's own (nothing else
running on the instance). Which mostly rules out concurrency issues with
the hash table locking etc.

I don't think any of this is an issue in practice, though. The important
thing is that there's no measurable impact on the regular workload.

Now, a couple of comments regarding individual parts of the patch.

0001-0003
---------

I do think 0001 - 0003 are ready, with some minor cosmetic issues:

1) I'd rephrase the last part of dshash_seq_init comment more like this:

* If consistent is set for dshash_seq_init, the all hash table
* partitions are locked in the requested mode (as determined by the
* exclusive flag), and the locks are held until the end of the scan.
* Otherwise the partition locks are acquired and released as needed
* during the scan (up to two partitions may be locked at the same time).

Replaced with this.

Maybe it should briefly explain what the consistency guarantees are (and
aren't), but considering we're not materially changing the existing
behavior probably is not really necessary.

Mmm. actually sequential scan is a new thing altogether, but..

Sure, there are new pieces. But does it significantly change consistency
guarantees when reading the stats? I don't think so - there was no
strict consistency guaranteed before (due to data interleaved with
inquiries, UDP throwing away packets under load, etc.). Based on the
discussion in this thread that seems to be the consensus.

2) I think the dshash_find_extended() signature should be more like
dshash_find(), i.e. just appending parameters instead of moving them
around unnecessarily. Perhaps we should add

Sure. It seems to have done by my off-lined finger;p Fixed.

;-)

0004 (+0005 and 0007)
---------------------

This seems fine, but I have my doubts about two changes - removing of
stats_temp_directory and the IDLE_STATS_UPDATE_TIMEOUT thingy.

There's a couple of issues with the stats_temp_directory. Firstly, I
don't understand why it's spread over multiple parts of the patch. The
GUC is removed in 0004, the underlying variable is removed in 0005 and
then the docs are updated in 0007. If we really want to do this, it
should happen in a single patch.

Sure.

But the main question is - do we really want to do that? I understand
this directory was meant for the stats data we're moving to shared
memory, so removing it seems natural. But clearly it's used by
pg_stat_statements - 0005 fixes that, of course, but I wonder if there
are other extensions using it to store files?
It's not just about how intensive I/O to those files is, but this also
means the files will now be included in backups / pg_rewind, and maybe
that's not really desirable?

Maybe it's fine but I'm not quite convinced about it ...

It was also in my mind. Anyway sorry for the strange separation.

I was confused about pgstat_stat_directory (the names are
actually very confusing..). Addition to that pg_stat_statements
does *not* use the variable stats_temp_directory, but using
PG_STAT_TMP_DIR. pgstat_stat_directory was used only by
basebackup.c.

The GUC base variable pgstat_temp_directory is not extern'ed so
we can just remove it along with the GUC
definition. pgstat_stat_directory (it actually stores *temporary*
stats directory) was extern'ed in pgstat.h and PG_STAT_TMP_DIR is
defined in pgstat.h. They are not removed in the new version.
Finally 0005 no longer breaks any other bins, contribs and
external extensions.

Great. I'll take a look.

I'm not sure I understand what IDLE_STATS_UPDATE_TIMEOUT does. You've
described it as

This adds a new timeout IDLE_STATS_UPDATE_TIMEOUT. This works
similarly to IDLE_IN_TRANSACTIION_SESSION_TIMEOUT. It fires in
at most PGSTAT_STAT_MIN_INTERVAL(500)ms to clean up pending
statistics update.

but I'm not sure what pending updates do you mean? Aren't we updating
the stats at the end of each transaction? At least that's what we've
been doing before, so maybe this patch changes that?

Without the timeout, updates on shared memory happens at the same
rate with transaction traffic and it easily causes congestion. So
the update frequency is limited to the timtout in this patch and
the local statistics made by trasactions committed within the
timeout interval will be merged into one shared stats update. It
is the "pending statistics".

With the socket-based stats collector, it doesn't update the
temporary stats file with the interval not shorter than the
timeout. The update timeout seemingly behaves the same way with
the socket-based stats collector in the view of readers.

If local statistics is not fully processed at the end of the last
transaction. We don't have a chance to flush them before the next
transaction ends. So timeout is loaded if any "panding stats"
remains. (around postgres.c:4175) The pending stats are processed
forcibly in ProcessInterrupts().

OK, thanks for the explanation. So it's essentially a protection against
stats from short transactions not being reported for a long time, when
the next transaction is long. For example we might end up with a
sequence of short transactions

T1: short, does not trigger IDLE_STATS_UPDATE_TIMEOUT -> local
T2: short, does not trigger IDLE_STATS_UPDATE_TIMEOUT -> local
...
TN: short, does not trigger IDLE_STATS_UPDATE_TIMEOUT -> local
T(N+1): long (say, several hours)

in which case stats from short ones are not reported until the end of
the long one. That makes sense.

That however raises the question - won't that also report some of the
stats from the last transaction? That would be a change compared to
current behavior, although I'm not sure it's undesirable - it's often
quite annoying that we don't receive stats from a transaction until it
completes. But I wonder - doesn't this affect pg_stat_xact catalogs?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/27/18 9:59 AM, Kyotaro HORIGUCHI wrote:

...

v10-0001-sequential-scan-for-dshash.patch
v10-0002-Add-conditional-lock-feature-to-dshash.patch
fixed.
v10-0003-Make-archiver-process-an-auxiliary-process.patch
fixed.
v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

I need to do a more thorough review of part 0006, but these patches
seems quite fine to me. I'd however merge 0007 into the other relevant
parts (it seems like a mix of docs changes for 0004, 0005 and 0006).

Thinking about it a bit more, I'm wondering if we need to keep 0004 and
0005 separate. My understanding is that the stats_temp_directory is used
only from the stats collector, so it probably does not make much sense
to keep it after 0004. We may also keep it separate and then commit both
0004 and 0005 together, of course. What do you think.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Nov-28, Tomas Vondra wrote:

v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

I need to do a more thorough review of part 0006, but these patches
seems quite fine to me. I'd however merge 0007 into the other relevant
parts (it seems like a mix of docs changes for 0004, 0005 and 0006).

Looking at 0001 - 0003 it seems OK to keep each as separate commits, but
I suggest to have 0004+0006 be a single commit, mostly because
introducing a bunch of "new" code in 0004 and then moving it over to
bestatus.c in 0006 makes "git blame" doubly painful. And I think
committing 0005 and not 0007 makes the documentation temporarily buggy,
so I see no reason to think of this as two commits, one being 0004+0006
and the other 0005+0007. And even those could conceivably be pushed
together instead of as a single patch. (But be sure to push very early
in your work day, to have plenty of time to deal with any resulting
buildfarm problems.)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/29/18 1:18 PM, Alvaro Herrera wrote:

On 2018-Nov-28, Tomas Vondra wrote:

v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

I need to do a more thorough review of part 0006, but these patches
seems quite fine to me. I'd however merge 0007 into the other relevant
parts (it seems like a mix of docs changes for 0004, 0005 and 0006).

Looking at 0001 - 0003 it seems OK to keep each as separate commits, but
I suggest to have 0004+0006 be a single commit, mostly because
introducing a bunch of "new" code in 0004 and then moving it over to
bestatus.c in 0006 makes "git blame" doubly painful. And I think
committing 0005 and not 0007 makes the documentation temporarily buggy,
so I see no reason to think of this as two commits, one being 0004+0006
and the other 0005+0007. And even those could conceivably be pushed
together instead of as a single patch. (But be sure to push very early
in your work day, to have plenty of time to deal with any resulting
buildfarm problems.)

Kyotaro-san, do you agree with committing the patch the way Alvaro
proposed? That is, 0001-0003 as separate commits, and 0004+0006 and
0005+0007 together. The plan seems reasonable to me.

FWIW I see cputube reports some build failures on Windows:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.26736#L3135

If I understand it correctly, it complains about this line in postmaster.c:

extern pgsocket pgStatSock;

which seems to only affect EXEC_BACKEND (including Win32). ISTM we
should get rid of all pgStatSock references, per the attached fix.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2019-01-01 18:39:12 +0100, Tomas Vondra wrote:

On 11/29/18 1:18 PM, Alvaro Herrera wrote:

On 2018-Nov-28, Tomas Vondra wrote:

v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

I need to do a more thorough review of part 0006, but these patches
seems quite fine to me. I'd however merge 0007 into the other relevant
parts (it seems like a mix of docs changes for 0004, 0005 and 0006).

Looking at 0001 - 0003 it seems OK to keep each as separate commits, but
I suggest to have 0004+0006 be a single commit, mostly because
introducing a bunch of "new" code in 0004 and then moving it over to
bestatus.c in 0006 makes "git blame" doubly painful. And I think
committing 0005 and not 0007 makes the documentation temporarily buggy,
so I see no reason to think of this as two commits, one being 0004+0006
and the other 0005+0007. And even those could conceivably be pushed
together instead of as a single patch. (But be sure to push very early
in your work day, to have plenty of time to deal with any resulting
buildfarm problems.)

Kyotaro-san, do you agree with committing the patch the way Alvaro
proposed? That is, 0001-0003 as separate commits, and 0004+0006 and
0005+0007 together. The plan seems reasonable to me.

Do you guys think these patches are ready already? I'm a bit doubtful, and
failures here could have quite wide-ranging symptoms.

Greetings,

Andres Freund

On 1/1/19 7:03 PM, Andres Freund wrote:

Hi,

On 2019-01-01 18:39:12 +0100, Tomas Vondra wrote:

On 11/29/18 1:18 PM, Alvaro Herrera wrote:

On 2018-Nov-28, Tomas Vondra wrote:

v10-0004-Shared-memory-based-stats-collector.patch
updated not to touch guc.
v10-0005-Remove-the-GUC-stats_temp_directory.patch
collected all guc-related changes.
updated not to break other programs.
v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
basebackup.c requires both bestats.h and pgstat.h
v10-0007-Documentation-update.patch
small change related to 0005.

I need to do a more thorough review of part 0006, but these patches
seems quite fine to me. I'd however merge 0007 into the other relevant
parts (it seems like a mix of docs changes for 0004, 0005 and 0006).

Looking at 0001 - 0003 it seems OK to keep each as separate commits, but
I suggest to have 0004+0006 be a single commit, mostly because
introducing a bunch of "new" code in 0004 and then moving it over to
bestatus.c in 0006 makes "git blame" doubly painful. And I think
committing 0005 and not 0007 makes the documentation temporarily buggy,
so I see no reason to think of this as two commits, one being 0004+0006
and the other 0005+0007. And even those could conceivably be pushed
together instead of as a single patch. (But be sure to push very early
in your work day, to have plenty of time to deal with any resulting
buildfarm problems.)

Kyotaro-san, do you agree with committing the patch the way Alvaro
proposed? That is, 0001-0003 as separate commits, and 0004+0006 and
0005+0007 together. The plan seems reasonable to me.

Do you guys think these patches are ready already? I'm a bit doubtful, and
failures here could have quite wide-ranging symptoms.

I agree it's a sensitive part of the code, so additional reviews would
be welcome of course. I've done as much review and testing as possible,
and overall it seems in a fairly good shape. Do you have any particular
concerns / ideas what to look for?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Jan-01, Tomas Vondra wrote:

I agree it's a sensitive part of the code, so additional reviews would
be welcome of course. I've done as much review and testing as possible,
and overall it seems in a fairly good shape. Do you have any particular
concerns / ideas what to look for?

I haven't reviewed this patch thoroughly.

Shall we do a triage run over the complete commitfest to determine the
highest priority items that we should put extra effort into reviewing?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

The patch needs rebasing, as it got broken by 285d8e1205, and there's
some other minor bitrot.

On 11/27/18 4:40 PM, Tomas Vondra wrote:

On 11/27/18 9:59 AM, Kyotaro HORIGUCHI wrote:

...>>
For the main workload there's pretty much no difference, but for
selects from the stats catalogs there's ~20% drop in throughput.
In absolute numbers this means drop from ~670tps to ~550tps. I
haven't investigated this, but I suppose this is due to dshash
seqscan being more expensive than reading the data from file.

Thanks for finding that. The three seqscan loops in
pgstat_vacuum_stat cannot take such a long time, I think. I'll
investigate it.

OK. I'm not sure this is related to pgstat_vacuum_stat - the
slowdown happens while querying the catalogs, so why would that
trigger vacuum of the stats? I may be missing something, of course.

FWIW, the "query statistics" test simply does this:

  SELECT * FROM pg_stat_all_tables;
  SELECT * FROM pg_stat_all_indexes;
  SELECT * FROM pg_stat_user_indexes;
  SELECT * FROM pg_stat_user_tables;
  SELECT * FROM pg_stat_sys_tables;
  SELECT * FROM pg_stat_sys_indexes;

and the slowdown happened even it was running on it's own (nothing
else running on the instance). Which mostly rules out concurrency
issues with the hash table locking etc.

Did you have time to investigate the slowdown?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thank you very much for reviewing this and sorry for the absense.

At Sun, 20 Jan 2019 18:13:04 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <b760035b-1941-38bb-5e84-c2fbc63fef6b@2ndquadrant.com>

Hi,

The patch needs rebasing, as it got broken by 285d8e1205, and there's
some other minor bitrot.

The most affected part was 0006 because of file splitting, but
actually only the follwing four (actually three) commits
affected.

42e2a58071 Fix typos in documentation and for one wait event
97c39498e5 Update copyright for 2019
578b229718 Remove WITH OIDS support, change oid catalog column visibility.
(125f551c8b Leave SIGTTIN/SIGTTOU signal handling alone in postmaster child processes.)

The last one is not relevant because stats collector is no longer
a process.

This contains the EXEC_BACKEND related bug pointed by

/messages/by-id/854d6d91-f2f3-e391-f0fc-064db51b391e@2ndquadrant.com

On 11/27/18 4:40 PM, Tomas Vondra wrote:

On 11/27/18 9:59 AM, Kyotaro HORIGUCHI wrote:

...>>
For the main workload there's pretty much no difference, but for
selects from the stats catalogs there's ~20% drop in throughput.
In absolute numbers this means drop from ~670tps to ~550tps. I
haven't investigated this, but I suppose this is due to dshash
seqscan being more expensive than reading the data from file.

Thanks for finding that. The three seqscan loops in
pgstat_vacuum_stat cannot take such a long time, I think. I'll
investigate it.

OK. I'm not sure this is related to pgstat_vacuum_stat - the
slowdown happens while querying the catalogs, so why would that
trigger vacuum of the stats? I may be missing something, of course.

FWIW, the "query statistics" test simply does this:

  SELECT * FROM pg_stat_all_tables;
  SELECT * FROM pg_stat_all_indexes;
  SELECT * FROM pg_stat_user_indexes;
  SELECT * FROM pg_stat_user_tables;
  SELECT * FROM pg_stat_sys_tables;
  SELECT * FROM pg_stat_sys_indexes;

and the slowdown happened even it was running on it's own (nothing
else running on the instance). Which mostly rules out concurrency
issues with the hash table locking etc.

Did you have time to investigate the slowdown?

It seems to me that the slowdown comes from local caching in
snapshot_statentry in several ways.

It searches local hash (HTAB), then shared hash (dshash) if not
found and copies the found entry into local hash (action A). *If*
the second reference in a transaction comes, HTAB returns the
result (action B). But it mostly takes action A in frequent-short
transactions. It can be reduced to the update interval of shared
stats, but it would be shorter if many backends runs.

Another bottle neck found in pgstat_fetch_stat_tabentry. It calls
pgstat_fetch_stat_dbentry() too often. It can be largely reduced.

A quick (and dirty) fix of the aboves reduced the slowdown
roughly by half. (59tps(master)->48tps(current)->54tps(the fix))

I'll reconsider the referer side of the stats.

I didn't merge the suggested two pairs of commits. I'll do that
after adressing the slowdown issue.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Mon, 21 Jan 2019 21:19:07 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190121.211907.59625409.horiguchi.kyotaro@lab.ntt.co.jp>

I'll reconsider the referer side of the stats.

The most significant cause of the slowdown is repeated search for
non-existent entries both on local and shared hash each time.

Negative cache in addition to cache expiration interval
eliminates the slowdown.

1000 times repetition with -O2 binary:

master : 124.99 tps
patched: 125.48 tps (+0.4%)

I didn't merge the suggested two pairs of commits. I'll do that
after adressing the slowdown issue.

I agree to commiting 0001-0003 separately. In the attached patch
set, old 0004+0006 are merged as 0004 and old 0005+0007 are
merged as new 0005.

Changed cacheing policy.
Expired at every xact end
-> Keeps at least for PGSTAT_STAT_MIN_INTERVAL (500ms).

Added negative cache feature (snapshot_statentry).

Improved separation between pgstat and bestat (separated AtEOXact_* functions).

Fixed doubious memory context usage.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jan 22, 2019 at 03:48:02PM +0900, Kyotaro HORIGUCHI wrote:

Fixed doubious memory context usage.

That's quite something that we have here for 0005:
84 files changed, 6588 insertions(+), 7501 deletions(-)

Moved to next CF for now.
--
Michael

Hi,

On 2018-11-12 20:10:42 +0900, Kyotaro HORIGUCHI wrote:

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7eed5866d2..e52ae54821 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8587,9 +8587,9 @@ LogCheckpointEnd(bool restartpoint)
&sync_secs, &sync_usecs);

/* Accumulate checkpoint timing summary data, in milliseconds. */
-	BgWriterStats.m_checkpoint_write_time +=
+	BgWriterStats.checkpoint_write_time +=
write_secs * 1000 + write_usecs / 1000;
-	BgWriterStats.m_checkpoint_sync_time +=
+	BgWriterStats.checkpoint_sync_time +=
sync_secs * 1000 + sync_usecs / 1000;

Why does this patch do renames like this in the same entry as actual
functional changes?

@@ -1273,16 +1276,22 @@ do_start_worker(void)
break;
}
}
-		if (skipit)
-			continue;
+		if (!skipit)
+		{
+			/* Remember the db with oldest autovac time. */
+			if (avdb == NULL ||
+				tmp->adw_entry->last_autovac_time <
+				avdb->adw_entry->last_autovac_time)
+			{
+				if (avdb)
+					pfree(avdb->adw_entry);
+				avdb = tmp;
+			}
+		}

-		/*
-		 * Remember the db with oldest autovac time.  (If we are here, both
-		 * tmp->entry and db->entry must be non-null.)
-		 */
-		if (avdb == NULL ||
-			tmp->adw_entry->last_autovac_time < avdb->adw_entry->last_autovac_time)
-			avdb = tmp;
+		/* Immediately free it if not used */
+		if(avdb != tmp)
+			pfree(tmp->adw_entry);
}

This looks like another unrelated refactoring. Don't do this.

/* Transfer stats counts into pending pgstats message */
-	BgWriterStats.m_buf_written_backend += CheckpointerShmem->num_backend_writes;
-	BgWriterStats.m_buf_fsync_backend += CheckpointerShmem->num_backend_fsync;
+	BgWriterStats.buf_written_backend += CheckpointerShmem->num_backend_writes;
+	BgWriterStats.buf_fsync_backend += CheckpointerShmem->num_backend_fsync;

More unrelated renaming. I'll stop mentioning this in the rest of this
patch, but really don't do this, makes such a large unnecessary harder
to review.

pgstat.c needs a header comment explaining the architecture of the
approach.

+/*
+ * Operation mode of pgstat_get_db_entry.
+ */
+#define PGSTAT_FETCH_SHARED	0
+#define PGSTAT_FETCH_EXCLUSIVE	1
+#define	PGSTAT_FETCH_NOWAIT 2
+
+typedef enum
+{

Please don't create anonymous enums that are then typedef'd to a
name. The underlying name and the one not scoped to typedefs shoudl be
the same.

+/*
+ *  report withholding facility.
+ *
+ *  some report items are withholded if required lock is not acquired
+ *  immediately.
+ */

This comment needs polishing. The variables are named _pending_, but the
comment talks about withholding - which doesn't seem like an apt name.

/*
* Structures in which backends store per-table info that's waiting to be
@@ -189,18 +189,14 @@ typedef struct TabStatHashEntry
* Hash table for O(1) t_id -> tsa_entry lookup
*/
static HTAB *pgStatTabHash = NULL;
+static HTAB *pgStatPendingTabHash = NULL;

/*
* Backends store per-function info that's waiting to be sent to the collector
* in this hash table (indexed by function OID).
*/
static HTAB *pgStatFunctions = NULL;
-
-/*
- * Indicates if backend has some function stats that it hasn't yet
- * sent to the collector.
- */
-static bool have_function_stats = false;
+static HTAB *pgStatPendingFunctions = NULL;

So this patch leaves us with a pgStatFunctions that has a comment
explaining it's about "waiting to be sent" stats, and then additionally
a pgStatPendingFunctions?

/* ------------------------------------------------------------
* Public functions used by backends follow
@@ -802,41 +436,107 @@ allow_immediate_pgstat_restart(void)
* pgstat_report_stat() -
*
*	Must be called by processes that performs DML: tcop/postgres.c, logical
- *	receiver processes, SPI worker, etc. to send the so far collected
- *	per-table and function usage statistics to the collector.  Note that this
- *	is called only when not within a transaction, so it is fair to use
- *	transaction stop time as an approximation of current time.
- * ----------
+ *	receiver processes, SPI worker, etc. to apply the so far collected
+ *	per-table and function usage statistics to the shared statistics hashes.
+ *
+ *  This requires taking some locks on the shared statistics hashes and some

Weird mix of different indentation.

+ *  of updates may be withholded on lock failure. Pending updates are
+ *  retried in later call of this function and finally cleaned up by calling
+ *  this function with force = true or PGSTAT_STAT_MAX_INTERVAL milliseconds
+ *  was elapsed since last cleanup. On the other hand updates by regular

s/was/has/

+
+	/* Forecibly update other stats if any. */

s/Forecibly/Forcibly/

Typo aside, what does forcibly mean here?

/*
-	 * Scan through the TabStatusArray struct(s) to find tables that actually
-	 * have counts, and build messages to send.  We have to separate shared
-	 * relations from regular ones because the databaseid field in the message
-	 * header has to depend on that.
+	 * XX: We cannot lock two dshash entries at once. Since we must keep lock

Typically we use three XXX (or alternatively NB:).

+	 * while tables stats are being updated we have no choice other than
+	 * separating jobs for shared table stats and that of egular tables.

s/egular/regular/

+	 * Looping over the array twice isapparently ineffcient and more efficient
+	 * way is expected.
*/

s/isapparently/is apparently/
s/ineffcient/inefficient/

But I don't know what this sentence is trying to say precisely. Are you
saying this cannot be committed unless this is fixed?

Nor do I understand why it's actually relevant that we cannot lock two
dshash entries at once. The same table is never shared and unshared,
no?

+/*
+ * Subroutine for pgstat_update_stat.
+ *
+ * Appies table stats in table status array merging with pending stats if any.

s/Appies/Applies/

+ * If force is true waits until required locks to be acquired. Elsewise stats

s/elsewise/ohterwise/

+ * merged stats as pending sats and it will be processed in the next chance.

s/sats/stats/
s/in the next change/at the next chance/

+			/* if pending update exists, it should be applied along with */
+			if (pgStatPendingTabHash != NULL)

Why is any of this done if there's no pending data?

{
-				pgstat_send_tabstat(this_msg);
-				this_msg->m_nentries = 0;
+				pentry = hash_search(pgStatPendingTabHash,
+									 (void *) entry, HASH_FIND, NULL);
+
+				if (pentry)
+				{
+					/* merge new update into pending updates */
+					pgstat_merge_tabentry(pentry, entry, false);
+					entry = pentry;
+				}
+			}
+
+			/* try to apply the merged stats */
+			if (pgstat_apply_tabstat(cxt, entry, !force))
+			{
+				/* succeeded. remove it if it was pending stats */
+				if (pentry && entry != pentry)
+					hash_search(pgStatPendingTabHash,
+								(void *) pentry, HASH_REMOVE, NULL);

Huh, how can entry != pentry in the case of pending stats? They're
literally set to the same value above?

+			else if (!pentry)
+			{
+				/* failed and there was no pending entry, create new one. */
+				bool found;
+
+				if (pgStatPendingTabHash == NULL)
+				{
+					HASHCTL		ctl;
+
+					memset(&ctl, 0, sizeof(ctl));
+					ctl.keysize = sizeof(Oid);
+					ctl.entrysize = sizeof(PgStat_TableStatus);
+					pgStatPendingTabHash =
+						hash_create("pgstat pending table stats hash",
+									TABSTAT_QUANTUM,
+									&ctl,
+									HASH_ELEM | HASH_BLOBS);
+				}
+
+				pentry = hash_search(pgStatPendingTabHash,
+									 (void *) entry, HASH_ENTER, &found);
+				Assert (!found);
+
+				*pentry = *entry;
}
}
-		/* zero out TableStatus structs after use */
-		MemSet(tsa->tsa_entries, 0,
-			   tsa->tsa_used * sizeof(PgStat_TableStatus));
-		tsa->tsa_used = 0;
+	}

I don't understand why we do this at all.

+ if (cxt->tabhash)
+ dshash_detach(cxt->tabhash);

Huh, why do we detach here?

+/*
+ * pgstat_apply_tabstat: update shared stats entry using given entry
+ *
+ * If nowait is true, just returns false on lock failure.  Dshashes for table
+ * and function stats are kept attached and stored in ctx. The caller must
+ * detach them after use.
+ */
+bool
+pgstat_apply_tabstat(pgstat_apply_tabstat_context *cxt,
+					 PgStat_TableStatus *entry, bool nowait)
+{
+	Oid dboid = entry->t_shared ? InvalidOid : MyDatabaseId;
+	int		table_mode = PGSTAT_FETCH_EXCLUSIVE;
+	bool updated = false;
+
+	if (nowait)
+		table_mode |= PGSTAT_FETCH_NOWAIT;
+
+	/*
+	 * We need to keep lock on dbentries for regular tables to avoid race
+	 * condition with drop database. So we hold it in the context variable. We
+	 * don't need that for shared tables.
+	 */
+	if (!cxt->dbentry)
+		cxt->dbentry = pgstat_get_db_entry(dboid, table_mode, NULL);

Oh, wait, what? *That's* the reason why we need to hold a lock on a
second entry?

Uhm, how can this actually be an issue? If we apply pending stats, we're
connected to the database, it therefore cannot be dropped while we're
applying stats, no?

+	/* attach shared stats table if not yet */
+	if (!cxt->tabhash)
+	{
+		/* apply database stats  */
+		if (!entry->t_shared)
+		{
+			/* Update database-wide stats  */
+			cxt->dbentry->n_xact_commit += pgStatXactCommit;
+			cxt->dbentry->n_xact_rollback += pgStatXactRollback;
+			cxt->dbentry->n_block_read_time += pgStatBlockReadTime;
+			cxt->dbentry->n_block_write_time += pgStatBlockWriteTime;
+			pgStatXactCommit = 0;
+			pgStatXactRollback = 0;
+			pgStatBlockReadTime = 0;
+			pgStatBlockWriteTime = 0;
+		}

Uh, this seems to have nothing to do with "attach shared stats table if
not yet".

+	/* create hash if not yet */
+	if (dbentry->functions == DSM_HANDLE_INVALID)
+	{
+		funchash = dshash_create(area, &dsh_funcparams, 0);
+		dbentry->functions = dshash_get_hash_table_handle(funchash);
+	}
+	else
+		funchash = dshash_attach(area, &dsh_funcparams, dbentry->functions, 0);

Why is this created on-demand?

+	/*
+	 * First, we empty the transaction stats. Just move numbers to pending
+	 * stats if any. Elsewise try to directly update the shared stats but
+	 * create a new pending entry on lock failure.
+	 */
+	if (pgStatFunctions)

I don't understand why we have both pgStatFunctions and pgStatFunctions
and pgStatPendingFunctions (and the same for other such pairs). That
seems to make no sense to me. The comments for the former literally are:
/*
* Backends store per-function info that's waiting to be sent to the collector
* in this hash table (indexed by function OID).
*/

static HTAB *
pgstat_collect_oids(Oid catalogid)
@@ -1241,62 +1173,54 @@ pgstat_collect_oids(Oid catalogid)
/* ----------
* pgstat_drop_database() -
*
- *	Tell the collector that we just dropped a database.
- *	(If the message gets lost, we will still clean the dead DB eventually
- *	via future invocations of pgstat_vacuum_stat().)
+ *	Remove entry for the database that we just dropped.
+ *
+ *  If some stats update happens after this, this entry will re-created but
+ *	we will still clean the dead DB eventually via future invocations of
+ *	pgstat_vacuum_stat().
* ----------
*/
+
void
pgstat_drop_database(Oid databaseid)
{

Mixed indentation, added newline.

+/*
+ * snapshot_statentry() - Find an entriy from source dshash.
+ *

s/entriy/entry/

Ok, getting too tired now. Two AM in an airport lounge is not the
easiest place and time to concentrate...

I don't think this is all that close to being committable :(

Greetings,

Andres Freund

Hi Kyatoro,

On 2019-02-07 13:10:08 -0800, Andres Freund wrote:

I don't think this is all that close to being committable :(

Are you planning to update this soon? I think this needs to be improved
pretty quickly to have any shot at getting into v12. I'm willing to put
in some resources towards that, but I definitely don't have the
resources to entirely polish it from my end.

Greetings,

Andres Freund

Hello. Thank you for the comment.

At Thu, 7 Feb 2019 13:10:08 -0800, Andres Freund <andres@anarazel.de> wrote in <20190207211008.nc3axviivmcoaluq@alap3.anarazel.de>

Hi,

On 2018-11-12 20:10:42 +0900, Kyotaro HORIGUCHI wrote:

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7eed5866d2..e52ae54821 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8587,9 +8587,9 @@ LogCheckpointEnd(bool restartpoint)
&sync_secs, &sync_usecs);

/* Accumulate checkpoint timing summary data, in milliseconds. */
-	BgWriterStats.m_checkpoint_write_time +=
+	BgWriterStats.checkpoint_write_time +=
write_secs * 1000 + write_usecs / 1000;
-	BgWriterStats.m_checkpoint_sync_time +=
+	BgWriterStats.checkpoint_sync_time +=
sync_secs * 1000 + sync_usecs / 1000;

Why does this patch do renames like this in the same entry as actual
functional changes?

Just because it is no longer "messages". I'm ok to preserve them
as historcal names. Reverted.

@@ -1273,16 +1276,22 @@ do_start_worker(void)
break;
}
}
-		if (skipit)
-			continue;
+		if (!skipit)
+		{
+			/* Remember the db with oldest autovac time. */
+			if (avdb == NULL ||
+				tmp->adw_entry->last_autovac_time <
+				avdb->adw_entry->last_autovac_time)
+			{
+				if (avdb)
+					pfree(avdb->adw_entry);
+				avdb = tmp;
+			}
+		}

-		/*
-		 * Remember the db with oldest autovac time.  (If we are here, both
-		 * tmp->entry and db->entry must be non-null.)
-		 */
-		if (avdb == NULL ||
-			tmp->adw_entry->last_autovac_time < avdb->adw_entry->last_autovac_time)
-			avdb = tmp;
+		/* Immediately free it if not used */
+		if(avdb != tmp)
+			pfree(tmp->adw_entry);
}

This looks like another unrelated refactoring. Don't do this.

Rewrited it to less invasive way.

/* Transfer stats counts into pending pgstats message */
-	BgWriterStats.m_buf_written_backend += CheckpointerShmem->num_backend_writes;
-	BgWriterStats.m_buf_fsync_backend += CheckpointerShmem->num_backend_fsync;
+	BgWriterStats.buf_written_backend += CheckpointerShmem->num_backend_writes;
+	BgWriterStats.buf_fsync_backend += CheckpointerShmem->num_backend_fsync;

More unrelated renaming. I'll stop mentioning this in the rest of this
patch, but really don't do this, makes such a large unnecessary harder
to review.

AFAICS it is done only to the struct. I reverted all of them.

pgstat.c needs a header comment explaining the architecture of the
approach.

I write some. Please check it.

+/*
+ * Operation mode of pgstat_get_db_entry.
+ */
+#define PGSTAT_FETCH_SHARED	0
+#define PGSTAT_FETCH_EXCLUSIVE	1
+#define	PGSTAT_FETCH_NOWAIT 2
+
+typedef enum
+{

Please don't create anonymous enums that are then typedef'd to a
name. The underlying name and the one not scoped to typedefs shoudl be
the same.

Sorry. I named the struct PgStat_TableLookupState and typedef'ed
with the same name.

+/*
+ *  report withholding facility.
+ *
+ *  some report items are withholded if required lock is not acquired
+ *  immediately.
+ */

This comment needs polishing. The variables are named _pending_, but the
comment talks about withholding - which doesn't seem like an apt name.

The comment was updated at v12, but it still doesn't
good. Rewrite as follows:

| * variables signals that the backend has some numbers that are waiting to be
| * written to shared stats.

/*
* Structures in which backends store per-table info that's waiting to be
@@ -189,18 +189,14 @@ typedef struct TabStatHashEntry
* Hash table for O(1) t_id -> tsa_entry lookup
*/
static HTAB *pgStatTabHash = NULL;
+static HTAB *pgStatPendingTabHash = NULL;

/*
* Backends store per-function info that's waiting to be sent to the collector
* in this hash table (indexed by function OID).
*/
static HTAB *pgStatFunctions = NULL;
-
-/*
- * Indicates if backend has some function stats that it hasn't yet
- * sent to the collector.
- */
-static bool have_function_stats = false;
+static HTAB *pgStatPendingFunctions = NULL;

So this patch leaves us with a pgStatFunctions that has a comment
explaining it's about "waiting to be sent" stats, and then additionally
a pgStatPendingFunctions?

Mmm. Thanks . I changed the comment and separated pgSTatPending*
stuff from there and merged with pgstat_pending_*. And unified
the naming.

/* ------------------------------------------------------------
* Public functions used by backends follow
@@ -802,41 +436,107 @@ allow_immediate_pgstat_restart(void)
* pgstat_report_stat() -
*
*	Must be called by processes that performs DML: tcop/postgres.c, logical
- *	receiver processes, SPI worker, etc. to send the so far collected
- *	per-table and function usage statistics to the collector.  Note that this
- *	is called only when not within a transaction, so it is fair to use
- *	transaction stop time as an approximation of current time.
- * ----------
+ *	receiver processes, SPI worker, etc. to apply the so far collected
+ *	per-table and function usage statistics to the shared statistics hashes.
+ *
+ *  This requires taking some locks on the shared statistics hashes and some

Weird mix of different indentation.

Fixed. Unifed to tabs.

+ *  of updates may be withholded on lock failure. Pending updates are
+ *  retried in later call of this function and finally cleaned up by calling
+ *  this function with force = true or PGSTAT_STAT_MAX_INTERVAL milliseconds
+ *  was elapsed since last cleanup. On the other hand updates by regular

s/was/has/

Ugg. Fixed.

+
+	/* Forecibly update other stats if any. */

s/Forecibly/Forcibly/

Typo aside, what does forcibly mean here?

Meant that it should wait for lock to be acquired. But I don't
recall why. Changed it to follow "force" flag.

/*
-	 * Scan through the TabStatusArray struct(s) to find tables that actually
-	 * have counts, and build messages to send.  We have to separate shared
-	 * relations from regular ones because the databaseid field in the message
-	 * header has to depend on that.
+	 * XX: We cannot lock two dshash entries at once. Since we must keep lock

Typically we use three XXX (or alternatively NB:).

Fixed. I thought that the number 'X's represents how bad it is.
(Just kidding).

+	 * while tables stats are being updated we have no choice other than
+	 * separating jobs for shared table stats and that of egular tables.

s/egular/regular/

Fixed

+	 * Looping over the array twice isapparently ineffcient and more efficient
+	 * way is expected.
*/

s/isapparently/is apparently/
s/ineffcient/inefficient/

But I don't know what this sentence is trying to say precisely. Are you
saying this cannot be committed unless this is fixed?

Just I wanted to say I'd happy if could do all in a
loop. So.. I rewote it as the follows.

| * Flush pending stats separately for regular tables and shared tables
| * since we cannot hold locks on two dshash entries at once.

Nor do I understand why it's actually relevant that we cannot lock two
dshash entries at once. The same table is never shared and unshared,
no?

Ah, I understood. A backend acculunates statistics on shared
tables, and tables of the connecting database. Shared table
entries are stored under database entry with id = 0 so we need to
use database entries of id = 0 and id = MyDatabaseId stored in
db_stats dshash.....

Ah, I found now that database entry for shared tables necessarily
in the db_stats dshash. Okey. I'll separate shared dbentry from
db_stats hash in the next version.

+/*
+ * Subroutine for pgstat_update_stat.
+ *
+ * Appies table stats in table status array merging with pending stats if any.

s/Appies/Applies/

+ * If force is true waits until required locks to be acquired. Elsewise stats

s/elsewise/ohterwise/

+ * merged stats as pending sats and it will be processed in the next chance.

s/sats/stats/
s/in the next change/at the next chance/

Sorry for many silly (but even difficult for me) mistakes. All
fixed. I'll check all by ispell later.

+			/* if pending update exists, it should be applied along with */
+			if (pgStatPendingTabHash != NULL)

Why is any of this done if there's no pending data?

Sorry, but I don't follow it. We cannot do anything to what doesn't exist.

{
-				pgstat_send_tabstat(this_msg);
-				this_msg->m_nentries = 0;
+				pentry = hash_search(pgStatPendingTabHash,
+									 (void *) entry, HASH_FIND, NULL);
+
+				if (pentry)
+				{
+					/* merge new update into pending updates */
+					pgstat_merge_tabentry(pentry, entry, false);
+					entry = pentry;
+				}
+			}
+
+			/* try to apply the merged stats */
+			if (pgstat_apply_tabstat(cxt, entry, !force))
+			{
+				/* succeeded. remove it if it was pending stats */
+				if (pentry && entry != pentry)
+					hash_search(pgStatPendingTabHash,
+								(void *) pentry, HASH_REMOVE, NULL);

Huh, how can entry != pentry in the case of pending stats? They're
literally set to the same value above?

Seems right. Removed. I might have done something complex before..

+			else if (!pentry)
+			{
+				/* failed and there was no pending entry, create new one. */
+				bool found;
+
+				if (pgStatPendingTabHash == NULL)
+				{
+					HASHCTL		ctl;
+
+					memset(&ctl, 0, sizeof(ctl));
+					ctl.keysize = sizeof(Oid);
+					ctl.entrysize = sizeof(PgStat_TableStatus);
+					pgStatPendingTabHash =
+						hash_create("pgstat pending table stats hash",
+									TABSTAT_QUANTUM,
+									&ctl,
+									HASH_ELEM | HASH_BLOBS);
+				}
+
+				pentry = hash_search(pgStatPendingTabHash,
+									 (void *) entry, HASH_ENTER, &found);
+				Assert (!found);
+
+				*pentry = *entry;
}
}
-		/* zero out TableStatus structs after use */
-		MemSet(tsa->tsa_entries, 0,
-			   tsa->tsa_used * sizeof(PgStat_TableStatus));
-		tsa->tsa_used = 0;
+	}

I don't understand why we do this at all.

If we didn't have pending stats of the table, and failed to apply
the stats in TSA, we should move it into pending stats hash.

Though we could merge numbers in TSA into pending hash, then
flush pending hash, I prefer to avoid useless relocation of stats
numbers from TSA to pending stats hash. Does it make sense?

+ if (cxt->tabhash)
+ dshash_detach(cxt->tabhash);

Huh, why do we detach here?

To release the lock on cxt->dbentry. It may be destroyed.

+/*
+ * pgstat_apply_tabstat: update shared stats entry using given entry
+ *
+ * If nowait is true, just returns false on lock failure.  Dshashes for table
+ * and function stats are kept attached and stored in ctx. The caller must
+ * detach them after use.
+ */
+bool
+pgstat_apply_tabstat(pgstat_apply_tabstat_context *cxt,
+					 PgStat_TableStatus *entry, bool nowait)
+{
+	Oid dboid = entry->t_shared ? InvalidOid : MyDatabaseId;
+	int		table_mode = PGSTAT_FETCH_EXCLUSIVE;
+	bool updated = false;
+
+	if (nowait)
+		table_mode |= PGSTAT_FETCH_NOWAIT;
+
+	/*
+	 * We need to keep lock on dbentries for regular tables to avoid race
+	 * condition with drop database. So we hold it in the context variable. We
+	 * don't need that for shared tables.
+	 */
+	if (!cxt->dbentry)
+		cxt->dbentry = pgstat_get_db_entry(dboid, table_mode, NULL);

Oh, wait, what? *That's* the reason why we need to hold a lock on a
second entry?

Yeah, one of the reasons.

Uhm, how can this actually be an issue? If we apply pending stats, we're
connected to the database, it therefore cannot be dropped while we're
applying stats, no?

Ah!!!!uch. You're right. I'll consider that in the next version
soon. Thnaks for the insight.

+	/* attach shared stats table if not yet */
+	if (!cxt->tabhash)
+	{
+		/* apply database stats  */
+		if (!entry->t_shared)
+		{
+			/* Update database-wide stats  */
+			cxt->dbentry->n_xact_commit += pgStatXactCommit;
+			cxt->dbentry->n_xact_rollback += pgStatXactRollback;
+			cxt->dbentry->n_block_read_time += pgStatBlockReadTime;
+			cxt->dbentry->n_block_write_time += pgStatBlockWriteTime;
+			pgStatXactCommit = 0;
+			pgStatXactRollback = 0;
+			pgStatBlockReadTime = 0;
+			pgStatBlockWriteTime = 0;
+		}

Uh, this seems to have nothing to do with "attach shared stats table if
not yet".

It's because the database stats needs to be applied once and
attaching tabhash happens once for a database. But, actually it
looks somewhat strange. In other words, I used cxt->tabhash as
the flat that indicates whether applying database stats is
requried. I rewote the comment there as the follows. It is
acceptable?

| * If we haven't attached the tabhash, we didn't apply database stats
| * yet. So apply it now..

+	/* create hash if not yet */
+	if (dbentry->functions == DSM_HANDLE_INVALID)
+	{
+		funchash = dshash_create(area, &dsh_funcparams, 0);
+		dbentry->functions = dshash_get_hash_table_handle(funchash);
+	}
+	else
+		funchash = dshash_attach(area, &dsh_funcparams, dbentry->functions, 0);

Why is this created on-demand?

The reason is that function stats are optional, but one dshash
takes 1MB memory at creation time.

+	/*
+	 * First, we empty the transaction stats. Just move numbers to pending
+	 * stats if any. Elsewise try to directly update the shared stats but
+	 * create a new pending entry on lock failure.
+	 */
+	if (pgStatFunctions)

I don't understand why we have both pgStatFunctions and pgStatFunctions
and pgStatPendingFunctions (and the same for other such pairs). That
seems to make no sense to me. The comments for the former literally are:
/*
* Backends store per-function info that's waiting to be sent to the collector
* in this hash table (indexed by function OID).
*/

I should have did that naively comparing to pgStatTabHash. I'll
remove pgStatPendingFunctions in the next version.

static HTAB *
pgstat_collect_oids(Oid catalogid)
@@ -1241,62 +1173,54 @@ pgstat_collect_oids(Oid catalogid)
/* ----------
* pgstat_drop_database() -
*
- *	Tell the collector that we just dropped a database.
- *	(If the message gets lost, we will still clean the dead DB eventually
- *	via future invocations of pgstat_vacuum_stat().)
+ *	Remove entry for the database that we just dropped.
+ *
+ *  If some stats update happens after this, this entry will re-created but
+ *	we will still clean the dead DB eventually via future invocations of
+ *	pgstat_vacuum_stat().
* ----------
*/
+
void
pgstat_drop_database(Oid databaseid)
{

Mixed indentation, added newline.

Fixed.

+/*
+ * snapshot_statentry() - Find an entriy from source dshash.
+ *

s/entriy/entry/

Ok, getting too tired now. Two AM in an airport lounge is not the
easiest place and time to concentrate...

Thank you very much for reviewing and sorry for the slow
response.

I don't think this is all that close to being committable :(

I'm going to work harder on this. The remaining taks just now are
the follows:

- Separte shared database stats from db_stats hash.

- Consider relaxing dbentry locking.

- Try removing pgStatPendingFunctions

- ispell on it.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2019-02-15 17:29:00 +0900, Kyotaro HORIGUCHI wrote:

At Thu, 7 Feb 2019 13:10:08 -0800, Andres Freund <andres@anarazel.de> wrote in <20190207211008.nc3axviivmcoaluq@alap3.anarazel.de>

Hi,

On 2018-11-12 20:10:42 +0900, Kyotaro HORIGUCHI wrote:

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7eed5866d2..e52ae54821 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8587,9 +8587,9 @@ LogCheckpointEnd(bool restartpoint)
&sync_secs, &sync_usecs);

/* Accumulate checkpoint timing summary data, in milliseconds. */
-	BgWriterStats.m_checkpoint_write_time +=
+	BgWriterStats.checkpoint_write_time +=
write_secs * 1000 + write_usecs / 1000;
-	BgWriterStats.m_checkpoint_sync_time +=
+	BgWriterStats.checkpoint_sync_time +=
sync_secs * 1000 + sync_usecs / 1000;

Why does this patch do renames like this in the same entry as actual
functional changes?

Just because it is no longer "messages". I'm ok to preserve them
as historcal names. Reverted.

It's fine to do such renames, just do them as separate patches. It's
hard enough to review changes this big...

/*
* Structures in which backends store per-table info that's waiting to be
@@ -189,18 +189,14 @@ typedef struct TabStatHashEntry
* Hash table for O(1) t_id -> tsa_entry lookup
*/
static HTAB *pgStatTabHash = NULL;
+static HTAB *pgStatPendingTabHash = NULL;

/*
* Backends store per-function info that's waiting to be sent to the collector
* in this hash table (indexed by function OID).
*/
static HTAB *pgStatFunctions = NULL;
-
-/*
- * Indicates if backend has some function stats that it hasn't yet
- * sent to the collector.
- */
-static bool have_function_stats = false;
+static HTAB *pgStatPendingFunctions = NULL;

So this patch leaves us with a pgStatFunctions that has a comment
explaining it's about "waiting to be sent" stats, and then additionally
a pgStatPendingFunctions?

Mmm. Thanks . I changed the comment and separated pgSTatPending*
stuff from there and merged with pgstat_pending_*. And unified
the naming.

I think my point is larger than that - I don't see why the pending
hashtables are needed at all. They seem purely superflous.

+ if (cxt->tabhash)
+ dshash_detach(cxt->tabhash);

Huh, why do we detach here?

To release the lock on cxt->dbentry. It may be destroyed.

Uh, how?

- Separte shared database stats from db_stats hash.

- Consider relaxing dbentry locking.

- Try removing pgStatPendingFunctions

- ispell on it.

Additionally:
- consider getting rid of all the pending stuff, not just for functions,
- as far as I can tell it's unnecessary

Thanks,

Andres

On 2019-Feb-15, Andres Freund wrote:

It's fine to do such renames, just do them as separate patches. It's
hard enough to review changes this big...

Talk about moving the whole file to another subdir ...

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Fri, 15 Feb 2019 17:29:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190215.172900.84235698.horiguchi.kyotaro@lab.ntt.co.jp>

I don't think this is all that close to being committable :(

I'm going to work harder on this. The remaining taks just now are
the follows:

- Separte shared database stats from db_stats hash.

Finally I didn't do that. It lead to more complexy.

- Consider relaxing dbentry locking.

Lock on a dbenty by dshash was useless to protect it from DROP
DB, so I relaxed locking on dbentry so that the dshash lock is
immediately released after fetching it. On the other hand table
and function counter hash are just destroyed at the time of a
counter reset ant it required some kind of arbitration. I could
introduce dshash_reset() but it requires many lwlocks, which
would be too-much. Instaed, I inroduced two-set of hash_handles
and reference counter in PgStat_StatDBEntry to stash out
to-be-removed-but-currently-accessed hash. pin_hashes() and
unpin_hashes(), and reset_dbentry_counters() are that.

After all, dbentries are no longer isolated by dshash partition
lock on updates, so every dbentry instead has LWLock to do
that. (tabentries/funcentries are still isolated by dshash).

pgstat_apply_tabstats() runs single-pass. Previously ran
two-passes, shared db and my database.

We could eliminate pgStatPendingTabHash, but manipulating TSA

I'm trying removing pgStatPendingTabHash it does't work yet. I'll
include it in the next version.

- Try removing pgStatPendingFunctions

Done. pgStatPendingDeadLocks and pgStatPendingTempfiles are also
removed.

- ispell on it.

I fixed many misspellings..

- Fixed several silly mistakes in the previous version.

I'll post the next version soon.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Mon, 18 Feb 2019 21:35:31 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190218.213531.89078771.horiguchi.kyotaro@lab.ntt.co.jp>

I'm trying removing pgStatPendingTabHash it does't work yet. I'll
include it in the next version.

Done. Passed a test for the case of internittent dshash lock
failure, which causes "pending" of local stats info.

- Removed pgStatPendingTabHash. "Pending" table entries are left
in pgStatTabList after pgstat_update_stat(). There's no longer
a "pending" stats store.

- Fixed several bugs of reading/writing at-rest file.

- Transactional snapshot behaved wrongly. Fixed it.

- In this project SQL helper functions are renamed from
pgstat_fetch_stat_* to backend_fetch_stat_* because the
functions with the similar cuntion are implemented for writer
side and they have the names of pgstat_fetch_stat_*.

But some of the functions had very confusing names that don't
follow the convention.

- Split pgStatLocalContext into pgSharedStatsContext and
pgStatSnapshotContext. The former is for shared statistics and
the latter is for transactional snapshot.

- Clean up pgstat.[ch] removing stale lines, fixing bogus comments.

This version is heavily improved from the previous version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 15 Feb 2019 15:53:28 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190215185328.GA29663@alvherre.pgsql>

On 2019-Feb-15, Andres Freund wrote:

It's fine to do such renames, just do them as separate patches. It's
hard enough to review changes this big...

Talk about moving the whole file to another subdir ...

Sounds reasonable. It was a searate patch at the first but
currently melded to bloat the patch. I'm going to revert the
separation/moving.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Wed, 20 Feb 2019 15:45:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190220.154517.24528798.horiguchi.kyotaro@lab.ntt.co.jp>

At Fri, 15 Feb 2019 15:53:28 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190215185328.GA29663@alvherre.pgsql>

Talk about moving the whole file to another subdir ...

Sounds reasonable. It was a searate patch at the first but
currently melded to bloat the patch. I'm going to revert the
separation/moving.

Done. This verison 16 looks as if the moving and splitting were
not happen. Major changes are:

- Restored old pgstats_* names. This largily shrinks the patch
size to less than a half lines of v15. More than that, it
gets easier to examine differences. (checkpointer.c and
bgwriter.c have a bit stale comments but it is an issue for
later.)

- Removed "oneshot" feature at all. This simplifies pgstat API
and let this patch far less simple.

- Moved StatsLock to LWTRANCHE_STATS, which is not necessary to
be in the main tranche.

- Fixed several bugs revealed by the shrinked size of the patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

On 21.02.2019 10:05, Kyotaro HORIGUCHI wrote:

Done. This verison 16 looks as if the moving and splitting were
not happen. Major changes are:

- Restored old pgstats_* names. This largily shrinks the patch
size to less than a half lines of v15. More than that, it
gets easier to examine differences. (checkpointer.c and
bgwriter.c have a bit stale comments but it is an issue for
later.)

- Removed "oneshot" feature at all. This simplifies pgstat API
and let this patch far less simple.

- Moved StatsLock to LWTRANCHE_STATS, which is not necessary to
be in the main tranche.

- Fixed several bugs revealed by the shrinked size of the patch.

I run regression tests. Unfortunately tests didn't pass, failed test is
'rangetypes':

rangetypes ... FAILED (test process exited with exit code 2)

It seems to me that an autovacuum process terminates because of segfault.

Segfault occurs within get_pgstat_tabentry_relid(). If I'm not mistaken,
somehow 'dbentry' hasn't valid pointer anymore.

'dbentry' is get in the line in do_autovacuum():

dbentry = pgstat_fetch_stat_dbentry(MyDatabaseId);

'dbentry' becomes invalid after calling pgstat_vacuum_stat().

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hello, Arthur.

At Thu, 21 Feb 2019 17:30:50 +0300, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote in <db346d14-4130-57a5-5f46-9a57e9982bec@postgrespro.ru>

Hello,

On 21.02.2019 10:05, Kyotaro HORIGUCHI wrote:

Done. This verison 16 looks as if the moving and splitting were
not happen. Major changes are:
- Restored old pgstats_* names. This largily shrinks the patch
size to less than a half lines of v15. More than that, it
gets easier to examine differences. (checkpointer.c and
bgwriter.c have a bit stale comments but it is an issue for
later.)
- Removed "oneshot" feature at all. This simplifies pgstat API
and let this patch far less simple.
- Moved StatsLock to LWTRANCHE_STATS, which is not necessary to
be in the main tranche.
- Fixed several bugs revealed by the shrinked size of the patch.

I run regression tests. Unfortunately tests didn't pass, failed test
is 'rangetypes':

rangetypes ... FAILED (test process exited with exit code 2)

It seems to me that an autovacuum process terminates because of
segfault.

Segfault occurs within get_pgstat_tabentry_relid(). If I'm not
mistaken, somehow 'dbentry' hasn't valid pointer anymore.

'dbentry' is get in the line in do_autovacuum():

dbentry = pgstat_fetch_stat_dbentry(MyDatabaseId);

'dbentry' becomes invalid after calling pgstat_vacuum_stat().

Thank you very much for the report. I haven't seen the error, but
I think you gave me enough information about the issue. I try to
reproduce it.

I found another problem commit_ts test reliably fails by dshash
corruption in startup process. I've not found why and will
investigate it, too.

regarsds.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Fri, 22 Feb 2019 17:19:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190222.171956.98584931.horiguchi.kyotaro@lab.ntt.co.jp>

It seems to me that an autovacuum process terminates because of
segfault.

Segfault occurs within get_pgstat_tabentry_relid(). If I'm not
mistaken, somehow 'dbentry' hasn't valid pointer anymore.

do_autovacuum does the followings:

dbentry = pgstat_fetch_stat_dbentry() -- create cached dbentry
StartTransactionCommand() -- starts transaction
pgstat_vacuum_stat() -- blows away the cached dbentry.
shared = pgstat_fetch_stat_dbentry()

It was harmless previously, but pgstat_* functions blow away
local cache at the first call after transaction start. As the
result dbentry becomes invalid. The reason I didin't see the same
crash is the second pgstat_fetch_stat_dbentry() accidentially
zeroes-out the once invalidated dbentry.

It is fixed by moving StartTransactionCommand to before the first
pgstat_f_s_dbentry(), which looks better not having this problem.

me> I found another problem commit_ts test reliably fails by dshash
me> corruption in startup process. I've not found why and will
me> investigate it, too.

It is rather stupid, pgstat_reset_all() releases an entry within
the sequential scan loop, which contradicts the protocol of
dshash_seq_next.

The two aboves are fixed in the attached v17.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 25.02.2019 07:52, Kyotaro HORIGUCHI wrote:

It is fixed by moving StartTransactionCommand to before the first
pgstat_f_s_dbentry(), which looks better not having this problem.

Thank you. Still there are couple TAP-test which don't pass:
002_archiving.pl and 010_pg_basebackup.pl. I think the next simple patch
solves the issue:

diff --git a/src/backend/postmaster/pgstat.c 
b/src/backend/postmaster/pgstat.c
index f9b22a4d71..d500f9d090 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3169,6 +3169,9 @@ pgstat_bestart(void)
             case StartupProcess:
                 beentry->st_backendType = B_STARTUP;
                 break;
+           case ArchiverProcess:
+               beentry->st_backendType = B_ARCHIVER;
+               break;
             case BgWriterProcess:
                 beentry->st_backendType = B_BG_WRITER;
                 break;
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl 
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 8939758c59..4f656c98a3 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -6,7 +6,7 @@ use File::Basename qw(basename dirname);
  use File::Path qw(rmtree);
  use PostgresNode;
  use TestLib;
-use Test::More tests => 106;
+use Test::More tests => 105;

program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');

010_pg_basebackup.pl has 105 tests now because pg_stat_tmp dir was
removed from the `foreach my $dirname` loop.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Sun, Feb 24, 2019 at 11:53 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

The two aboves are fixed in the attached v17.

Andres just drew my attention to patch 0004 in this series, which is
definitely not OK. That patch allows the postmaster to use dynamic
shared memory, claiming: "Shared memory baesd stats collector needs it
to work on postmaster and no problem found to do that. Just allow it."

But if you just look a little further down in the code from where that
assertion is located, you'll find this:

/* Lock the control segment so we can register the new segment. */
LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);

It is a well-established principle that the postmaster must not
acquire any locks, because if it did, a corrupted shared memory
segment could take down not only individual backends but the
postmaster as well. So this is entirely not OK in the postmaster. I
think there might be other reasons as well why this is not OK that
aren't occurring to me at the moment, but that one is enough by
itself.

But even if for some reason that were OK, I'm pretty sure that any
design that involves the postmaster interacting with the data stored
in shared memory by the stats collector is an extremely bad idea.
Again, the postmaster is supposed to have as little interaction with
shared memory as possible, precisely so that it is doesn't crash and
burn when some other process corrupts shared memory. Dynamic shared
memory is included in that. So, really, the LWLock here is just the
tip of the iceberg: the postmaster not only CAN'T safely run this
code, but it shouldn't WANT to do so.

And I'm kind of baffled that it does. I haven't looked at the other
patches, but it seems to me that, while a shared-memory stats
collector is a good idea in general to avoid the I/O and CPU costs of
with reading and writing temporary files, I don't see why the
postmaster would need to be involved in any of that. Whatever the
reason, though, I'm pretty sure that's GOT to be changed for this
patch set to have any chance of being accepted.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

From 88740269660d00d548910c2f3aa631878c7cf0d4 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 21 Feb 2019 12:42:07 +0900
Subject: [PATCH 4/6] Allow dsm to use on postmaster.

DSM is inhibited to be used on postmaster. Shared memory baesd stats
collector needs it to work on postmaster and no problem found to do
that. Just allow it.

Maybe I'm missing something, but why? postmaster doesn't actually need
to process stats messages in any way?

From 774b1495136db1ad6d174ab261487fdf6cb6a5ed Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 21 Feb 2019 12:44:56 +0900
Subject: [PATCH 5/6] Shared-memory based stats collector

Previously activity statistics is shared via files on disk. Every
backend sends the numbers to the stats collector process via a socket.
It makes snapshots as a set of files on disk with a certain interval
then every backend reads them as necessary. It worked fine for
comparatively small set of statistics but the set is under the
pressure to growing up and the file size has reached the order of
megabytes. To deal with larger statistics set, this patch let backends
directly share the statistics via shared memory.

Btw, you can make the life of a committer easier by collecting the
reviewers and co-authors of a patch yourself...

This desparately needs an introductory comment in pgstat.c or such
explaining how the new scheme works.

+LWLock StatsMainLock;
+#define StatsLock (&StatsMainLock)

Wait, what? You can't just define a lock this way. That's process local
memory, locking that doesn't do anything useful.

+/* Shared stats bootstrap information */
+typedef struct StatsShmemStruct {

Please note that in PG's coding style the { comes in the next line.

+/*
+ *  Backends store various database-wide info that's waiting to be flushed out
+ *  to shared memory in these variables.
+ */
+static int		n_deadlocks = 0;
+static size_t	n_tmpfiles = 0;
+static size_t	n_tmpfilesize = 0;
+
+/*
+ * have_recovery_conflicts represents the existence of any kind if conflict
+ */
+static bool		have_recovery_conflicts = false;
+static int		n_conflict_tablespace = 0;
+static int		n_conflict_lock = 0;
+static int		n_conflict_snapshot = 0;
+static int		n_conflict_bufferpin = 0;
+static int		n_conflict_startup_deadlock = 0;

Probably worthwhile to group those into a struct, even just to make
debugging easier.

-/* ----------
- * pgstat_init() -
- *
- *	Called from postmaster at startup. Create the resources required
- *	by the statistics collector process.  If unable to do so, do not
- *	fail --- better to let the postmaster start with stats collection
- *	disabled.
- * ----------
- */
-void
-pgstat_init(void)
+static void
+pgstat_postmaster_shutdown(int code, Datum arg)

You can't have a function like that without explaining why it's there.

+	/* trash the stats on crash */
+	if (code == 0)
+		pgstat_write_statsfiles();
}

And especially not without documenting what that code is supposed to
mean.

pgstat_report_stat(bool force)
{
-	/* we assume this inits to all zeroes: */
-	static const PgStat_TableCounts all_zeroes;
-	static TimestampTz last_report = 0;
-
+	static TimestampTz last_flush = 0;
+	static TimestampTz pending_since = 0;
TimestampTz now;
-	PgStat_MsgTabstat regular_msg;
-	PgStat_MsgTabstat shared_msg;
-	TabStatusArray *tsa;
-	int			i;
+	pgstat_flush_stat_context cxt = {0};
+	bool		have_other_stats = false;
+	bool		pending_stats = false;
+	long		elapsed;
+	long		secs;
+	int			usecs;
+
+	/* Do we have anything to flush? */
+	if (have_recovery_conflicts || n_deadlocks != 0 || n_tmpfiles != 0)
+		have_other_stats = true;

/* Don't expend a clock check if nothing to do */
if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) &&
pgStatXactCommit == 0 && pgStatXactRollback == 0 &&
-		!have_function_stats)
-		return;
+		!have_other_stats && !have_function_stats)
+		return 0;

"other" seems like a pretty mysterious category. Seems better to either
name precisely, or just use the underlying variables for checks.

+/* -------
+ * Subroutines for pgstat_flush_stat.
+ * -------
+ */
+
+/*
+ * snapshot_statentry() - Find an entry from source dshash with cache.
+ *

Is snapshot_statentry() really a subroutine for pgstat_flush_stat()?

+static void *
+snapshot_statentry(pgstat_snapshot_cxt *cxt, Oid key)
+{
+	char *lentry = NULL;
+	size_t keysize = cxt->dsh_params->key_size;
+	size_t dsh_entrysize = cxt->dsh_params->entry_size;
+	bool found;
+	bool *negative;
+
+	/* caches the result entry */

/*
-	 * Don't send a message unless it's been at least PGSTAT_STAT_INTERVAL
-	 * msec since we last sent one, or the caller wants to force stats out.
+	 * Create new hash with arbitrary initial entries since we don't know how
+	 * this hash will grow. The boolean put at the end of the entry is
+	 * negative flag.
*/

That, uh, seems pretty ugly and hard to understand.

+/*
+ * pgstat_flush_stat: Flushes table stats out to shared statistics.
+ *
+ *  If nowait is true, returns with false if required lock was not acquired

s/with false/false/

+ * immediately. In the case, infos of some tables may be left alone in TSA to

TSA? I assume TabStatusArray, but I don't think that's a common or
useful abbreviation. It'd be ok to just refer to the variable name.

+static bool
+pgstat_flush_stat(pgstat_flush_stat_context *cxt, bool nowait)
+{

+			/* try to apply the tab stats */
+			if (!pgstat_flush_tabstat(cxt, nowait, entry))
{
-				pgstat_send_tabstat(this_msg);
-				this_msg->m_nentries = 0;
+				/*
+				 * Failed. Leave it alone filling at the beginning in TSA.
+				 */
+				TabStatHashEntry *hash_entry;
+				bool found;
+
+				if (new_tsa_hash == NULL)
+					new_tsa_hash = create_tabstat_hash();
+
+				/* Create hash entry for this entry */
+				hash_entry = hash_search(new_tsa_hash, &entry->t_id,
+										 HASH_ENTER, &found);
+				Assert(!found);
+
+				/*
+				 * Move insertion pointer to the next segment. There must be
+				 * enough space segments since we are just leaving some of the
+				 * current elements.
+				 */
+				if (dest_elem >= TABSTAT_QUANTUM)
+				{
+					Assert(dest_tsa->tsa_next != NULL);
+					dest_tsa = dest_tsa->tsa_next;
+					dest_elem = 0;
+				}
+
+				/* Move the entry if needed */
+				if (tsa != dest_tsa || i != dest_elem)
+				{
+					PgStat_TableStatus *new_entry;
+					new_entry = &dest_tsa->tsa_entries[dest_elem];
+					*new_entry = *entry;
+					entry = new_entry;
+				}
+
+				hash_entry->tsa_entry = entry;
+				dest_elem++;

This seems a lot of work for just leaving an entry around to be
processed later. Shouldn't code for that already exist elsewhere?

void
pgstat_vacuum_stat(void)
{
-	HTAB	   *htab;
-	PgStat_MsgTabpurge msg;
-	PgStat_MsgFuncpurge f_msg;
-	HASH_SEQ_STATUS hstat;
+	HTAB	   *oidtab;
+	dshash_table *dshtable;
+	dshash_seq_status dshstat;
PgStat_StatDBEntry *dbentry;
PgStat_StatTabEntry *tabentry;
PgStat_StatFuncEntry *funcentry;
-	int			len;

-	if (pgStatSock == PGINVALID_SOCKET)
+	/* we don't collect statistics under standalone mode */
+	if (!IsUnderPostmaster)
return;

-	/*
-	 * If not done for this transaction, read the statistics collector stats
-	 * file into some hash tables.
-	 */
-	backend_read_statsfile();
+	/* If not done for this transaction, take a snapshot of stats */
+	pgstat_snapshot_global_stats();

Hm, why do we need a snapshot here?

/*
* Now repeat the above steps for functions. However, we needn't bother
* in the common case where no function stats are being collected.
*/

Can't we move the act of iterating through these hashes and probing
against another hash into a helper function and reuse? These
duplications aren't pretty.

Greetings,

Andres Freund

Ping? Unless there's a new version pretty soon, we're going to have to
move this to the next CF, I think.

On 3/22/19 10:33 PM, Andres Freund wrote:

Ping? Unless there's a new version pretty soon, we're going to have to
move this to the next CF, I think.

Agreed. I've marked it Waiting on Author for now and will move it to
PG13 on the 28th if no new patch appears.

Regards,
--
-David
david@pgmasters.net

Arthur, Andres and Robert, thank you all for the comment, and
sorry for being late.

At Wed, 6 Mar 2019 14:43:32 -0800, Andres Freund <andres@anarazel.de> wrote in <20190306224332.374sdf6hsjh27m7t@alap3.anarazel.de>

DSM is inhibited to be used on postmaster. Shared memory baesd stats
collector needs it to work on postmaster and no problem found to do
that. Just allow it.

Maybe I'm missing something, but why? postmaster doesn't actually need
to process stats messages in any way?

Agreed. I should have do the initial work in the startup process.
Thank you Robert for the detailed explanation.

Done in the attached patch. Each backend craete/destroys shared
memory and loads/writers stats file. Startup process firstly
creates shared stats and destroys at its end. The first backend
after that again loads the file and craetes shared memory. It is
saved and destroys at server stop. This is because I didn't pin
the dsm underlying to dsa, which could be a cause of troubles.

From 774b1495136db1ad6d174ab261487fdf6cb6a5ed Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 21 Feb 2019 12:44:56 +0900
Subject: [PATCH 5/6] Shared-memory based stats collector

Previously activity statistics is shared via files on disk. Every
backend sends the numbers to the stats collector process via a socket.
It makes snapshots as a set of files on disk with a certain interval
then every backend reads them as necessary. It worked fine for
comparatively small set of statistics but the set is under the
pressure to growing up and the file size has reached the order of
megabytes. To deal with larger statistics set, this patch let backends
directly share the statistics via shared memory.

Btw, you can make the life of a committer easier by collecting the
reviewers and co-authors of a patch yourself...

This desparately needs an introductory comment in pgstat.c or such
explaining how the new scheme works.

Mmm. I'm not sure what you want exactly but this behaves a bit
complex way than the previous version, I added a description that
explains that in the comment at the beginning of pgstat.c. I'm
very happy if you give me a little more information about what
you want.

+LWLock StatsMainLock;
+#define StatsLock (&StatsMainLock)

Wait, what? You can't just define a lock this way. That's process local
memory, locking that doesn't do anything useful.

Oops.. I don't understand why I did that. Moved to StatsShmemStruct.

+/* Shared stats bootstrap information */
+typedef struct StatsShmemStruct {

Please note that in PG's coding style the { comes in the next line.

Fixed.

+/*
+ *  Backends store various database-wide info that's waiting to be flushed out
+ *  to shared memory in these variables.
+ */
+static int		n_deadlocks = 0;

...

+static int		n_conflict_bufferpin = 0;
+static int		n_conflict_startup_deadlock = 0;

Probably worthwhile to group those into a struct, even just to make
debugging easier.

Done. Moved to BackendDBStats. But didn't for pgStatXact* and
pgStatBlock* to make the patch smaller.

-void
-pgstat_init(void)
+static void
+pgstat_postmaster_shutdown(int code, Datum arg)

You can't have a function like that without explaining why it's there.

+	/* trash the stats on crash */
+	if (code == 0)
+		pgstat_write_statsfiles();
}

And especially not without documenting what that code is supposed to
mean.

Added comments to all functions.

pgstat_report_stat(bool force)
{

...

+ bool have_other_stats = false;

..

+	/* Do we have anything to flush? */
+	if (have_recovery_conflicts || n_deadlocks != 0 || n_tmpfiles != 0)
+		have_other_stats = true;

..

"other" seems like a pretty mysterious category. Seems better to either
name precisely, or just use the underlying variables for checks.

Agreed. It is mainly database-wide stats so I named it
dbstats. To move the expression near to the definitions of the
used variables I defined two macros, HAVE_PENDING_DBSTATS() and
HAVE_PENDING_CONFLICTS().

+/* -------
+ * Subroutines for pgstat_flush_stat.
+ * -------
+ */
+
+/*
+ * snapshot_statentry() - Find an entry from source dshash with cache.
+ *

Is snapshot_statentry() really a subroutine for pgstat_flush_stat()?

No. Sorry, it is just misplaced. Fixed.

+static void *
+snapshot_statentry(pgstat_snapshot_cxt *cxt, Oid key)

..

+	 * Create new hash with arbitrary initial entries since we don't know how
+	 * this hash will grow. The boolean put at the end of the entry is
+	 * negative flag.
*/

That, uh, seems pretty ugly and hard to understand.

Ok. I introduced common struct PgStat_snapshot_head to hold the
negative flag.

+/*
+ * pgstat_flush_stat: Flushes table stats out to shared statistics.
+ *
+ *  If nowait is true, returns with false if required lock was not acquired

s/with false/false/

Fixed. Thanks.

+ * immediately. In the case, infos of some tables may be left alone in TSA to

TSA? I assume TabStatusArray, but I don't think that's a common or
useful abbreviation. It'd be ok to just refer to the variable name.

Yeah I think so and I though that it's not my original but
actually is.. Replaced them with a complete word.

+static bool
+pgstat_flush_stat(pgstat_flush_stat_context *cxt, bool nowait)
+{

+			/* try to apply the tab stats */
+			if (!pgstat_flush_tabstat(cxt, nowait, entry))
{
-				pgstat_send_tabstat(this_msg);
-				this_msg->m_nentries = 0;
+				/*
+				 * Failed. Leave it alone filling at the beginning in TSA.
+				 */

...

This seems a lot of work for just leaving an entry around to be
processed later. Shouldn't code for that already exist elsewhere?

No. TabStatusArray was alweays emptied after sending stats data
to collector. So the packing (entry moving) code is a brand new
stuff.

void
pgstat_vacuum_stat(void)

...

-	backend_read_statsfile();
+	/* If not done for this transaction, take a snapshot of stats */
+	pgstat_snapshot_global_stats();

Hm, why do we need a snapshot here?

Thank you for pointing out. It used to be needed during
development but no longer needed. Removed.

/*
* Now repeat the above steps for functions. However, we needn't bother
* in the common case where no function stats are being collected.
*/

Can't we move the act of iterating through these hashes and probing
against another hash into a helper function and reuse? These
duplications aren't pretty.

Like snapshot_statentry? The first member of both
PgStat_StatTab/FuncEntry is Oid. So just accessing the hash entry
as Oid makes it possible. There's no point in providing common
header struct as I did for backend snapshot.

pgstat_remove_useless_entries() is that.

At Mon, 25 Feb 2019 14:27:13 +0300, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote in <fb4ca586-c3f0-7c2f-ca2e-2aa49b66d146@postgrespro.ru>

Thank you. Still there are couple TAP-test which don't pass:
002_archiving.pl and 010_pg_basebackup.pl. I think the next simple
patch solves the issue:

I accidentially removed it in 0004. Fixed it. Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

Unfortunately I don't think it's realistic to target this to v12. I
think it was unlikely to make at the beginning of the CF, but since then
development just wasn't quick enough to warrant aiming for it. It's a
large and somewhat complex patch, and has some significant risks
associated. Therefore I think we should mark this as targeting v13, and
move to the next CF?

Greetings,

Andres Freund

Hello.

At Wed, 3 Apr 2019 12:56:59 -0700, Andres Freund <andres@anarazel.de> wrote in <20190403195659.fcmk2i7ruxhtyqjl@alap3.anarazel.de>

Unfortunately I don't think it's realistic to target this to v12. I
think it was unlikely to make at the beginning of the CF, but since then
development just wasn't quick enough to warrant aiming for it. It's a
large and somewhat complex patch, and has some significant risks
associated. Therefore I think we should mark this as targeting v13, and
move to the next CF?

I'd like to get this in 12 but actually the time is coming. If
everyone think that that is not realistic, I do that.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Apr 04, 2019 at 09:25:12AM +0900, Kyotaro HORIGUCHI wrote:

Hello.

At Wed, 3 Apr 2019 12:56:59 -0700, Andres Freund <andres@anarazel.de> wrote in <20190403195659.fcmk2i7ruxhtyqjl@alap3.anarazel.de>

Unfortunately I don't think it's realistic to target this to v12. I
think it was unlikely to make at the beginning of the CF, but since then
development just wasn't quick enough to warrant aiming for it. It's a
large and somewhat complex patch, and has some significant risks
associated. Therefore I think we should mark this as targeting v13, and
move to the next CF?

I'd like to get this in 12 but actually the time is coming. If
everyone think that that is not realistic, I do that.

Unfortunately, now that we're past code freeze it's clear this is a PG12
matter now :-(

I personally consider this to be very worthwhile & beneficial improvement,
but I agree with Andres the patch did not quite get to committable state
in the last CF. Conidering how sensitive part it touches, I suggest we try
to get it committed early in the PG13 cycle. I'm willing to spend some
time on doing test/benchmarks and reviewing the code, if needed.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Tue, 9 Apr 2019 17:03:33 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <20190409150333.5iashyjxm5jmraml@development>

Unfortunately, now that we're past code freeze it's clear this is a
PG12
matter now :-(

I personally consider this to be very worthwhile & beneficial
improvement,
but I agree with Andres the patch did not quite get to committable
state
in the last CF. Conidering how sensitive part it touches, I suggest we
try
to get it committed early in the PG13 cycle. I'm willing to spend some
time on doing test/benchmarks and reviewing the code, if needed.

I'm very happy to be told that. Actually the code was a rush work
(mainly for reverting refactoring) and left some stupid
mistakes. I'm going through on the patch again and polish code.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Apr 10, 2019 at 09:39:29AM +0900, Kyotaro HORIGUCHI wrote:

At Tue, 9 Apr 2019 17:03:33 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <20190409150333.5iashyjxm5jmraml@development>

Unfortunately, now that we're past code freeze it's clear this is a
PG12
matter now :-(

I personally consider this to be very worthwhile & beneficial
improvement,
but I agree with Andres the patch did not quite get to committable
state
in the last CF. Conidering how sensitive part it touches, I suggest we
try
to get it committed early in the PG13 cycle. I'm willing to spend some
time on doing test/benchmarks and reviewing the code, if needed.

I'm very happy to be told that. Actually the code was a rush work
(mainly for reverting refactoring) and left some stupid
mistakes. I'm going through on the patch again and polish code.

While reviewing the patch I've always had issue with evaluating how it
behaves for various scenarios / workloads. The reviews generally did one
specific benchmark, but I find that unsatisfactory. I wonder whether if
we could develop a small set of more comprehensive workloads for this
patch (i.e. different numbers of objects, access patterns, ...).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello.

At Wed, 10 Apr 2019 11:13:27 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <20190410091327.fpnvjbuu74dzxizl@development>

While reviewing the patch I've always had issue with evaluating how it
behaves for various scenarios / workloads. The reviews generally did
one
specific benchmark, but I find that unsatisfactory. I wonder whether
if
we could develop a small set of more comprehensive workloads for this
patch (i.e. different numbers of objects, access patterns, ...).

Indeed. I'm having difficulty also with catcache pruning
stuff. But I might have found a clue to that.

I took performance numbers after some amendment and polishment of
the patch.

I expected operf might work but it doesn't show meaningful
information with O2'ed binary. gprof slows binary to about one
third. But just running pgbench gave me rather stable numbers
(differently from catcache stuff..).

The numbers are tps for 300 minutes run and ratio between master
and patched.

[A-D]1 are just running stats-updator clients.

master-O2 patched patched/master-O2
A1: 13431.603208 13457.968950 100.1963
B1: 72284.737474 72535.169437 100.3465
C1: 19.963315 20.037411 100.3712
D1: 193.027074 196.651603 101.8777

[A-D]2 tests introduces stats-reader client.

master-O2 patched patched/master-O2
updator / reader updator / reader updator / reader
A2: 12929.418503/512.784200 13066.150297/584.686889 101.0575 / 114.0220
B2: 71673.804812/ 20.102687 71916.823242/ 22.109251 100.3391 / 109.9816
C2: 16.066719/485.788495 16.487942/577.930340 102.6217 / 118.9675
D2: 189.563306/ 36.252532 193.817075/ 44.661707 102.2440 / 123.1961

Case A1 is most simplest case: 1 client repeatedly updated stats
of pgbench_acconts (of scale-1, but that doesn't matter)

Case B1 is A1 from 100 concurrent clients.

Case C1 is massive(?) number of stats update: Concretely select
sum() on a partitioned table with 1000 children, from 1 client.

Case D1 doesn C1 from 97 concurrent clients.

A2-D2 are running a single stats referencing client while A1-D1
are running respectively. (select sum(seq_scan) from pg_stat_user_tables)

Perhaps the number will get worse having many rerefencing clients
but I think it's not realistic.

I'll run test with many databases (-100?) and expanded tabstat
entry cases.

The attached files are:

v19-0001-sequential-scan-for-dshash.patch:
v19-0002-Add-conditional-lock-feature-to-dshash.patch:
v19-0003-Make-archiver-process-an-auxiliary-process.patch:
v19-0005-Remove-the-GUC-stats_temp_directory.patch:

not changed since v18 except rebasing.

v19-0004-Shared-memory-based-stats-collector.patch:

Rebased. Fixed several bugs. Improved performance in some
cases. Made structs/code tidier. Added/rewrote comments.

run.sh : main test script
gencr.pl : partitioned table generator script generator
(perl gencr.pl | psql postgres to run)
tr.sql : stats-updator client script used by run.sh
ref.sql : stats-reader client script used by run.sh

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi.

At Fri, 17 May 2019 14:27:22 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190517.142722.139901807.horiguchi.kyotaro@lab.ntt.co.jp>

The attached files are:

It's broken perhaps by recent core change?

I'll fix it.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

me> It's broken perhaps by recent core change?
me>
me> I'll fix it.

Not by core change, but my silly mistake in memory context usage.

Fixed. Version 20.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, May 17, 2019 at 6:48 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Fixed. Version 20.

Hello Horiguchi-san,

A new Commitfest is here. This doesn't apply (maybe just because of
the new improved pgindent). Could we please have a fresh rebase?

Thanks,

--
Thomas Munro
https://enterprisedb.com

Hello.

At Mon, 1 Jul 2019 23:19:31 +1200, Thomas Munro <thomas.munro@gmail.com> wrote in <CA+hUKGK5WNCEe9g4ie=-6Oym-WNqYBXX9A1qPgKv89KGkzW72g@mail.gmail.com>

Fixed. Version 20.

Hello Horiguchi-san,

A new Commitfest is here. This doesn't apply (maybe just because of
the new improved pgindent). Could we please have a fresh rebase?

Thank you for noticing, Thomas. I rebased and made some possible
improvement. More than that, I wrote a new test script.

- Rebased.

- Reworded almost all of comments. Many of them was found
broken. Added some comments.

- Shortened some LWLocked code paths.

- Got rid of useless palloc for snapshot area of globalStats.

The attached files are:

gendb.pl:
script to generate databases.

statbehch.pl:
benchmarking script.

0001-sequential-scan-for-dshash.patch:
Adds sequential scan feature to dshash

0002-Add-conditional-lock-feature-to-dshash.patch:
Adds conditional lock feature to dshash

0003-Make-archiver-process-an-auxiliary-process.patch:
Change archiver process to auxiliary process. This is needed to
let archive access shared memory.

0004-Shared-memory-based-stats-collector.patch:
The body of this patchset. Changes pgstat from a process
connected by socket to shared memory shared among backends.

0005-Remove-the-GUC-stats_temp_directory.patch:
Remove GUC stats_temp_directory. Separated from 0004 to make it
smaller.

====
Tomas said upthread:

While reviewing the patch I've always had issue with evaluating how it
behaves for various scenarios / workloads. The reviews generally did
one
specific benchmark, but I find that unsatisfactory. I wonder whether
if
we could develop a small set of more comprehensive workloads for this
patch (i.e. different numbers of objects, access patterns, ...).

The structure of shared stats area follows:

  dshash for databsae stats
   + dshash entry for db1
      + dshash for table stats
          + dshash entry for tbl1
          + dshash entry for tbl2
          + dshash entry for tbl3
           ...
   + db2
     ...
   + db3
   ...

Dshash restiricts an entry to be accessed only by a single
process. This is quite inconvenient since that a database hash
entry becomes a bottle neck. On the other hand dbstat dshash
entry used on a backend is not removed since it is removed after
all accessor to the databse have gone. So this patch immediately
releases dbstats dshash entry so that it doesn't become a
bottleneck.

Another bottle neck would be lock conflicts on a
database/table/function stats entry. This is avoided by, like
existing stats collector, enforces intervals not shorter than 500
ms (PGSTAT_STAT_MIN_INTERVAL) between two successive updates on
one process and skipping the update if lock is going to conflict.

Yet another bottle neck was conflict between reset and
udpate. Since all processes are working on the same data on
shared memory, counters cannot be reset until all referer are
gone. So I let dbentry have two sets of table/function stats
dshash in order to separate accessors come after reset and
existing accessors. A process "pins" the current table/function
dshashes before accessing them (pin_hashes()/unpin_hashes()). All
updates in the round are performed on the "pinned" generation of
dshashes. If two or more successive reset requests come in a
very short time, the requests other than the first one are just
ignored (reset_dbentry_counters()). So client can see some
non-zero numbers just after reset if many processes reset stats
at the same time but I don't think that is worth amending.

After all, almost all potential bottle necks are eliminated in
this patch. If so many n clients are running, the mean interval
of updates would be 500/n ms so 1000 or more clients can hit by
the bottle neck but I think also the current stats collecter
suffers from such many clients. (No matter what would happen with
such massive number of processes, I don't have an environment
that can let such many clients/backends live on...)

That being said, this patch is doomed to update stats in
reasonable period, 1000ms in this patch
(PGSTAST_STAT_MAX_INTERVAL). If that duration elapsed, this patch
waits all required locks to be acquired. So, the possible bottle
neck is still on database and table/function shard hash
entries. It is efficiently causes the conflicts that many
processes on the same database update the same table.

I remade benchmark script so that many parameters can be changed
easily. I took numbers for the following access patterns. Every
number is the mean of 6 runs. I choosed the configurations so
that no disk access happenes while running benchmark.

#db : number of accessing database
#tbl : number of tables per database
#client : number of stats-updator clients
#iter : number of query iterations
#xactlen : number of queries in a transaction
#referers: number of stats-referenceing clients

#db #tbl #clients #iter #xactlen #referers
A1: 1 1 1 20000 10 0
A2: 1 1 1 20000 10 1
B1: 1 1 90 2000 10 0
B2: 1 1 90 2000 10 1
C1: 1 50 90 2000 10 0
C2: 1 50 90 2000 10 1
D1: 50 1 90 2000 10 0
D2: 50 1 90 2000 10 1
E1: 50 1 90 2000 10 10
F1: 50 1 10 2000 10 90

master patched
updator referrer updator referrer
time / stdev count / stdev time / stdev count / stdev
A1: 1769.13 / 71.87 1729.97 / 61.58
A2: 1903.94 / 75.65 2906.67 / 78.28 1849.41 / 43.00 2855.33 / 62.95
B1: 2967.84 / 9.88 2984.20 / 6.10
B2: 3005.38 / 5.32 253.00 / 33.09 3007.26 / 5.70 253.33 / 60.63
C1: 3066.14 / 13.80 3069.34 / 11.65
C2: 3353.66 / 8.14 282.92 / 20.65 3341.36 / 12.44 251.65 / 21.13
D1: 2977.12 / 5.12 2991.60 / 6.68
D2: 3005.80 / 6.44 252.50 / 38.34 3010.58 / 7.34 282.33 / 57.07
E1: 3255.47 / 8.91 244.02 / 17.03 3293.88 / 18.05 249.13 / 14.58
F1: 2620.85 / 9.17 202.46 / 3.35 2668.60 / 41.04 208.19 / 6.79

ratio (100: same, smaller value means patched version is faster)

updator referrer
patched/master(%) master/patched (%)
A1: 97.79 -
A2: 97.14 101.80
B1: 100.55
B2: 100.06 99.87
C1: 100.10
C2: 99.63 112.43
D1: 100.49
D2: 100.16 89.43
E1: 101.18 97.95
F1: 101.82 97.25

Mmm... I don't see distinctive tendency.. Referrer side shows
larger fluctuation but I'm not sure that suggests something
meaningful.

I'll rerun the bencmarks with loger period (many itererations).

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 04 Jul 2019 19:27:54 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in <20190704.192754.27063464.horikyota.ntt@gmail.com>

#db #tbl #clients #iter #xactlen #referers
A1: 1 1 1 20000 10 0
A2: 1 1 1 20000 10 1
B1: 1 1 90 2000 10 0
B2: 1 1 90 2000 10 1
C1: 1 50 90 2000 10 0
C2: 1 50 90 2000 10 1
D1: 50 1 90 2000 10 0
D2: 50 1 90 2000 10 1
E1: 50 1 90 2000 10 10
F1: 50 1 10 2000 10 90

master patched
updator referrer updator referrer
time / stdev count / stdev time / stdev count / stdev
A1: 1769.13 / 71.87 1729.97 / 61.58
A2: 1903.94 / 75.65 2906.67 / 78.28 1849.41 / 43.00 2855.33 / 62.95
B1: 2967.84 / 9.88 2984.20 / 6.10
B2: 3005.38 / 5.32 253.00 / 33.09 3007.26 / 5.70 253.33 / 60.63
C1: 3066.14 / 13.80 3069.34 / 11.65
C2: 3353.66 / 8.14 282.92 / 20.65 3341.36 / 12.44 251.65 / 21.13
D1: 2977.12 / 5.12 2991.60 / 6.68
D2: 3005.80 / 6.44 252.50 / 38.34 3010.58 / 7.34 282.33 / 57.07
E1: 3255.47 / 8.91 244.02 / 17.03 3293.88 / 18.05 249.13 / 14.58
F1: 2620.85 / 9.17 202.46 / 3.35 2668.60 / 41.04 208.19 / 6.79

ratio (100: same, smaller value means patched version is faster)

updator referrer
patched/master(%) master/patched (%)
A1: 97.79 -
A2: 97.14 101.80
B1: 100.55
B2: 100.06 99.87
C1: 100.10
C2: 99.63 112.43
D1: 100.49
D2: 100.16 89.43
E1: 101.18 97.95
F1: 101.82 97.25

Mmm... I don't see distinctive tendency.. Referrer side shows
larger fluctuation but I'm not sure that suggests something
meaningful.

I'll rerun the bencmarks with loger period (many itererations).

I put more puressure to the system.

G1: 1 db, 400 clients, 1000 tables, 20000 loops/client, 1000 query/tr, 0 reader
G2: 1 db, 400 clients, 1000 tables, 20000 loops/client, 1000 query/tr, 1 reader

Result:
master patched
updator referrer updator referrer
time / stdev count / stdev time / stdev count / stdev
G1: 125946.22 / 796.83 125227.24 / 89.82
G2: 126463.47 / 81.87 1985.70 / 33.96 125427.95 / 82.35 1985.60 / 55.24

Ratio: (100: same, smaller value means patched version is faster)

updator referrer
patched/master(%) master/patched (%)
G1: 99.40 -
G2: 99.18 100.0

Slightly faster, or maybe significantly enough considering the
stdev. More crucial difference is shown outside the
numbers. Non-patched version complained that (incorrectly) "stats
collector not respond, used stale stats" many times, which is not
seen on patched version. That means that the reader reads older
numbers as far as 1 second ago. (It might be good that writer
should complain about update holdoff for more than, say 0.75s.)

CF-bot warned that it doesn't work on Windows. I'm experiencing
an very painful time to wait for tortoise git is walking slowly
as its name suggests. It would be fixed in the next version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. This is v21 of the patch.

CF-bot warned that it doesn't work on Windows. I'm experiencing
an very painful time to wait for tortoise git is walking slowly
as its name suggests. It would be fixed in the next version.

Found a bug in initialization. StatsShememInit() was placed at a
wrong place and stats code on child processes accessed
uninitialized pointer. It is a leftover from the previous shape
where dsm was activated on postmaster.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2019-Jul-11, Kyotaro Horiguchi wrote:

Hello. This is v21 of the patch.

CF-bot warned that it doesn't work on Windows. I'm experiencing
an very painful time to wait for tortoise git is walking slowly
as its name suggests. It would be fixed in the next version.

Found a bug in initialization. StatsShememInit() was placed at a
wrong place and stats code on child processes accessed
uninitialized pointer. It is a leftover from the previous shape
where dsm was activated on postmaster.

This doesn't apply anymore. Can you please rebase?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Tue, 3 Sep 2019 18:28:05 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190903222805.GA13932@alvherre.pgsql>

Found a bug in initialization. StatsShememInit() was placed at a
wrong place and stats code on child processes accessed
uninitialized pointer. It is a leftover from the previous shape
where dsm was activated on postmaster.

This doesn't apply anymore. Can you please rebase?

Thanks! I forgot to post rebased version after doing. Here it is.

- (Re)Rebased to the current master.
- Passed all tests for me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2019-Sep-10, Kyotaro Horiguchi wrote:

At Tue, 3 Sep 2019 18:28:05 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190903222805.GA13932@alvherre.pgsql>

Found a bug in initialization. StatsShememInit() was placed at a
wrong place and stats code on child processes accessed
uninitialized pointer. It is a leftover from the previous shape
where dsm was activated on postmaster.

This doesn't apply anymore. Can you please rebase?

Thanks! I forgot to post rebased version after doing. Here it is.

- (Re)Rebased to the current master.
- Passed all tests for me.

This seems to have very trivial conflicts -- please rebase again?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Wed, 25 Sep 2019 18:01:02 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190925210102.GA26396@alvherre.pgsql>

On 2019-Sep-10, Kyotaro Horiguchi wrote:

At Tue, 3 Sep 2019 18:28:05 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20190903222805.GA13932@alvherre.pgsql>

Found a bug in initialization. StatsShememInit() was placed at a
wrong place and stats code on child processes accessed
uninitialized pointer. It is a leftover from the previous shape
where dsm was activated on postmaster.

This doesn't apply anymore. Can you please rebase?

Thanks! I forgot to post rebased version after doing. Here it is.

- (Re)Rebased to the current master.
- Passed all tests for me.

This seems to have very trivial conflicts -- please rebase again?

Affected by the code movement in 9a86f03b4e. Just
rebased. Thanks.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Sep 27, 2019 at 09:46:47AM +0900, Kyotaro Horiguchi wrote:

Affected by the code movement in 9a86f03b4e. Just
rebased. Thanks.

This does not apply anymore. Could you provide a rebase? I have
moved the patch to next CF, waiting on author.

Thanks,
--
Michael

At Sun, 1 Dec 2019 11:12:32 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Fri, Sep 27, 2019 at 09:46:47AM +0900, Kyotaro Horiguchi wrote:

Affected by the code movement in 9a86f03b4e. Just
rebased. Thanks.

This does not apply anymore. Could you provide a rebase? I have
moved the patch to next CF, waiting on author.

Thanks! Rebased.

# I should design then run a performance test on this..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 03 Dec 2019 17:27:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

Thanks! Rebased.

CFbots seem unhappy with this. Rebased.

# I should design then run a performance test on this..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

CFbots seem unhappy with this. Rebased.

It's failing to apply again, so I rebased again. I haven't
read the code or done any testing beyond check-world.

regards, tom lane

Tom Lane escribió:

In patch 0003,

/*
-		 * Was it the archiver?  If so, just try to start a new one; no need
-		 * to force reset of the rest of the system.  (If fail, we'll try
-		 * again in future cycles of the main loop.).  Unless we were waiting
-		 * for it to shut down; don't restart it in that case, and
-		 * PostmasterStateMachine() will advance to the next shutdown step.
+		 * Was it the archiver?  Normal exit can be ignored; we'll start a new
+		 * one at the next iteration of the postmaster's main loop, if
+		 * necessary. Any other exit condition is treated as a crash.
*/
if (pid == PgArchPID)
{
PgArchPID = 0;
if (!EXIT_STATUS_0(exitstatus))
-				LogChildExit(LOG, _("archiver process"),
-							 pid, exitstatus);
-			if (PgArchStartupAllowed())
-				PgArchPID = pgarch_start();
+				HandleChildCrash(pid, exitstatus,
+								 _("archiver process"));
continue;
}

I'm worried that we're causing all processes to terminate when an
archiver dies in some ugly way; but in the current coding, it's pretty
harmless and we'd just start a new one. I think this needs to be
reconsidered. As far as I know, pgarchiver remains unconnected to
shared memory so a crash-restart cycle is not necessary. We should
continue to just log the error message and move on.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2020-03-09 15:37:05 -0300, Alvaro Herrera wrote:

Tom Lane escribió:

In patch 0003,

/*
-		 * Was it the archiver?  If so, just try to start a new one; no need
-		 * to force reset of the rest of the system.  (If fail, we'll try
-		 * again in future cycles of the main loop.).  Unless we were waiting
-		 * for it to shut down; don't restart it in that case, and
-		 * PostmasterStateMachine() will advance to the next shutdown step.
+		 * Was it the archiver?  Normal exit can be ignored; we'll start a new
+		 * one at the next iteration of the postmaster's main loop, if
+		 * necessary. Any other exit condition is treated as a crash.
*/
if (pid == PgArchPID)
{
PgArchPID = 0;
if (!EXIT_STATUS_0(exitstatus))
-				LogChildExit(LOG, _("archiver process"),
-							 pid, exitstatus);
-			if (PgArchStartupAllowed())
-				PgArchPID = pgarch_start();
+				HandleChildCrash(pid, exitstatus,
+								 _("archiver process"));
continue;
}

I'm worried that we're causing all processes to terminate when an
archiver dies in some ugly way; but in the current coding, it's pretty
harmless and we'd just start a new one. I think this needs to be
reconsidered. As far as I know, pgarchiver remains unconnected to
shared memory so a crash-restart cycle is not necessary. We should
continue to just log the error message and move on.

Why is it worth having the archiver be "robust" that way? Except that
random implementation details led to it not being connected to shared
memory, and thus allowing a restart for any exit code, I don't see a
need? It doesn't have exit paths that could validly trigger another exit
code, as far as I can see.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2020-03-09 15:37:05 -0300, Alvaro Herrera wrote:

I'm worried that we're causing all processes to terminate when an
archiver dies in some ugly way; but in the current coding, it's pretty
harmless and we'd just start a new one. I think this needs to be
reconsidered. As far as I know, pgarchiver remains unconnected to
shared memory so a crash-restart cycle is not necessary. We should
continue to just log the error message and move on.

Why is it worth having the archiver be "robust" that way?

I'd ask a different question: what the heck is this patchset doing
touching the archiver in the first place? I can see no plausible
reason for that doing anything related to stats collection. If we
now need some new background processing for stats, let's make a
new postmaster child process to do that, not overload the archiver
with unrelated responsibilities.

regards, tom lane

Hi,

On 2020-03-09 15:04:23 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2020-03-09 15:37:05 -0300, Alvaro Herrera wrote:

I'm worried that we're causing all processes to terminate when an
archiver dies in some ugly way; but in the current coding, it's pretty
harmless and we'd just start a new one. I think this needs to be
reconsidered. As far as I know, pgarchiver remains unconnected to
shared memory so a crash-restart cycle is not necessary. We should
continue to just log the error message and move on.

Why is it worth having the archiver be "robust" that way?

I'd ask a different question: what the heck is this patchset doing
touching the archiver in the first place? I can see no plausible
reason for that doing anything related to stats collection.

As of a release or two back, it sends stats messages for archiving
events:

if (pgarch_archiveXlog(xlog))
{
/* successful */
pgarch_archiveDone(xlog);

/*
* Tell the collector about the WAL file that we successfully
* archived
*/
pgstat_send_archiver(xlog, false);

break; /* out of inner retry loop */
}
else
{
/*
* Tell the collector about the WAL file that we failed to
* archive
*/
pgstat_send_archiver(xlog, true);

If we now need some new background processing for stats, let's make a
new postmaster child process to do that, not overload the archiver
with unrelated responsibilities.

I don't think that's what's going on. It's just changing the archiver so
it can report stats via shared memory - which subsequently means that it
needs to deal differently with errors than when it wasn't connected.

Greetings,

Andres Freund

Thank you all for the suggestions.

At Mon, 9 Mar 2020 12:25:39 -0700, Andres Freund <andres@anarazel.de> wrote in

Hi,

On 2020-03-09 15:04:23 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2020-03-09 15:37:05 -0300, Alvaro Herrera wrote:

I'm worried that we're causing all processes to terminate when an
archiver dies in some ugly way; but in the current coding, it's pretty
harmless and we'd just start a new one. I think this needs to be
reconsidered. As far as I know, pgarchiver remains unconnected to
shared memory so a crash-restart cycle is not necessary. We should
continue to just log the error message and move on.

Why is it worth having the archiver be "robust" that way?

I'd ask a different question: what the heck is this patchset doing
touching the archiver in the first place? I can see no plausible
reason for that doing anything related to stats collection.

As of a release or two back, it sends stats messages for archiving
events:

if (pgarch_archiveXlog(xlog))
{
/* successful */
pgarch_archiveDone(xlog);

/*
* Tell the collector about the WAL file that we successfully
* archived
*/
pgstat_send_archiver(xlog, false);

break; /* out of inner retry loop */
}
else
{
/*
* Tell the collector about the WAL file that we failed to
* archive
*/
pgstat_send_archiver(xlog, true);

If we now need some new background processing for stats, let's make a
new postmaster child process to do that, not overload the archiver
with unrelated responsibilities.

I don't think that's what's going on. It's just changing the archiver so
it can report stats via shared memory - which subsequently means that it
it can report stats via shared memory - which subsequently means that it
needs to deal differently with errors than when it wasn't connected.

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation. We may
need the second static shared memory segment apart from the current
one.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 10 Mar 2020 12:27:25 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation. We may
need the second static shared memory segment apart from the current
one.

Anyway, I changed the target version of the entry to 14.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

We may need the second static shared memory segment apart from the
current one.

That seems absurd to me. Solving a non-problem by introducing complex
new infrastructure.

At Mon, 9 Mar 2020 20:34:20 -0700, Andres Freund <andres@anarazel.de> wrote in

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

That theoretically raises the chance of server-crash by a small amount
of probability. But, yes, it's absurd to prmise that archiver process
crashes.

We may need the second static shared memory segment apart from the
current one.

That seems absurd to me. Solving a non-problem by introducing complex
new infrastructure.

Ok. I think I must be worrying too much.

Thanks for the suggestion.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020-Mar-10, Kyotaro Horiguchi wrote:

At Mon, 9 Mar 2020 20:34:20 -0700, Andres Freund <andres@anarazel.de> wrote in

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

That theoretically raises the chance of server-crash by a small amount
of probability. But, yes, it's absurd to prmise that archiver process
crashes.

The case I'm worried about is a misconfigured archive_command that
causes the archiver to misbehave (exit with a code other than 0); if
that already doesn't happen, or we can make it not happen, then I'm okay
with the changes to archiver.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 10, 2020 at 1:48 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Mar-10, Kyotaro Horiguchi wrote:

At Mon, 9 Mar 2020 20:34:20 -0700, Andres Freund <andres@anarazel.de> wrote in

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

That theoretically raises the chance of server-crash by a small amount
of probability. But, yes, it's absurd to prmise that archiver process
crashes.

The case I'm worried about is a misconfigured archive_command that
causes the archiver to misbehave (exit with a code other than 0); if
that already doesn't happen, or we can make it not happen, then I'm okay
with the changes to archiver.

Any script that gets killed, or that exit with a return code > 127
would do that.

On 2020-03-10 09:48:07 -0300, Alvaro Herrera wrote:

On 2020-Mar-10, Kyotaro Horiguchi wrote:

At Mon, 9 Mar 2020 20:34:20 -0700, Andres Freund <andres@anarazel.de> wrote in

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

That theoretically raises the chance of server-crash by a small amount
of probability. But, yes, it's absurd to prmise that archiver process
crashes.

The case I'm worried about is a misconfigured archive_command that
causes the archiver to misbehave (exit with a code other than 0); if
that already doesn't happen, or we can make it not happen, then I'm okay
with the changes to archiver.

Well, an exit(1) is also fine, afaict. No?

The archive command can just trigger either a FATAL or a LOG:

rc = system(xlogarchcmd);
if (rc != 0)
{
/*
* If either the shell itself, or a called command, died on a signal,
* abort the archiver. We do this because system() ignores SIGINT and
* SIGQUIT while waiting; so a signal is very likely something that
* should have interrupted us too. Also die if the shell got a hard
* "command not found" type of error. If we overreact it's no big
* deal, the postmaster will just start the archiver again.
*/
int lev = wait_result_is_any_signal(rc, true) ? FATAL : LOG;

if (WIFEXITED(rc))
{
ereport(lev,
(errmsg("archive command failed with exit code %d",
WEXITSTATUS(rc)),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
}
else if (WIFSIGNALED(rc))
{
#if defined(WIN32)
ereport(lev,
(errmsg("archive command was terminated by exception 0x%X",
WTERMSIG(rc)),
errhint("See C include file \"ntstatus.h\" for a description of the hexadecimal value."),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
#else
ereport(lev,
(errmsg("archive command was terminated by signal %d: %s",
WTERMSIG(rc), pg_strsignal(WTERMSIG(rc))),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
#endif
}
else
{
ereport(lev,
(errmsg("archive command exited with unrecognized status %d",
rc),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
}

snprintf(activitymsg, sizeof(activitymsg), "failed on %s", xlog);
set_ps_display(activitymsg, false);

return false;
}

I.e. there's only normal ways to shut down the archiver due to a failing
archvie command.

Greetings,

Andres Freund

Hi,

On 2020-03-10 19:52:22 +0100, Julien Rouhaud wrote:

On Tue, Mar 10, 2020 at 1:48 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Mar-10, Kyotaro Horiguchi wrote:

At Mon, 9 Mar 2020 20:34:20 -0700, Andres Freund <andres@anarazel.de> wrote in

On 2020-03-10 12:27:25 +0900, Kyotaro Horiguchi wrote:

That's true, but I have the same concern with Tom. The archive bacame
too-tightly linked with other processes than actual relation.

What's the problem here? We have a number of helper processes
(checkpointer, bgwriter) that are attached to shared memory, and it's
not a problem.

That theoretically raises the chance of server-crash by a small amount
of probability. But, yes, it's absurd to prmise that archiver process
crashes.

The case I'm worried about is a misconfigured archive_command that
causes the archiver to misbehave (exit with a code other than 0); if
that already doesn't happen, or we can make it not happen, then I'm okay
with the changes to archiver.

Any script that gets killed, or that exit with a return code > 127
would do that.

But just with a FATAL, not with something worse. And the default
handling for aux backends accepts exit code 1 (which elog uses for
FATAL) as a normal shutdown. Am I missing something here?

Greetings,

Andres Freund

Hi,

Thomas, could you look at the first two patches here, and my review
questions?

General comments about this series:
- A lot of the review comments feel like I've written them before, a
year or more ago. I feel this patch ought to be in a much better
state. There's a lot of IMO fairly obvious stuff here, and things that
have been mentioned multiple times previously.
- There's a *lot* of typos in here. I realize being an ESL is hard, but
a lot of these can be found with the simplest spellchecker. That's
one thing for a patch that just has been hacked up as a POC, but this
is a multi year thread?
- There's some odd formatting. Consider using pgindent more regularly.

More detailed comments below.

I'm considering rewriting the parts of the patchset that I don't like -
but it'll look quite different afterwards.

On 2020-01-22 17:24:04 +0900, Kyotaro Horiguchi wrote:

From 5f7946522dc189429008e830af33ff2db435dd42 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 29 Jun 2018 16:41:04 +0900
Subject: [PATCH 1/5] sequential scan for dshash

Add sequential scan feature to dshash.

dsa_pointer item_pointer = hash_table->buckets[i];
@@ -549,9 +560,14 @@ dshash_delete_entry(dshash_table *hash_table, void *entry)
LW_EXCLUSIVE));

delete_item(hash_table, item);
-	hash_table->find_locked = false;
-	hash_table->find_exclusively_locked = false;
-	LWLockRelease(PARTITION_LOCK(hash_table, partition));
+
+	/* We need to keep partition lock while sequential scan */
+	if (!hash_table->seqscan_running)
+	{
+		hash_table->find_locked = false;
+		hash_table->find_exclusively_locked = false;
+		LWLockRelease(PARTITION_LOCK(hash_table, partition));
+	}
}

This seems like a failure prone API.

/*
@@ -568,6 +584,8 @@ dshash_release_lock(dshash_table *hash_table, void *entry)
Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition_index),
hash_table->find_exclusively_locked
? LW_EXCLUSIVE : LW_SHARED));
+	/* lock is under control of sequential scan */
+	Assert(!hash_table->seqscan_running);

hash_table->find_locked = false;
hash_table->find_exclusively_locked = false;
@@ -592,6 +610,164 @@ dshash_memhash(const void *v, size_t size, void *arg)
return tag_hash(v, size);
}

+/*
+ * dshash_seq_init/_next/_term
+ *           Sequentially scan trhough dshash table and return all the
+ *           elements one by one, return NULL when no more.

s/trhough/through/

This uses a different comment style that the other functions in this
file. Why?

+ * dshash_seq_term should be called for incomplete scans and otherwise
+ * shoudln't. Finished scans are cleaned up automatically.

s/shoudln't/shouldn't/

I find the "cleaned up automatically" API terrible. I know you copied it
from dynahash, but I find it to be really failure prone. dynahash isn't
an example of good postgres code, the opposite, I'd say. It's a lot
easier to unconditionally have a terminate call if we need that.

+ * Returned elements are locked as is the case with dshash_find.  However, the
+ * caller must not release the lock.
+ *
+ * Same as dynanash, the caller may delete returned elements midst of a scan.

I think it's a bad idea to refer to dynahash here. That's just going to
get out of date. Also, code should be documented on its own.

+ * If consistent is set for dshash_seq_init, the all hash table partitions are
+ * locked in the requested mode (as determined by the exclusive flag) during
+ * the scan.  Otherwise partitions are locked in one-at-a-time way during the
+ * scan.

Yet delete unconditionally retains locks?

+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+				bool consistent, bool exclusive)
+{

Why does this patch add the consistent mode? There's no users currently?
Without it's not clear that we need a seperate _term function, I think?

I think we also can get rid of the dshash_delete changes, by instead
adding a dshash_delete_current(dshash_seq_stat *status, void *entry) API
or such.

@@ -70,7 +86,6 @@ extern dshash_table *dshash_attach(dsa_area *area,
extern void dshash_detach(dshash_table *hash_table);
extern dshash_table_handle dshash_get_hash_table_handle(dshash_table *hash_table);
extern void dshash_destroy(dshash_table *hash_table);
-
/* Finding, creating, deleting entries. */
extern void *dshash_find(dshash_table *hash_table,
const void *key, bool
exclusive);

There's a number of spurious changes like this.

From 60da67814fe40fd2a0c1870b15dcf6fcb21c989a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 27 Sep 2018 11:15:19 +0900
Subject: [PATCH 2/5] Add conditional lock feature to dshash

Dshash currently waits for lock unconditionally. This commit adds new
interfaces for dshash_find and dshash_find_or_insert. The new
interfaces have an extra parameter "nowait" taht commands not to wait
for lock.

s/taht/that/

There should be at least a sentence or two explaining why these are
useful.

+/*
+ * The version of dshash_find, which is allowed to return immediately on lock
+ * failure. Lock status is set to *lock_failed in that case.
+ */

Hm. Not sure I like the *lock_acquired API.

+void *
+dshash_find_extended(dshash_table *hash_table, const void *key,
+					 bool exclusive, bool nowait, bool *lock_acquired)
{
dshash_hash hash;
size_t		partition;
dshash_table_item *item;

+	/*
+	 * No need to return lock resut when !nowait. Otherwise the caller may
+	 * omit the lock result when NULL is returned.
+	 */
+	Assert(nowait || !lock_acquired);
+
hash = hash_key(hash_table, key);
partition = PARTITION_FOR_HASH(hash);

Assert(hash_table->control->magic == DSHASH_MAGIC);
Assert(!hash_table->find_locked);

-	LWLockAcquire(PARTITION_LOCK(hash_table, partition),
-				  exclusive ? LW_EXCLUSIVE : LW_SHARED);
+	if (nowait)
+	{
+		if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partition),
+									  exclusive ? LW_EXCLUSIVE : LW_SHARED))
+		{
+			if (lock_acquired)
+				*lock_acquired = false;

Why is the test for lock_acquired needed here? I don't think it's
possible to use nowait correctly without passing in lock_acquired?

Think it'd make sense to document & assert that nowait = true implies
lock_acquired set, and nowait = false implies lock_acquired not being
set.

But, uh, why do we even need the lock_acquired parameter? If we couldn't
find an entry, then we should just release the lock, no?

I'm however inclined to think it's better to just have a separate
function for the nowait case, rather than an extended version supporting
both (with an internal helper doing most of the work).

+/*
+ * The version of dshash_find_or_insert, which is allowed to return immediately
+ * on lock failure.
+ *
+ * Notes above dshash_find_extended() regarding locking and error handling
+ * equally apply here.

They don't, there's no lock_acquired parameter.

+ */
+void *
+dshash_find_or_insert_extended(dshash_table *hash_table,
+							   const void *key,
+							   bool *found,
+							   bool nowait)

I think it's absurd to have dshash_find, dshash_find_extended,
dshash_find_or_insert, dshash_find_or_insert_extended. If they're
extended they should also be able to specify whether the entry will get
created.

From d10c1117cec77a474dbb2cff001086d828b79624 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 7 Nov 2018 16:53:49 +0900
Subject: [PATCH 3/5] Make archiver process an auxiliary process

This is a preliminary patch for shared-memory based stats collector.
Archiver process must be a auxiliary process since it uses shared
memory after stats data wes moved onto shared-memory. Make the process

s/wes/was/ s/onto/into/

an auxiliary process in order to make it work.

@@ -451,6 +454,11 @@ AuxiliaryProcessMain(int argc, char *argv[])
StartupProcessMain();
proc_exit(1); /* should never return */

+		case ArchiverProcess:
+			/* don't set signals, archiver has its own agenda */
+			PgArchiverMain();
+			proc_exit(1);		/* should never return */
+
case BgWriterProcess:
/* don't set signals, bgwriter has its own agenda */
BackgroundWriterMain();

I think I'd rather remove the two comments that are copied to 6 out of 8
cases - they don't add anything.

/* ------------------------------------------------------------
* Local functions called by archiver follow
* ------------------------------------------------------------
@@ -219,8 +148,8 @@ pgarch_forkexec(void)
*	The argc/argv parameters are valid only in EXEC_BACKEND case.  However,
*	since we don't use 'em, it hardly matters...
*/
-NON_EXEC_STATIC void
-PgArchiverMain(int argc, char *argv[])
+void
+PgArchiverMain(void)
{
/*
* Ignore all signals usually bound to some action in the postmaster,
@@ -252,8 +181,27 @@ PgArchiverMain(int argc, char *argv[])
static void
pgarch_exit(SIGNAL_ARGS)
{
-	/* SIGQUIT means curl up and die ... */
-	exit(1);
+	PG_SETMASK(&BlockSig);
+
+	/*
+	 * We DO NOT want to run proc_exit() callbacks -- we're here because
+	 * shared memory may be corrupted, so we don't want to try to clean up our
+	 * transaction.  Just nail the windows shut and get out of town.  Now that
+	 * there's an atexit callback to prevent third-party code from breaking
+	 * things by calling exit() directly, we have to reset the callbacks
+	 * explicitly to make this work as intended.
+	 */
+	on_exit_reset();
+
+	/*
+	 * Note we do exit(2) not exit(0).  This is to force the postmaster into a
+	 * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random
+	 * process.  This is necessary precisely because we don't clean up our
+	 * shared memory state.  (The "dead man switch" mechanism in pmsignal.c
+	 * should ensure the postmaster sees this as a crash, too, but no harm in
+	 * being doubly sure.)
+	 */
+	exit(2);
}

This seems to be a copy of code & comments from other signal handlers that predates

commit 8e19a82640d3fa2350db146ec72916856dd02f0a
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: 2018-08-08 19:08:10 +0300

Don't run atexit callbacks in quickdie signal handlers.

I think this just should use SignalHandlerForCrashExit().

I think we can even commit that separately - there's not really a reason
to not do that today, as far as I can tell?

/* SIGUSR1 signal handler for archiver process */

Hm - this currently doesn't set up a correct sigusr1 handler for a
shared memory backend - needs to invoke procsignal_sigusr1_handler
somewhere.

We can probably just convert to using normal latches here, and remove
the current 'wakened' logic? That'll remove the indirection via
postmaster too, which is nice.

@@ -4273,6 +4276,9 @@ pgstat_get_backend_desc(BackendType backendType)

switch (backendType)
{
+		case B_ARCHIVER:
+			backendDesc = "archiver";
+			break;

should imo include 'WAL' or such.

From 5079583c447c3172aa0b4f8c0f0a46f6e1512812 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 21 Feb 2019 12:44:56 +0900
Subject: [PATCH 4/5] Shared-memory based stats collector

Previously activity statistics is shared via files on disk. Every
backend sends the numbers to the stats collector process via a socket.
It makes snapshots as a set of files on disk with a certain interval
then every backend reads them as necessary. It worked fine for
comparatively small set of statistics but the set is under the
pressure to growing up and the file size has reached the order of
megabytes. To deal with larger statistics set, this patch let backends
directly share the statistics via shared memory.

This spends a fair bit describing the old state, but very little
describing the new state.

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 0bfd6151c4..a6b0bdec12 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -53,7 +53,6 @@ postgres  15554  0.0  0.0  57536  1184 ?        Ss   18:02   0:00 postgres: back
postgres  15555  0.0  0.0  57536   916 ?        Ss   18:02   0:00 postgres: checkpointer
postgres  15556  0.0  0.0  57536   916 ?        Ss   18:02   0:00 postgres: walwriter
postgres  15557  0.0  0.0  58504  2244 ?        Ss   18:02   0:00 postgres: autovacuum launcher
-postgres  15558  0.0  0.0  17512  1068 ?        Ss   18:02   0:00 postgres: stats collector
postgres  15582  0.0  0.0  58772  3080 ?        Ss   18:04   0:00 postgres: joe runbug 127.0.0.1 idle
postgres  15606  0.0  0.0  58772  3052 ?        Ss   18:07   0:00 postgres: tgl regression [local] SELECT waiting
postgres  15610  0.0  0.0  58772  3056 ?        Ss   18:07   0:00 postgres: tgl regression [local] idle in transaction
@@ -65,9 +64,8 @@ postgres  15610  0.0  0.0  58772  3056 ?        Ss   18:07   0:00 postgres: tgl
master server process.  The command arguments
shown for it are the same ones used when it was launched.  The next five
processes are background worker processes automatically launched by the
-   master process.  (The <quote>stats collector</quote> process will not be present
-   if you have set the system not to start the statistics collector; likewise
-   the <quote>autovacuum launcher</quote> process can be disabled.)
+   master process.  (The <quote>autovacuum launcher</quote> process will not
+   be present if you have set the system not to start it.)
Each of the remaining
processes is a server process handling one client connection.  Each such
process sets its command line display in the form

There's more references to the stats collector than this... E.g. in
catalogs.sgml

<xref linkend="view-table"/> lists the system views described here.
More detailed documentation of each view follows below.
There are some additional views that provide access to the results of
the statistics collector; they are described in <xref
linkend="monitoring-stats-views-table"/>.
</para>

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 6d1f28c327..8dcb0fb7f7 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1956,15 +1956,15 @@ do_autovacuum(void)
ALLOCSET_DEFAULT_SIZES);
MemoryContextSwitchTo(AutovacMemCxt);

+	/* Start a transaction so our commands have one to play into. */
+	StartTransactionCommand();
+
/*
* may be NULL if we couldn't find an entry (only happens if we are
* forcing a vacuum for anti-wrap purposes).
*/
dbentry = pgstat_fetch_stat_dbentry(MyDatabaseId);

-	/* Start a transaction so our commands have one to play into. */
-	StartTransactionCommand();
-
/*
* Clean up any dead statistics collector entries for this DB. We always
* want to do this exactly once per DB-processing cycle, even if we find
@@ -2747,12 +2747,10 @@ get_pgstat_tabentry_relid(Oid relid, bool isshared, PgStat_StatDBEntry *shared,
if (isshared)
{
if (PointerIsValid(shared))
-			tabentry = hash_search(shared->tables, &relid,
-								   HASH_FIND, NULL);
+			tabentry = pgstat_fetch_stat_tabentry_extended(shared, relid);
}
else if (PointerIsValid(dbentry))
-		tabentry = hash_search(dbentry->tables, &relid,
-							   HASH_FIND, NULL);
+		tabentry = pgstat_fetch_stat_tabentry_extended(dbentry, relid);

return tabentry;
}

Why is pgstat_fetch_stat_tabentry_extended called "_extended"? Outside
the stats subsystem there are exactly one caller for the non extended
version, as far as I can see. That's index_concurrently_swap() - and imo
that's code that should live in the stats subsystem, rather than open
coded in index.c.

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ca5c6376e5..1ffe073a1f 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1,15 +1,23 @@
/* ----------
* pgstat.c
*
- *	All the statistics collector stuff hacked up in one big, ugly file.
+ *	Statistics collector facility.
*
- *	TODO:	- Separate collector, postmaster and backend stuff
- *			  into different files.
+ *  Collects per-table and per-function usage statistics of all backends on
+ *  shared memory. pg_count_*() and friends are the interface to locally store
+ *  backend activities during a transaction. Then pgstat_flush_stat() is called
+ *  at the end of a transaction to pulish the local stats on shared memory.
*

I'd rather not exhaustively list the different objects this handles -
it'll either be annoying to maintain, or just get out of date.

- *			- Add some automatic call for pgstat vacuuming.
+ *  To avoid congestion on the shared memory, we update shared stats no more
+ *  often than intervals of PGSTAT_STAT_MIN_INTERVAL(500ms). In the case where
+ *  all the local numbers cannot be flushed immediately, we postpone updates
+ *  and try the next chance after the interval of
+ *  PGSTAT_STAT_RETRY_INTERVAL(100ms), but we don't wait for no longer than
+ *  PGSTAT_STAT_MAX_INTERVAL(1000ms).

I'm not convinced by this backoff logic. The basic interval seems quite
high for something going through shared memory, and the max retry seems
pretty low.

+/*
+ * Operation mode and return code of pgstat_get_db_entry.
+ */
+#define	PGSTAT_SHARED		0

This is unreferenced.

+#define PGSTAT_EXCLUSIVE 1
+#define PGSTAT_NOWAIT 2

And these should imo rather be parameters.

+typedef enum PgStat_TableLookupResult
+{
+	NOT_FOUND,
+	FOUND,
+	LOCK_FAILED
+} PgStat_TableLookupResult;

This seems like a seriously bad idea to me. These are very generic
names. There's also basically no references except setting them to the
first two?

+#define StatsLock (&StatsShmem->StatsMainLock)

-static time_t last_pgstat_start_time;
+/* Shared stats bootstrap information */
+typedef struct StatsShmemStruct
+{
+	LWLock				StatsMainLock;		/* lock to protect this struct */
+	dsa_handle 			stats_dsa_handle;	/* DSA handle for stats data */
+	dshash_table_handle db_hash_handle;
+	dsa_pointer			global_stats;
+	dsa_pointer			archiver_stats;
+	int					refcount;
+} StatsShmemStruct;

Why isn't this an lwlock in lwlock in lwlocknames.h, rather than
allocated here?

+/*
+ * BgWriter global statistics counters. The name cntains a remnant from the
+ * time when the stats collector was a dedicate process, which used sockets to
+ * send it.
+ */
+PgStat_MsgBgWriter BgWriterStats = {0};

I am strongly against keeping the 'Msg' prefix. That seems extremely
confusing going forward.

+/* common header of snapshot entry in reader snapshot hash */
+typedef struct PgStat_snapshot
+{
+	Oid		key;
+	bool	negative;
+	void   *body;				/* end of header part: to keep alignment */
+} PgStat_snapshot;

+/* context struct for snapshot_statentry */
+typedef struct pgstat_snapshot_param
+{
+	char		   *hash_name;			/* name of the snapshot hash */
+	int				hash_entsize;		/* element size of hash entry */
+	dshash_table_handle	dsh_handle;		/* dsh handle to attach */
+	const dshash_parameters *dsh_params;/* dshash params */
+	HTAB		  **hash;				/* points to variable to hold hash */
+	dshash_table  **dshash;				/* ditto for dshash */
+} pgstat_snapshot_param;

Why does this exist? The struct contents are actually constant across
calls, yet you have declared them inside functions (as static - static
on function scope isn't really the same as global static).

If we want it, I think we should separate the naming more
meaningfully. The important difference between 'hash' and 'dshash' isn't
the hashing module, it's that one is a local copy, the other a shared
hashtable!

+/*
+ * Backends store various database-wide info that's waiting to be flushed out
+ * to shared memory in these variables.
+ *
+ * checksum_failures is the exception in that it is cluster-wide value.
+ */
+typedef struct BackendDBStats
+{
+	int		n_conflict_tablespace;
+	int		n_conflict_lock;
+	int		n_conflict_snapshot;
+	int		n_conflict_bufferpin;
+	int		n_conflict_startup_deadlock;
+	int		n_deadlocks;
+	size_t	n_tmpfiles;
+	size_t	tmpfilesize;
+	HTAB	*checksum_failures;
+} BackendDBStats;

Why is this a separate struct from PgStat_StatDBEntry? We should have
these fields in multiple places.

+ if (StatsShmem->refcount > 0)
+ StatsShmem->refcount++;

What prevents us from leaking the refcount here? We could e.g. error out
while attaching, no? Which'd mean we'd leak the refcount.

To me it looks like there's a lot of added complexity just because you
want to be able to reset stats via

void
pgstat_reset_all(void)a
{

/*
* We could directly remove files and recreate the shared memory area. But
* detach then attach for simplicity.
*/
pgstat_detach_shared_stats(false); /* Don't write */
pgstat_attach_shared_stats();

Without that you'd not need the complexity of attaching, detaching to
the same degree - every backend could just cache lookup data during
initialization, instead of having to constantly re-compute that.

Nor would the dynamic re-creation of the db dshash table be needed.

+/* ----------
+ * pgstat_report_stat() -
+ *
+ *	Must be called by processes that performs DML: tcop/postgres.c, logical
+ *	receiver processes, SPI worker, etc. to apply the so far collected
+ *	per-table and function usage statistics to the shared statistics hashes.
+ *
+ *  Updates are applied not more frequent than the interval of
+ *  PGSTAT_STAT_MIN_INTERVAL milliseconds. They are also postponed on lock
+ *  failure if force is false and there's no pending updates longer than
+ *  PGSTAT_STAT_MAX_INTERVAL milliseconds. Postponed updates are retried in
+ *  succeeding calls of this function.
+ *
+ *	Returns the time until the next timing when updates are applied in
+ *	milliseconds if there are no updates holded for more than
+ *	PGSTAT_STAT_MIN_INTERVAL milliseconds.
+ *
+ *	Note that this is called only out of a transaction, so it is fine to use
+ *	transaction stop time as an approximation of current time.
+ *	----------
+ */

Inconsistent indentation.

+long
+pgstat_report_stat(bool force)
{

+	/* Flush out table stats */
+	if (pgStatTabList != NULL && !pgstat_flush_stat(&cxt, !force))
+		pending_stats = true;
+
+	/* Flush out function stats */
+	if (pgStatFunctions != NULL && !pgstat_flush_funcstats(&cxt, !force))
+		pending_stats = true;

This seems weird. pgstat_flush_stat(), pgstat_flush_funcstats() operate
on pgStatTabList/pgStatFunctions, but don't actually reset it? Besides
being confusing while reading the code, it also made the diff much
harder to read.

-		snprintf(fname, sizeof(fname), "%s/%s", directory,
-				 entry->d_name);
-		unlink(fname);
+	/* Flush out database-wide stats */
+	if (HAVE_PENDING_DBSTATS())
+	{
+		if (!pgstat_flush_dbstats(&cxt, !force))
+			pending_stats = true;
}

Linearly checking a number of stats doesn't seem like the right way
going forward. Also seems fairly omission prone.

Why does this code check live in pgstat_report_stat(), rather than
pgstat_flush_dbstats()?

/*
* snapshot_statentry() - Common routine for functions
* pgstat_fetch_stat_*entry()
*

Why has this function been added between the closely linked
pgstat_report_stat() and pgstat_flush_stat() etc?

* Returns the pointer to a snapshot of a shared entry for the key or NULL if
* not found. Returned snapshots are stable during the current transaction or
* until pgstat_clear_snapshot() is called.
*
* The snapshots are stored in a hash, pointer to which is stored in the
* *HTAB variable pointed by cxt->hash. If not created yet, it is created
* using hash_name, hash_entsize in cxt.
*
* cxt->dshash points to dshash_table for dbstat entries. If not yet
* attached, it is attached using cxt->dsh_handle.

Why do we still have this? A hashtable lookup is cheap, compared to
fetching a file - so it's not to save time. Given how infrequent the
pgstat_fetch_* calls are, it's not to avoid contention either.

At first one could think it's for consistency - but no, that's not it
either, because snapshot_statentry() refetches the snapshot without
control from the outside:

/*
* We don't want so frequent update of stats snapshot. Keep it at least
* for PGSTAT_STAT_MIN_INTERVAL ms. Not postpone but just ignore the cue.
*/
if (clear_snapshot)
{
clear_snapshot = false;

if (pgStatSnapshotContext &&
snapshot_globalStats.stats_timestamp <
GetCurrentStatementStartTimestamp() -
PGSTAT_STAT_MIN_INTERVAL * 1000)
{
MemoryContextReset(pgStatSnapshotContext);

/* Reset variables */
global_snapshot_is_valid = false;
pgStatSnapshotContext = NULL;
pgStatLocalHash = NULL;

pgstat_setup_memcxt();
}
}

I think we should just remove this entire local caching snapshot layer
for lookups.

/*
* pgstat_flush_stat: Flushes table stats out to shared statistics.
*

Why is this named pgstat_flush_stat, rather than pgstat_flush_tabstats
or such? Given that the code for dealing with an individual table's
entry is named pgstat_flush_tabstat() that's very confusing.

* If nowait is true, returns false if required lock was not acquired
* immediately. In that case, unapplied table stats updates are left alone in
* TabStatusArray to wait for the next chance. cxt holds some dshash related
* values that we want to carry around while updating shared stats.
*
* Returns true if all stats info are flushed. Caller must detach dshashes
* stored in cxt after use.
*/
static bool
pgstat_flush_stat(pgstat_flush_stat_context *cxt, bool nowait)
{
static const PgStat_TableCounts all_zeroes;
TabStatusArray *tsa;
HTAB *new_tsa_hash = NULL;
TabStatusArray *dest_tsa = pgStatTabList;
int dest_elem = 0;
int i;

/* nothing to do, just return */
if (pgStatTabHash == NULL)
return true;

/*
* Destroy pgStatTabHash before we start invalidating PgStat_TableEntry
* entries it points to.
*/
hash_destroy(pgStatTabHash);
pgStatTabHash = NULL;

/*
* Scan through the TabStatusArray struct(s) to find tables that actually
* have counts, and try flushing it out to shared stats. We may fail on
* some entries in the array. Leaving the entries being packed at the
* beginning of the array.
*/
for (tsa = pgStatTabList; tsa != NULL; tsa = tsa->tsa_next)
{

It seems odd that there's a tabstat specific code in pgstat_flush_stat
(also note singular while it's processing all stats, whereas you're
below treating pgstat_flush_tabstat as only affecting one table).

for (i = 0; i < tsa->tsa_used; i++)
{
PgStat_TableStatus *entry = &tsa->tsa_entries[i];

/* Shouldn't have any pending transaction-dependent counts */
Assert(entry->trans == NULL);

/*
* Ignore entries that didn't accumulate any actual counts, such
* as indexes that were opened by the planner but not used.
*/
if (memcmp(&entry->t_counts, &all_zeroes,
sizeof(PgStat_TableCounts)) == 0)
continue;

/* try to apply the tab stats */
if (!pgstat_flush_tabstat(cxt, nowait, entry))
{
/*
* Failed. Move it to the beginning in TabStatusArray and
* leave it.
*/
TabStatHashEntry *hash_entry;
bool found;

if (new_tsa_hash == NULL)
new_tsa_hash = create_tabstat_hash();

/* Create hash entry for this entry */
hash_entry = hash_search(new_tsa_hash, &entry->t_id,
HASH_ENTER, &found);
Assert(!found);

/*
* Move insertion pointer to the next segment if the segment
* is filled up.
*/
if (dest_elem >= TABSTAT_QUANTUM)
{
Assert(dest_tsa->tsa_next != NULL);
dest_tsa = dest_tsa->tsa_next;
dest_elem = 0;
}

/*
* Pack the entry at the begining of the array. Do nothing if
* no need to be moved.
*/
if (tsa != dest_tsa || i != dest_elem)
{
PgStat_TableStatus *new_entry;
new_entry = &dest_tsa->tsa_entries[dest_elem];
*new_entry = *entry;

/* use new_entry as entry hereafter */
entry = new_entry;
}

hash_entry->tsa_entry = entry;
dest_elem++;
}

This seems like too much code. Why is this entirely different from the
way funcstats works? The difference was already too big before, but this
made it *way* worse.

One goal of this project, as I understand it, is to make it easier to
add additional stats. As is, this seems to make it harder from the code
level.

bool
pgstat_flush_tabstat(pgstat_flush_stat_context *cxt, bool nowait,
PgStat_TableStatus *entry)
{
Oid dboid = entry->t_shared ? InvalidOid : MyDatabaseId;
int table_mode = PGSTAT_EXCLUSIVE;
bool updated = false;
dshash_table *tabhash;
PgStat_StatDBEntry *dbent;
int generation;

if (nowait)
table_mode |= PGSTAT_NOWAIT;

/* Attach required table hash if not yet. */
if ((entry->t_shared ? cxt->shdb_tabhash : cxt->mydb_tabhash) == NULL)
{
/*
* Return if we don't have corresponding dbentry. It would've been
* removed.
*/
dbent = pgstat_get_db_entry(dboid, table_mode, NULL);
if (!dbent)
return false;

/*
* We don't hold lock on the dbentry since it cannot be dropped while
* we are working on it.
*/
generation = pin_hashes(dbent);
tabhash = attach_table_hash(dbent, generation);

This again is just cost incurred by insisting on destroying hashtables
instead of keeping them around as long as necessary.

if (entry->t_shared)
{
cxt->shgeneration = generation;
cxt->shdbentry = dbent;
cxt->shdb_tabhash = tabhash;
}
else
{
cxt->mygeneration = generation;
cxt->mydbentry = dbent;
cxt->mydb_tabhash = tabhash;

/*
* We come here once per database. Take the chance to update
* database-wide stats
*/
LWLockAcquire(&dbent->lock, LW_EXCLUSIVE);
dbent->n_xact_commit += pgStatXactCommit;
dbent->n_xact_rollback += pgStatXactRollback;
dbent->n_block_read_time += pgStatBlockReadTime;
dbent->n_block_write_time += pgStatBlockWriteTime;
LWLockRelease(&dbent->lock);
pgStatXactCommit = 0;
pgStatXactRollback = 0;
pgStatBlockReadTime = 0;
pgStatBlockWriteTime = 0;
}
}
else if (entry->t_shared)
{
dbent = cxt->shdbentry;
tabhash = cxt->shdb_tabhash;
}
else
{
dbent = cxt->mydbentry;
tabhash = cxt->mydb_tabhash;
}

/*
* Local table stats should be applied to both dbentry and tabentry at
* once. Update dbentry only if we could update tabentry.
*/
if (pgstat_update_tabentry(tabhash, entry, nowait))
{
pgstat_update_dbentry(dbent, entry);
updated = true;
}

At this point we're very deeply nested. pgstat_report_stat() ->
pgstat_flush_stat() -> pgstat_flush_tabstat() ->
pgstat_update_tabentry().

That's way over the top imo.

I don't think it makes much sense that pgstat_update_dbentry() is called
separately for each table. Why would we want to constantly lock that
entry? It seems to be much more sensible to instead have
pgstat_flush_stat() transfer the stats it reported to the pending
database wide counters, and then report that to shared memory *once* per
pgstat_report_stat() with pgstat_flush_dbstats()?

/*
* pgstat_flush_dbstats: Flushes out miscellaneous database stats.
*
* If nowait is true, returns with false on lock failure on dbentry.
*
* Returns true if all stats are flushed out.
*/
static bool
pgstat_flush_dbstats(pgstat_flush_stat_context *cxt, bool nowait)
{
/* get dbentry if not yet */
if (cxt->mydbentry == NULL)
{
int op = PGSTAT_EXCLUSIVE;
if (nowait)
op |= PGSTAT_NOWAIT;

cxt->mydbentry = pgstat_get_db_entry(MyDatabaseId, op, NULL);

/* return if lock failed. */
if (cxt->mydbentry == NULL)
return false;

/* we use this generation of table /function stats in this turn */
cxt->mygeneration = pin_hashes(cxt->mydbentry);
}

LWLockAcquire(&cxt->mydbentry->lock, LW_EXCLUSIVE);
if (HAVE_PENDING_CONFLICTS())
pgstat_flush_recovery_conflict(cxt->mydbentry);
if (BeDBStats.n_deadlocks != 0)
pgstat_flush_deadlock(cxt->mydbentry);
if (BeDBStats.n_tmpfiles != 0)
pgstat_flush_tempfile(cxt->mydbentry);
if (BeDBStats.checksum_failures != NULL)
pgstat_flush_checksum_failure(cxt->mydbentry);
LWLockRelease(&cxt->mydbentry->lock);

What's the point of having all these sub-functions? I see that you, for
an undocumented reason, have pgstat_report_recovery_conflict() flush
conflict stats immediately:

dbentry = pgstat_get_db_entry(MyDatabaseId,
PGSTAT_EXCLUSIVE | PGSTAT_NOWAIT,
&status);

if (status == LOCK_FAILED)
return;

/* We had a chance to flush immediately */
pgstat_flush_recovery_conflict(dbentry);

dshash_release_lock(pgStatDBHash, dbentry);

But I don't understand why? Nor why we'd not just report all pending
database wide changes in that case?

The fact that you're locking the per-database entry unconditionally once
for each table almost guarantees contention - and you're not using the
'conditional lock' approach for that. I don't understand.

/* ----------
* pgstat_vacuum_stat() -
*
* Remove objects we can get rid of.
* ----------
*/
void
pgstat_vacuum_stat(void)
{
HTAB *oidtab;
dshash_seq_status dshstat;
PgStat_StatDBEntry *dbentry;

/* we don't collect stats under standalone mode */
if (!IsUnderPostmaster)
return;

/*
* Read pg_database and make a list of OIDs of all existing databases
*/
oidtab = pgstat_collect_oids(DatabaseRelationId, Anum_pg_database_oid);

/*
* Search the database hash table for dead databases and drop them
* from the hash.
*/

dshash_seq_init(&dshstat, pgStatDBHash, false, true);
while ((dbentry = (PgStat_StatDBEntry *) dshash_seq_next(&dshstat)) != NULL)
{
Oid dbid = dbentry->databaseid;

CHECK_FOR_INTERRUPTS();

/* the DB entry for shared tables (with InvalidOid) is never dropped */
if (OidIsValid(dbid) &&
hash_search(oidtab, (void *) &dbid, HASH_FIND, NULL) == NULL)
pgstat_drop_database(dbid);
}

/* Clean up */
hash_destroy(oidtab);

So, uh, pgstat_drop_database() again does a *separate* lookup in the
dshash, locking the entry. Which only works because you added this dirty
hack:

/* We need to keep partition lock while sequential scan */
if (!hash_table->seqscan_running)
{
hash_table->find_locked = false;
hash_table->find_exclusively_locked = false;
LWLockRelease(PARTITION_LOCK(hash_table, partition));
}

to dshash_delete_entry(). This seems insane to me. There's not even a
comment explaining this?

/*
* Similarly to above, make a list of all known relations in this DB.
*/
oidtab = pgstat_collect_oids(RelationRelationId, Anum_pg_class_oid);

/*
* Check for all tables listed in stats hashtable if they still exist.
* Stats cache is useless here so directly search the shared hash.
*/
pgstat_remove_useless_entries(dbentry->tables, &dsh_tblparams, oidtab);

/*
* Repeat the above but we needn't bother in the common case where no
* function stats are being collected.
*/
if (dbentry->functions != DSM_HANDLE_INVALID)
{
oidtab = pgstat_collect_oids(ProcedureRelationId, Anum_pg_proc_oid);

pgstat_remove_useless_entries(dbentry->functions, &dsh_funcparams,
oidtab);
}
dshash_release_lock(pgStatDBHash, dbentry);

Wait, why are we holding the database partition lock across all this?
Again without any comments explaining why?

+void
+pgstat_send_archiver(const char *xlog, bool failed)

Why do we still have functions named pgstat_send*?

Greetings,

Andres Freund

Thank you very much!!

At Thu, 12 Mar 2020 20:13:24 -0700, Andres Freund <andres@anarazel.de> wrote in

Hi,

Thomas, could you look at the first two patches here, and my review
questions?

General comments about this series:
- A lot of the review comments feel like I've written them before, a
year or more ago. I feel this patch ought to be in a much better
state. There's a lot of IMO fairly obvious stuff here, and things that
have been mentioned multiple times previously.

I apologize for all of the obvious stuff or things that have been
mentioned.. I'll address them.

- There's a *lot* of typos in here. I realize being an ESL is hard, but
a lot of these can be found with the simplest spellchecker. That's
one thing for a patch that just has been hacked up as a POC, but this
is a multi year thread?

I'll review all changed part again. I used ispell but I should have
failed to check much of the changes.

- There's some odd formatting. Consider using pgindent more regularly.

I'll do so.

More detailed comments below.

Thank you very much for the intensive review, I'm going to revise the
patch according to them.

I'm considering rewriting the parts of the patchset that I don't like -
but it'll look quite different afterwards.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2020-03-13 16:34:50 +0900, Kyotaro Horiguchi wrote:

Thank you very much!!

At Thu, 12 Mar 2020 20:13:24 -0700, Andres Freund <andres@anarazel.de> wrote in

Hi,

Thomas, could you look at the first two patches here, and my review
questions?

General comments about this series:
- A lot of the review comments feel like I've written them before, a
year or more ago. I feel this patch ought to be in a much better
state. There's a lot of IMO fairly obvious stuff here, and things that
have been mentioned multiple times previously.

I apologize for all of the obvious stuff or things that have been
mentioned.. I'll address them.

- There's a *lot* of typos in here. I realize being an ESL is hard, but
a lot of these can be found with the simplest spellchecker. That's
one thing for a patch that just has been hacked up as a POC, but this
is a multi year thread?

I'll review all changed part again. I used ispell but I should have
failed to check much of the changes.

- There's some odd formatting. Consider using pgindent more regularly.

I'll do so.

More detailed comments below.

Thank you very much for the intensive review, I'm going to revise the
patch according to them.

I'm considering rewriting the parts of the patchset that I don't like -
but it'll look quite different afterwards.

I take your response to mean that you'd prefer to evolve the patch
largely on your own? I'm mainly asking because I think there's some
chance that we could till get this into v13, but if so we'll have to go
for it now.

Greetings,

Andres Freund

Hi Horiguchi-san, Andres,

I tried to rebase this (see attached, no intentional changes beyond
rebasing). Some feedback:

On Fri, Mar 13, 2020 at 4:13 PM Andres Freund <andres@anarazel.de> wrote:

Thomas, could you look at the first two patches here, and my review
questions?

Ack.

dsa_pointer item_pointer = hash_table->buckets[i];
@@ -549,9 +560,14 @@ dshash_delete_entry(dshash_table *hash_table, void *entry)
LW_EXCLUSIVE));

delete_item(hash_table, item);
-     hash_table->find_locked = false;
-     hash_table->find_exclusively_locked = false;
-     LWLockRelease(PARTITION_LOCK(hash_table, partition));
+
+     /* We need to keep partition lock while sequential scan */
+     if (!hash_table->seqscan_running)
+     {
+             hash_table->find_locked = false;
+             hash_table->find_exclusively_locked = false;
+             LWLockRelease(PARTITION_LOCK(hash_table, partition));
+     }
}

This seems like a failure prone API.

If I understand correctly, the only purpose of the seqscan_running
variable is to control that behaviour ^^^. That is, to make
dshash_delete_entry() keep the partition lock if you delete an entry
while doing a seq scan. Why not get rid of that, and provide a
separate interface for deleting while scanning?
dshash_seq_delete(dshash_seq_status *scan, void *entry). I suppose it
would be most common to want to delete the "current" item in the seq
scan, but it could allow you to delete anything in the same partition,
or any entry if using the "consistent" mode. Oh, I see that Andres
said the same thing later.

[Andres complaining about comments and language stuff]

I would be happy to proof read and maybe extend the comments (writing
new comments will also help me understand and review the code!), and
maybe some code changes to move this forward. Horiguchi-san, are you
working on another version now? If so I'll wait for it before I do
that.

+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+                             bool consistent, bool exclusive)
+{

Why does this patch add the consistent mode? There's no users currently?
Without it's not clear that we need a seperate _term function, I think?

+1, let's not do that if we don't need it!

The fact that you're locking the per-database entry unconditionally once
for each table almost guarantees contention - and you're not using the
'conditional lock' approach for that. I don't understand.

Right, I also noticed that:

/*
* Local table stats should be applied to both dbentry and tabentry at
* once. Update dbentry only if we could update tabentry.
*/
if (pgstat_update_tabentry(tabhash, entry, nowait))
{
pgstat_update_dbentry(dbent, entry);
updated = true;
}

So pgstat_update_tabentry() goes to great trouble to take locks
conditionally, but then pgstat_update_dbentry() immediately does:

LWLockAcquire(&dbentry->lock, LW_EXCLUSIVE);
dbentry->n_tuples_returned += stat->t_counts.t_tuples_returned;
dbentry->n_tuples_fetched += stat->t_counts.t_tuples_fetched;
dbentry->n_tuples_inserted += stat->t_counts.t_tuples_inserted;
dbentry->n_tuples_updated += stat->t_counts.t_tuples_updated;
dbentry->n_tuples_deleted += stat->t_counts.t_tuples_deleted;
dbentry->n_blocks_fetched += stat->t_counts.t_blocks_fetched;
dbentry->n_blocks_hit += stat->t_counts.t_blocks_hit;
LWLockRelease(&dbentry->lock);

Why can't we be "lazy" with the dbentry stats too? Is it really
important for the table stats and DB stats to agree with each other?
Even if it were, your current coding doesn't achieve that: the table
stats are updated before the DB stat under different locks, so I'm not
sure why it can't wait longer.

Hmm. Even if you change the above code use a conditional lock, I am
wondering (admittedly entirely without data) if this approach is still
too clunky: even trying and failing to acquire the lock creates
contention, just a bit less. I wonder if it would make sense to make
readers do more work, so that writers can avoid contention. For
example, maybe PgStat_StatDBEntry could hold an array of N sets of
counters, and readers have to add them all up. An advanced version of
this idea would use a reasonably fresh copy of something like
sched_getcpu() and numa_node_of_cpu() to select a partition to
minimise contention and cross-node traffic, with a portable fallback
based on PID or something. CPU core/node awareness is something I
haven't looked into too seriously, but it's been on my mind to solve
some other problems.

Thank you for the comment.

The new version is attached.

At Thu, 12 Mar 2020 20:13:24 -0700, Andres Freund <andres@anarazel.de> wrote in

General comments about this series:
- A lot of the review comments feel like I've written them before, a
year or more ago. I feel this patch ought to be in a much better
state. There's a lot of IMO fairly obvious stuff here, and things that
have been mentioned multiple times previously.
- There's a *lot* of typos in here. I realize being an ESL is hard, but
a lot of these can be found with the simplest spellchecker. That's
one thing for a patch that just has been hacked up as a POC, but this
is a multi year thread?
- There's some odd formatting. Consider using pgindent more regularly.

More detailed comments below.

I'm considering rewriting the parts of the patchset that I don't like -
but it'll look quite different afterwards.

On 2020-01-22 17:24:04 +0900, Kyotaro Horiguchi wrote:

From 5f7946522dc189429008e830af33ff2db435dd42 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 29 Jun 2018 16:41:04 +0900
Subject: [PATCH 1/5] sequential scan for dshash

Add sequential scan feature to dshash.

dsa_pointer item_pointer = hash_table->buckets[i];
@@ -549,9 +560,14 @@ dshash_delete_entry(dshash_table *hash_table, void *entry)
LW_EXCLUSIVE));

delete_item(hash_table, item);
-	hash_table->find_locked = false;
-	hash_table->find_exclusively_locked = false;
-	LWLockRelease(PARTITION_LOCK(hash_table, partition));
+
+	/* We need to keep partition lock while sequential scan */
+	if (!hash_table->seqscan_running)
+	{
+		hash_table->find_locked = false;
+		hash_table->find_exclusively_locked = false;
+		LWLockRelease(PARTITION_LOCK(hash_table, partition));
+	}
}

This seems like a failure prone API.

[001]: (Fixed) As the result of the fixed in [044], it's gone now.
As the result of the fixed in [044]Following [001] and [009], I added dshash_delete_currenet(). pgstat_vacuum_stat() uses it instead of pgstat_delete_entry(). The hack is gone., it's gone now.

/*
@@ -568,6 +584,8 @@ dshash_release_lock(dshash_table *hash_table, void *entr> > + * dshash_seq_init/_next/_term
+ *           Sequentially scan trhough dshash table and return all the
+ *           elements one by one, return NULL when no more.

s/trhough/through/

[002]: (Fixed)

This uses a different comment style that the other functions in this
file. Why?

[003]: (Fixed)

It was following the equivalent in dynahash.c. I rewrote it different
way.

+ * dshash_seq_term should be called for incomplete scans and otherwise
+ * shoudln't. Finished scans are cleaned up automatically.

s/shoudln't/shouldn't/

[004]: (Fixed)

I find the "cleaned up automatically" API terrible. I know you copied it
from dynahash, but I find it to be really failure prone. dynahash isn't
an example of good postgres code, the opposite, I'd say. It's a lot
easier to unconditionally have a terminate call if we need that.

[005]: (Fixed) OK, I remember I had a similar thought on this. Fixed with the all correspondents.
OK, I remember I had a similar thought on this. Fixed with the
all correspondents.

+ * Returned elements are locked as is the case with dshash_find.  However, the
+ * caller must not release the lock.
+ *
+ * Same as dynanash, the caller may delete returned elements midst of a scan.

I think it's a bad idea to refer to dynahash here. That's just going to
get out of date. Also, code should be documented on its own.

[006]: (Fixed) Understood, fixed as the follows.
Understood, fixed as the follows.

* Returned elements are locked and the caller must not explicitly release
* it.

+ * If consistent is set for dshash_seq_init, the all hash table partitions are
+ * locked in the requested mode (as determined by the exclusive flag) during
+ * the scan.  Otherwise partitions are locked in one-at-a-time way during the
+ * scan.

Yet delete unconditionally retains locks?

[007]: (Not fixed) Yes. If we release the lock on the current partition, hash resize breaks the concurrent scans.
Yes. If we release the lock on the current partition, hash resize
breaks the concurrent scans.

+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+				bool consistent, bool exclusive)
+{

Why does this patch add the consistent mode? There's no users currently?
Without it's not clear that we need a seperate _term function, I think?

[008]: (Fixed) I remember that it is used in early stage of development. I left it for a matter of API completeness but actually it is not used. _term is another matter. We need to release lock and clean up some dshash status if we allow seq scan to be exited before it reaches to the end.
I remember that it is used in early stage of development. I left it
for a matter of API completeness but actually it is not used. _term is
another matter. We need to release lock and clean up some dshash
status if we allow seq scan to be exited before it reaches to the end.

I removed the "consistent" from dshash_seq_init and reverted
dshash_seq_term.

I think we also can get rid of the dshash_delete changes, by instead
adding a dshash_delete_current(dshash_seq_stat *status, void *entry) API
or such.

[009]: (Fixed) I'm not sure about the point of having two interfaces that are hard to distinguish. Maybe dshash_delete_current(dshash_seq_stat *status) is enough(). I also reverted the dshash_delete().
I'm not sure about the point of having two interfaces that are hard to
distinguish. Maybe dshash_delete_current(dshash_seq_stat *status) is
enough(). I also reverted the dshash_delete().

@@ -70,7 +86,6 @@ extern dshash_table *dshash_attach(dsa_area *area,
extern void dshash_detach(dshash_table *hash_table);
extern dshash_table_handle dshash_get_hash_table_handle(dshash_table *hash_table);
extern void dshash_destroy(dshash_table *hash_table);
-
/* Finding, creating, deleting entries. */
extern void *dshash_find(dshash_table *hash_table,
const void *key, bool
exclusive);

There's a number of spurious changes like this.

[010]: (Fixed) I found such isolated line insertion or removal, two in 0001, eight in 0004.
I found such isolated line insertion or removal, two in 0001, eight in
0004.

From 60da67814fe40fd2a0c1870b15dcf6fcb21c989a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 27 Sep 2018 11:15:19 +0900
Subject: [PATCH 2/5] Add conditional lock feature to dshash

Dshash currently waits for lock unconditionally. This commit adds new
interfaces for dshash_find and dshash_find_or_insert. The new
interfaces have an extra parameter "nowait" taht commands not to wait
for lock.

s/taht/that/

[011]: (Fixed) Sounds reasonable. I rewrote it that way.
Applied ispell on all commit messages.

There should be at least a sentence or two explaining why these are
useful.

[011]: (Fixed) Sounds reasonable. I rewrote it that way.
Sounds reasonable. I rewrote it that way.

+/*
+ * The version of dshash_find, which is allowed to return immediately on lock
+ * failure. Lock status is set to *lock_failed in that case.
+ */

Hm. Not sure I like the *lock_acquired API.

+void *
+dshash_find_extended(dshash_table *hash_table, const void *key,
+					 bool exclusive, bool nowait, bool *lock_acquired)

...

+ Assert(nowait || !lock_acquired);

...

+ if (lock_acquired)
+ *lock_acquired = false;

Why is the test for lock_acquired needed here? I don't think it's
possible to use nowait correctly without passing in lock_acquired?

Think it'd make sense to document & assert that nowait = true implies
lock_acquired set, and nowait = false implies lock_acquired not being
set.

But, uh, why do we even need the lock_acquired parameter? If we couldn't
find an entry, then we should just release the lock, no?

[012]: (Fixed) (related to [013], [014]) The name is confusing. In this version the old dshash_find_extended and dshash_find_or_insert_extended are merged into new dshash_find_extended, which covers all the functions of dshash_find and dshash_find_or_insert plus insertion with shared lock is allowed now.
The name is confusing. In this version the old dshash_find_extended
and dshash_find_or_insert_extended are merged into new
dshash_find_extended, which covers all the functions of dshash_find
and dshash_find_or_insert plus insertion with shared lock is allowed
now.

I'm however inclined to think it's better to just have a separate
function for the nowait case, rather than an extended version supporting
both (with an internal helper doing most of the work).

[013]: (Fixed) (related to [012], [014]) After some thoughts, the nowait is no longer a matter of complexity. Finally I did as [012].
After some thoughts, the nowait is no longer a matter of complexity.
Finally I did as [012](Fixed) (related to [013], [014]) The name is confusing. In this version the old dshash_find_extended and dshash_find_or_insert_extended are merged into new dshash_find_extended, which covers all the functions of dshash_find and dshash_find_or_insert plus insertion with shared lock is allowed now..

+/*
+ * The version of dshash_find_or_insert, which is allowed to return immediately
+ * on lock failure.
+ *
+ * Notes above dshash_find_extended() regarding locking and error handling
+ * equally apply here.

They don't, there's no lock_acquired parameter.

+ */
+void *
+dshash_find_or_insert_extended(dshash_table *hash_table,
+							   const void *key,
+							   bool *found,
+							   bool nowait)

I think it's absurd to have dshash_find, dshash_find_extended,
dshash_find_or_insert, dshash_find_or_insert_extended. If they're
extended they should also be able to specify whether the entry will get
created.

[014]: (Fixed) (related to [012], [013]) As mentioned above, this version has the original two functions and one dshash_find_extended().
As mentioned above, this version has the original two functions and
one dshash_find_extended().

From d10c1117cec77a474dbb2cff001086d828b79624 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 7 Nov 2018 16:53:49 +0900
Subject: [PATCH 3/5] Make archiver process an auxiliary process

This is a preliminary patch for shared-memory based stats collector.
Archiver process must be a auxiliary process since it uses shared
memory after stats data wes moved onto shared-memory. Make the process

s/wes/was/ s/onto/into/

[015]: (Fixed)

an auxiliary process in order to make it work.

@@ -451,6 +454,11 @@ AuxiliaryProcessMain(int argc, char *argv[])
StartupProcessMain();
proc_exit(1); /* should never return */

+		case ArchiverProcess:
+			/* don't set signals, archiver has its own agenda */
+			PgArchiverMain();
+			proc_exit(1);		/* should never return */
+
case BgWriterProcess:
/* don't set signals, bgwriter has its own agenda */
BackgroundWriterMain();

I think I'd rather remove the two comments that are copied to 6 out of 8
cases - they don't add anything.

[016]: (Fixed) Agreed. I removed the comments from StartProcess to WalReceiverProcess.
Agreed. I removed the comments from StartProcess to WalReceiverProcess.

pgarch_exit(SIGNAL_ARGS)
{

..

+	 * We DO NOT want to run proc_exit() callbacks -- we're here because
+	 * shared memory may be corrupted, so we don't want to try to clean up our

...

+	 * being doubly sure.)
+	 */
+	exit(2);

...

This seems to be a copy of code & comments from other signal handlers that predates

..

I think this just should use SignalHandlerForCrashExit().
I think we can even commit that separately - there's not really a reason
to not do that today, as far as I can tell?

[017]: (Fixed, separate patch 0001) Exactly. Although on_*_exit_list is empty on the process, SIGQUIT ought to prevent the process from calling the functions even if any. That changes the exit status of archiver on SIGQUIT from 1 to 2, but that doesn't make any behavior change (other than log message).
Exactly. Although on_*_exit_list is empty on the process, SIGQUIT
ought to prevent the process from calling the functions even if
any. That changes the exit status of archiver on SIGQUIT from 1 to 2,
but that doesn't make any behavior change (other than log message).

/* SIGUSR1 signal handler for archiver process */

Hm - this currently doesn't set up a correct sigusr1 handler for a
shared memory backend - needs to invoke procsignal_sigusr1_handler
somewhere.

We can probably just convert to using normal latches here, and remove
the current 'wakened' logic? That'll remove the indirection via
postmaster too, which is nice.

[018]: (Fixed, separate patch 0005) It seems better. I added it as a separate patch just after the patch that turns archiver an auxiliary process.
It seems better. I added it as a separate patch just after the patch
that turns archiver an auxiliary process.

@@ -4273,6 +4276,9 @@ pgstat_get_backend_desc(BackendType backendType)

switch (backendType)
{
+		case B_ARCHIVER:
+			backendDesc = "archiver";
+			break;

should imo include 'WAL' or such.

[019]: (Not Fixed) It is already named "archiver" by 8e8a0becb3. Do I rename it in this patch set?
It is already named "archiver" by 8e8a0becb3. Do I rename it in this
patch set?

From 5079583c447c3172aa0b4f8c0f0a46f6e1512812 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 21 Feb 2019 12:44:56 +0900
Subject: [PATCH 4/5] Shared-memory based stats collector

..

megabytes. To deal with larger statistics set, this patch let backends
directly share the statistics via shared memory.

This spends a fair bit describing the old state, but very little
describing the new state.

[020]: (Fixed, Maybe) Ugg. I got the same comment in the last round. I rewrote it this time.
Ugg. I got the same comment in the last round. I rewrote it this time.

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 0bfd6151c4..a6b0bdec12 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml

...

-   master process.  (The <quote>stats collector</quote> process will not be present
-   if you have set the system not to start the statistics collector; likewise
+   master process.  (The <quote>autovacuum launcher</quote> process will not

...

There's more references to the stats collector than this... E.g. in
catalogs.sgml

[021]: (Fixed, separate patch 0007) However the "statistics collector process" is gone, I'm not sure "statistics collector" feature also is gone. But actually the word "collector" looks a bit odd in some context. I replaced "the results of statistics collector" with "the activity statistics". (I'm not sure "the activity statistics" is proper as a subsystem name.) The word "collect" is replaced with "track". I didn't change section IDs corresponding to the renaming so that old links can work. I also fixed the tranche name for LWTRANCHE_STATS from "activity stats" to "activity_statistics"
However the "statistics collector process" is gone, I'm not sure
"statistics collector" feature also is gone. But actually the word
"collector" looks a bit odd in some context. I replaced "the results
of statistics collector" with "the activity statistics". (I'm not sure
"the activity statistics" is proper as a subsystem name.) The word
"collect" is replaced with "track". I didn't change section IDs
corresponding to the renaming so that old links can work. I also fixed
the tranche name for LWTRANCHE_STATS from "activity stats" to
"activity_statistics"

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 6d1f28c327..8dcb0fb7f7 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c

...

@@ -2747,12 +2747,10 @@ get_pgstat_tabentry_relid(Oid relid, bool isshared, PgStat_StatDBEntry *shared,
if (isshared)
{
if (PointerIsValid(shared))
-			tabentry = hash_search(shared->tables, &relid,
-								   HASH_FIND, NULL);
+			tabentry = pgstat_fetch_stat_tabentry_extended(shared, relid);
}
else if (PointerIsValid(dbentry))
-		tabentry = hash_search(dbentry->tables, &relid,
-							   HASH_FIND, NULL);
+		tabentry = pgstat_fetch_stat_tabentry_extended(dbentry, relid);

return tabentry;
}

Why is pgstat_fetch_stat_tabentry_extended called "_extended"? Outside

[022]: (Fixed) The _extended function is not an extended version of the original function. I renamed pgstat_fetch_stat_tabentry_extended to pgstat_fetch_stat_tabentry_snapshot. Also pgstat_fetch_funcentry_extended and pgstat_fetch_dbentry() are renamed following that.
The _extended function is not an extended version of the original
function. I renamed pgstat_fetch_stat_tabentry_extended to
pgstat_fetch_stat_tabentry_snapshot. Also
pgstat_fetch_funcentry_extended and pgstat_fetch_dbentry() are renamed
following that.

the stats subsystem there are exactly one caller for the non extended
version, as far as I can see. That's index_concurrently_swap() - and imo
that's code that should live in the stats subsystem, rather than open
coded in index.c.

[023]: (Fixed) Agreed. I added a new function "pgstat_copy_index_counters()" and now pgstat_fetch_stat_tabentry() has no caller sites outside pgstat subsystem.
Agreed. I added a new function "pgstat_copy_index_counters()" and now
pgstat_fetch_stat_tabentry() has no caller sites outside pgstat
subsystem.

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ca5c6376e5..1ffe073a1f 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
+ *  Collects per-table and per-function usage statistics of all backends on
+ *  shared memory. pg_count_*() and friends are the interface to locally store
+ *  backend activities during a transaction. Then pgstat_flush_stat() is called
+ *  at the end of a transaction to pulish the local stats on shared memory.
*

I'd rather not exhaustively list the different objects this handles -
it'll either be annoying to maintain, or just get out of date.

[024]: (Fixed, Maybe) Although not sure I get you correctly, I rewrote it as the follows.
Although not sure I get you correctly, I rewrote it as the follows.

* Collects per-table and per-function usage statistics of all backends on
* shared memory. The activity numbers are once stored locally, then written
* to shared memory at commit time or by idle-timeout.

- *			- Add some automatic call for pgstat vacuuming.
+ *  To avoid congestion on the shared memory, we update shared stats no more
+ *  often than intervals of PGSTAT_STAT_MIN_INTERVAL(500ms). In the case where
+ *  all the local numbers cannot be flushed immediately, we postpone updates
+ *  and try the next chance after the interval of
+ *  PGSTAT_STAT_RETRY_INTERVAL(100ms), but we don't wait for no longer than
+ *  PGSTAT_STAT_MAX_INTERVAL(1000ms).

I'm not convinced by this backoff logic. The basic interval seems quite
high for something going through shared memory, and the max retry seems
pretty low.

[025]: (Not Fixed) Is it the matter of intervals? Is (MIN, RETRY, MAX) = (1000, 500, 10000) reasonable?
Is it the matter of intervals? Is (MIN, RETRY, MAX) = (1000, 500,
10000) reasonable?

+/*
+ * Operation mode and return code of pgstat_get_db_entry.
+ */
+#define	PGSTAT_SHARED		0

This is unreferenced.

+#define PGSTAT_EXCLUSIVE 1
+#define PGSTAT_NOWAIT 2

And these should imo rather be parameters.

[026]: (Fixed) Mmm. Right. The two symbols conveys just two distinct parameters. Two booleans suffice. But I found some confusion here. As the result pgstat_get_db_entry have three booleans parameters exclusive, nowait and create.
Mmm. Right. The two symbols conveys just two distinct parameters. Two
booleans suffice. But I found some confusion here. As the result
pgstat_get_db_entry have three booleans parameters exclusive, nowait
and create.

+typedef enum PgStat_TableLookupResult
+{
+	NOT_FOUND,
+	FOUND,
+	LOCK_FAILED
+} PgStat_TableLookupResult;

This seems like a seriously bad idea to me. These are very generic
names. There's also basically no references except setting them to the
first two?

[027]: (Fixed) Considering some related comments above, I decided not to return lock status from pgstat_get_db_entry. That makes the enum useless and makes the function simpler.
Considering some related comments above, I decided not to return lock
status from pgstat_get_db_entry. That makes the enum useless and makes
the function simpler.

+#define StatsLock (&StatsShmem->StatsMainLock)

-static time_t last_pgstat_start_time;
+/* Shared stats bootstrap information */
+typedef struct StatsShmemStruct
+{
+	LWLock				StatsMainLock;		/* lock to protect this struct */

...

+} StatsShmemStruct;

Why isn't this an lwlock in lwlock in lwlocknames.h, rather than
allocated here?

[028]: (Fixed) The activity stats system already used a dedicate tranche, so I might think that it is natural that it is in the same tranche. Actually it's not so firm reason. Moved the lock into main tranche.
The activity stats system already used a dedicate tranche, so I might
think that it is natural that it is in the same tranche. Actually it's
not so firm reason. Moved the lock into main tranche.

+/*
+ * BgWriter global statistics counters. The name cntains a remnant from the
+ * time when the stats collector was a dedicate process, which used sockets to
+ * send it.
+ */
+PgStat_MsgBgWriter BgWriterStats = {0};

I am strongly against keeping the 'Msg' prefix. That seems extremely
confusing going forward.

[029]: (Fixed) (Related to [046]) Mmm. It's following your old suggestion to avoid unsubstantial diffs. I'm happy to change it. The functions that have "send" in their names are for the same reason. I removed the prefix "m_" of the members of the struct. (The comment above (with a typo) explains that).
Mmm. It's following your old suggestion to avoid unsubstantial
diffs. I'm happy to change it. The functions that have "send" in their
names are for the same reason. I removed the prefix "m_" of the
members of the struct. (The comment above (with a typo) explains that).

+/* common header of snapshot entry in reader snapshot hash */
+typedef struct PgStat_snapshot
+{
+	Oid		key;
+	bool	negative;
+	void   *body;				/* end of header part: to keep alignment */
+} PgStat_snapshot;

+/* context struct for snapshot_statentry */
+typedef struct pgstat_snapshot_param
+{
+	char		   *hash_name;			/* name of the snapshot hash */
+	int				hash_entsize;		/* element size of hash entry */
+	dshash_table_handle	dsh_handle;		/* dsh handle to attach */
+	const dshash_parameters *dsh_params;/* dshash params */
+	HTAB		  **hash;				/* points to variable to hold hash */
+	dshash_table  **dshash;				/* ditto for dshash */
+} pgstat_snapshot_param;

Why does this exist? The struct contents are actually constant across
calls, yet you have declared them inside functions (as static - static
on function scope isn't really the same as global static).

[030]: (Fixed) IIUC, I didn't want it initialized at every call and it doesn't need an external linkage. So it was static variable on function scope. But, first, the name _param is bogus since it actually contains context variables. Second the "context" variables have been moved to other variables. I removed the struct and moved the members to the parameter of snapshot_statentry.
IIUC, I didn't want it initialized at every call and it doesn't need
an external linkage. So it was static variable on function scope.
But, first, the name _param is bogus since it actually contains
context variables. Second the "context" variables have been moved to
other variables. I removed the struct and moved the members to the
parameter of snapshot_statentry.

If we want it, I think we should separate the naming more
meaningfully. The important difference between 'hash' and 'dshash' isn't
the hashing module, it's that one is a local copy, the other a shared
hashtable!

[031]: (Fixed) Definitely. The parameters of snapshot_statentry now have more meaningful names.
Definitely. The parameters of snapshot_statentry now have more
meaningful names.

+/*
+ * Backends store various database-wide info that's waiting to be flushed out
+ * to shared memory in these variables.
+ *
+ * checksum_failures is the exception in that it is cluster-wide value.
+ */
+typedef struct BackendDBStats
+{
+	int		n_conflict_tablespace;
+	int		n_conflict_lock;
+	int		n_conflict_snapshot;
+	int		n_conflict_bufferpin;
+	int		n_conflict_startup_deadlock;
+	int		n_deadlocks;
+	size_t	n_tmpfiles;
+	size_t	tmpfilesize;
+	HTAB	*checksum_failures;
+} BackendDBStats;

Why is this a separate struct from PgStat_StatDBEntry? We should have
these fields in multiple places.

[032]: (Fixed, Maybe) (Related to [042]) It is almost a subset of PgStat_StatDBEntry with an exception. checksum_failures is different between the two. Anyway, tracking of conflict events don't need to be so fast so they have been changed to be counted on shared hash entries directly.
It is almost a subset of PgStat_StatDBEntry with an
exception. checksum_failures is different between the two.
Anyway, tracking of conflict events don't need to be so fast so they
have been changed to be counted on shared hash entries directly.

Checkpoint failure is handled different way so only it is left alone.

+ if (StatsShmem->refcount > 0)
+ StatsShmem->refcount++;

What prevents us from leaking the refcount here? We could e.g. error out
while attaching, no? Which'd mean we'd leak the refcount.

[033]: (Fixed) We don't attach shared stats on postmaster process, so I want to know the first attacher process and the last detacher process of shared stats. It's not leaks that I'm considering here. (continued below)
We don't attach shared stats on postmaster process, so I want to know
the first attacher process and the last detacher process of shared
stats. It's not leaks that I'm considering here.
(continued below)

To me it looks like there's a lot of added complexity just because you
want to be able to reset stats via

void
pgstat_reset_all(void)
{

/*
* We could directly remove files and recreate the shared memory area. But
* detach then attach for simplicity.
*/
pgstat_detach_shared_stats(false); /* Don't write */
pgstat_attach_shared_stats();

Without that you'd not need the complexity of attaching, detaching to
the same degree - every backend could just cache lookup data during
initialization, instead of having to constantly re-compute that.

Mmm. I don't get that (or I failed to read clear meaning). The
function is assumed be called only from StartupXLOG().
(continued)

Nor would the dynamic re-creation of the db dshash table be needed.

Maybe you are mentioning the complexity of reset_dbentry_counters? It
is actually complex. Shared stats dshash cannot be destroyed (or
dshash entry cannot be removed) during someone is working on it. It
was simpler to wait for another process to end its work but that could
slow not only the clearing process but also other processes by
frequent resetting of counters.

After some thoughts, I decided to rip the all "generation" stuff off
and it gets far simpler. But counter reset may conflict with other
backends with a litter higher degree because counter reset needs
exclusive lock.

+/* ----------
+ * pgstat_report_stat() -
+ *
+ *	Must be called by processes that performs DML: tcop/postgres.c, logical
+ *	receiver processes, SPI worker, etc. to apply the so far collected
+ *	per-table and function usage statistics to the shared statistics hashes.
+ *
+ *  Updates are applied not more frequent than the interval of
+ *  PGSTAT_STAT_MIN_INTERVAL milliseconds. They are also postponed on lock

...

Inconsistent indentation.

[034]: (Fixed)

+long
+pgstat_report_stat(bool force)
{

+	/* Flush out table stats */
+	if (pgStatTabList != NULL && !pgstat_flush_stat(&cxt, !force))
+		pending_stats = true;
+
+	/* Flush out function stats */
+	if (pgStatFunctions != NULL && !pgstat_flush_funcstats(&cxt, !force))
+		pending_stats = true;

This seems weird. pgstat_flush_stat(), pgstat_flush_funcstats() operate
on pgStatTabList/pgStatFunctions, but don't actually reset it? Besides
being confusing while reading the code, it also made the diff much
harder to read.

[035]: (Maybe Fixed) Is the question that, is there any case where pgstat_flush_stat/functions leaves some counters unflushed? It skips tables where someone else is working on (or another table that is in the same dshash partition), Or "!force == nowait" is the cause of confusion? It is now translated as "nowait = !force". (Or change the parameter of pgstat_report_stat from "force" to "nowait"?)
Is the question that, is there any case where
pgstat_flush_stat/functions leaves some counters unflushed? It skips
tables where someone else is working on (or another table that is in
the same dshash partition), Or "!force == nowait" is the cause of
confusion? It is now translated as "nowait = !force". (Or change the
parameter of pgstat_report_stat from "force" to "nowait"?)

-		snprintf(fname, sizeof(fname), "%s/%s", directory,
-				 entry->d_name);
-		unlink(fname);
+	/* Flush out database-wide stats */
+	if (HAVE_PENDING_DBSTATS())
+	{
+		if (!pgstat_flush_dbstats(&cxt, !force))
+			pending_stats = true;
}

Linearly checking a number of stats doesn't seem like the right way
going forward. Also seems fairly omission prone.

Why does this code check live in pgstat_report_stat(), rather than
pgstat_flush_dbstats()?

[036]: (Maybe Fixed) (Related to [041]) It's to avoid useless calls but it is no longer exists. Anyway the code disappeared by [041].
It's to avoid useless calls but it is no longer exists. Anyway the
code disappeared by [041](Fixed) (Related to [036]) Completely agree, It is a result of that I wanted to avoid scanning pgStatTables twice. (continued).

| /* Flush out individual stats tables */
| pending_stats |= pgstat_flush_stat(&cxt, nowait);
| pending_stats |= pgstat_flush_funcstats(&cxt, nowait);
| pending_stats |= pgstat_flush_checksum_failure(cxt.mydbentry, nowait);

/*
* snapshot_statentry() - Common routine for functions
* pgstat_fetch_stat_*entry()
*

Why has this function been added between the closely linked
pgstat_report_stat() and pgstat_flush_stat() etc?

[037]: It seems to be left there after some editing. Moved it to just before the caller functdions.
It seems to be left there after some editing. Moved it to just before
the caller functdions.

Why do we still have this? A hashtable lookup is cheap, compared to
fetching a file - so it's not to save time. Given how infrequent the
pgstat_fetch_* calls are, it's not to avoid contention either.

At first one could think it's for consistency - but no, that's not it
either, because snapshot_statentry() refetches the snapshot without
control from the outside:

[038]: I don't get the second paragraph. When the function re*create*s a snapshot without control from the outside? It keeps snapshots during a transaction. If not, it is broken. (continued)
I don't get the second paragraph. When the function re*create*s a
snapshot without control from the outside? It keeps snapshots during a
transaction. If not, it is broken.
(continued)

/*
* We don't want so frequent update of stats snapshot. Keep it at least
* for PGSTAT_STAT_MIN_INTERVAL ms. Not postpone but just ignore the cue.
*/

...

I think we should just remove this entire local caching snapshot layer
for lookups.

Currently the behavior is documented as the follows and it seems reasonable.

Another important point is that when a server process is asked to display
any of these statistics, it first fetches the most recent report emitted by
the collector process and then continues to use this snapshot for all
statistical views and functions until the end of its current transaction.
So the statistics will show static information as long as you continue the
current transaction. Similarly, information about the current queries of
all sessions is collected when any such information is first requested
within a transaction, and the same information will be displayed throughout
the transaction.
This is a feature, not a bug, because it allows you to perform several
queries on the statistics and correlate the results without worrying that
the numbers are changing underneath you. But if you want to see new
results with each query, be sure to do the queries outside any transaction
block. Alternatively, you can invoke
<function>pg_stat_clear_snapshot</function>(), which will discard the
current transaction's statistics snapshot (if any). The next use of
statistical information will cause a new snapshot to be fetched.

/*
* pgstat_flush_stat: Flushes table stats out to shared statistics.
*

Why is this named pgstat_flush_stat, rather than pgstat_flush_tabstats
or such? Given that the code for dealing with an individual table's
entry is named pgstat_flush_tabstat() that's very confusing.

[039]: The names are hchanged while adressing [041]
Definitely. The names are hchanged while adressing [041](Fixed) (Related to [036]) Completely agree, It is a result of that I wanted to avoid scanning pgStatTables twice. (continued)

static bool
pgstat_flush_stat(pgstat_flush_stat_context *cxt, bool nowait)

...

It seems odd that there's a tabstat specific code in pgstat_flush_stat
(also note singular while it's processing all stats, whereas you're
below treating pgstat_flush_tabstat as only affecting one table).

[039]: The names are hchanged while adressing [041]
The names are hchanged while adressing [041](Fixed) (Related to [036]) Completely agree, It is a result of that I wanted to avoid scanning pgStatTables twice. (continued)

for (i = 0; i < tsa->tsa_used; i++)
{
PgStat_TableStatus *entry = &tsa->tsa_entries[i];

<many TableStatsArray code>

hash_entry->tsa_entry = entry;
dest_elem++;
}

This seems like too much code. Why is this entirely different from the
way funcstats works? The difference was already too big before, but this
made it *way* worse.

[040]: Maybe you are insisting the reverse? The pin_hash complexity is left in this version. -> [033]
We don't flush stats until transaction ends. So the description about
TabStatuArray is stale?

* NOTE: once allocated, TabStatusArray structures are never moved or deleted
* for the life of the backend. Also, we zero out the t_id fields of the
* contained PgStat_TableStatus structs whenever they are not actively in use.
* This allows relcache pgstat_info pointers to be treated as long-lived data,
* avoiding repeated searches in pgstat_initstats() when a relation is
* repeatedly opened during a transaction.
(continued to below)

One goal of this project, as I understand it, is to make it easier to
add additional stats. As is, this seems to make it harder from the code
level.

Indeed. I removed the TabStatsArray. Having said that it lives a long
life, its life lasts for at most transaction end. I used dynahash
entry as pgstat_info entry. One tricky part is I had to clear
entry->t_id after removal of the entry so that pgstat_initstats can
detect the removal. It is actually safe but we can add another table
id member in the struct for the use.

bool
pgstat_flush_tabstat(pgstat_flush_stat_context *cxt, bool nowait,
PgStat_TableStatus *entry)
{
Oid dboid = entry->t_shared ? InvalidOid : MyDatabaseId;
int table_mode = PGSTAT_EXCLUSIVE;
bool updated = false;
dshash_table *tabhash;
PgStat_StatDBEntry *dbent;
int generation;

if (nowait)
table_mode |= PGSTAT_NOWAIT;

/* Attach required table hash if not yet. */
if ((entry->t_shared ? cxt->shdb_tabhash : cxt->mydb_tabhash) == NULL)
{
/*
* Return if we don't have corresponding dbentry. It would've been
* removed.
*/
dbent = pgstat_get_db_entry(dboid, table_mode, NULL);
if (!dbent)
return false;

/*
* We don't hold lock on the dbentry since it cannot be dropped while
* we are working on it.
*/
generation = pin_hashes(dbent);
tabhash = attach_table_hash(dbent, generation);

This again is just cost incurred by insisting on destroying hashtables
instead of keeping them around as long as necessary.

[040]: Maybe you are insisting the reverse? The pin_hash complexity is left in this version. -> [033]
Maybe you are insisting the reverse? The pin_hash complexity is left
in this version. -> [033](Fixed) We don't attach shared stats on postmaster process, so I want to know the first attacher process and the last detacher process of shared stats. It's not leaks that I'm considering here. (continued below)

/*
* Local table stats should be applied to both dbentry and tabentry at
* once. Update dbentry only if we could update tabentry.
*/
if (pgstat_update_tabentry(tabhash, entry, nowait))
{
pgstat_update_dbentry(dbent, entry);
updated = true;
}

At this point we're very deeply nested. pgstat_report_stat() ->
pgstat_flush_stat() -> pgstat_flush_tabstat() ->
pgstat_update_tabentry().

That's way over the top imo.

[041]: (Fixed) (Related to [036]) Completely agree, It is a result of that I wanted to avoid scanning pgStatTables twice. (continued)
Completely agree, It is a result of that I wanted to avoid scanning
pgStatTables twice.
(continued)

I don't think it makes much sense that pgstat_update_dbentry() is called
separately for each table. Why would we want to constantly lock that
entry? It seems to be much more sensible to instead have
pgstat_flush_stat() transfer the stats it reported to the pending
database wide counters, and then report that to shared memory *once* per
pgstat_report_stat() with pgstat_flush_dbstats()?

In the attched it scans PgStat_StatDBEntry twice. Once for the tables
of current database and another for shared tables. That change
simplified the logic around.

pgstat_report_stat()
pgstat_flush_tabstats(<tables of current dataase>)
pgstat_update_tabentry() (at bottom)

LWLockAcquire(&cxt->mydbentry->lock, LW_EXCLUSIVE);
if (HAVE_PENDING_CONFLICTS())
pgstat_flush_recovery_conflict(cxt->mydbentry);
if (BeDBStats.n_deadlocks != 0)
pgstat_flush_deadlock(cxt->mydbentry);

..

What's the point of having all these sub-functions? I see that you, for
an undocumented reason, have pgstat_report_recovery_conflict() flush
conflict stats immediately:

[042]: Fixed by [032].
Fixed by [032](Fixed, Maybe) (Related to [042]) It is almost a subset of PgStat_StatDBEntry with an exception. checksum_failures is different between the two. Anyway, tracking of conflict events don't need to be so fast so they have been changed to be counted on shared hash entries directly..

dbentry = pgstat_get_db_entry(MyDatabaseId,
PGSTAT_EXCLUSIVE | PGSTAT_NOWAIT,
&status);

if (status == LOCK_FAILED)
return;

/* We had a chance to flush immediately */
pgstat_flush_recovery_conflict(dbentry);

dshash_release_lock(pgStatDBHash, dbentry);

But I don't understand why? Nor why we'd not just report all pending
database wide changes in that case?

The fact that you're locking the per-database entry unconditionally once
for each table almost guarantees contention - and you're not using the
'conditional lock' approach for that. I don't understand.

[043]: (Maybe fixed) (Related to [045].) Vacuum, analyze, DROP DB and reset cannot be delayed. So the conditional lock is mainly used by pgstat_report_stat(). dshash_find_or_insert didn't allow shared lock. I changed dshash_find_extended to allow shared-lock even if it is told to create a missing entry. Alrhough it takes exclusive lock at the mement of entry creation, most of all cases it doesn't need exclusive lock. This allows use shared lock while processing vacuum or analyze stats.
Vacuum, analyze, DROP DB and reset cannot be delayed. So the
conditional lock is mainly used by
pgstat_report_stat(). dshash_find_or_insert didn't allow shared
lock. I changed dshash_find_extended to allow shared-lock even if it
is told to create a missing entry. Alrhough it takes exclusive lock at
the mement of entry creation, most of all cases it doesn't need
exclusive lock. This allows use shared lock while processing vacuum or
analyze stats.

Previously I thought that we can work on a shared database entry while
lock is not held, but actually there are cases where insertion of a
new database entry causes rehash (resize). The operation moves entries
so we need at least shared lock on database entry while we are working
on it. So in the attched basically most operations are working by the
following steps.

- get shared database entry with shared lock
- attach table/function hash
- fetch an entry with exclusive lock
- update entry
- release the table/function entry
- detach table/function hash

if needed
- take LW_EXCLUSIVE on database entry
- update database numbers
- release LWLock
- release shared database entry

pgstat_vacuum_stat(void)
{

...

dshash_seq_init(&dshstat, pgStatDBHash, false, true);
while ((dbentry = (PgStat_StatDBEntry *) dshash_seq_next(&dshstat)) != NULL)
pgstat_drop_database(dbid);

..

So, uh, pgstat_drop_database() again does a *separate* lookup in the
dshash, locking the entry. Which only works because you added this dirty
hack:

/* We need to keep partition lock while sequential scan */
if (!hash_table->seqscan_running)
{
hash_table->find_locked = false;
hash_table->find_exclusively_locked = false;
LWLockRelease(PARTITION_LOCK(hash_table, partition));
}

to dshash_delete_entry(). This seems insane to me. There's not even a
comment explaining this?

[044]: Following [001] and [009], I added dshash_delete_currenet(). pgstat_vacuum_stat() uses it instead of pgstat_delete_entry(). The hack is gone.

Following [001](Fixed) As the result of the fixed in [044], it's gone now. and [009](Fixed) I'm not sure about the point of having two interfaces that are hard to distinguish. Maybe dshash_delete_current(dshash_seq_stat *status) is enough(). I also reverted the dshash_delete()., I added
dshash_delete_currenet(). pgstat_vacuum_stat() uses it instead of
pgstat_delete_entry(). The hack is gone.

(pgstat_vacuum_stat(void))

}
dshash_release_lock(pgStatDBHash, dbentry);

Wait, why are we holding the database partition lock across all this?
Again without any comments explaining why?

[045]: (I'm not sure it is fixed) The lock is shared lock in the current version. The database entry is needed only for attaching table hash and now the hashes won't be removed. So, as maybe you suggested, the lock can be released earlier in:
The lock is shared lock in the current version. The database entry is
needed only for attaching table hash and now the hashes won't be
removed. So, as maybe you suggested, the lock can be released earlier in:

pgstat_report_stat()
pgstat_flush_funcstats()
pgstat_vacuum_stat()
pgstat_reset_single_counter()
pgstat_report_vacuum()
pgstat_report_analyze()

The following functions are working on the database entry so lock needs to be retained till the end of its work.

pgstat_flush_dbstats()
pgstat_drop_database() /* needs exclusive lock */
pgstat_reset_counters()
pgstat_report_autovac()
pgstat_report_recovery_conflict()
pgstat_report_deadlock()
pgstat_report_tempfile()
pgstat_report_checksum_failures_in_db()
pgstat_flush_checksum_failure() /* repeats short-time lock on each dbs */

+void
+pgstat_send_archiver(const char *xlog, bool failed)

Why do we still have functions named pgstat_send*?

[046]: (Fixed) Same as [029] and I changed it to pgstat_report_archiver().
Same as [029](Fixed) (Related to [046]) Mmm. It's following your old suggestion to avoid unsubstantial diffs. I'm happy to change it. The functions that have "send" in their names are for the same reason. I removed the prefix "m_" of the members of the struct. (The comment above (with a typo) explains that). and I changed it to pgstat_report_archiver().

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center