From: Antonin Houska <ah@cybertec.at>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
cc: andres@anarazel.de, magnus@hagander.net, robertmhaas@gmail.com,
    tgl@sss.pgh.pa.us, pgsql-hackers@postgresql.org
Subject: Re: shared-memory based stats collector
In-reply-to: <20180710.210740.66209890.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20180706185750.b6h5cwif53zfieu7@alap3.anarazel.de>
 <CABUevEzRR0O7iR-_9tZGUOw0+1r4vmVVE-_frT01Kk+ithgftA@mail.gmail.com>
 <20180706201036.awheoi6tk556x6aj@alap3.anarazel.de>
 <20180710.210740.66209890.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Tue, 10 Jul 2018 21:07:40 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Thu, 20 Sep 2018 09:55:27 +0200
Message-ID: <11936.1537430127@linux-at0a>
Precedence: bulk

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 tabl=
es
   themselves, but that might make them wait if the same entries are access=
ed
   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 collec=
tor
   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" a=
nd
   leaves the containing hash table partition locked *exclusively* even if =
it
   changes only the containing table entries, while changes of the containi=
ng
   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 sha=
red
   mode: it may need a loop in which we release the exclusive lock and acqu=
ire
   the shared lock unless the entry was already removed.)

3. Data in both shared_archiverStats and shared_globalStats is mostly acces=
sed
   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 ca=
lls
  pgstat_attach_shared_stats(), but the latter takes care of the context
  itself.

* pgstat_attach_shared_stats() - header comment should explain what the ret=
urn
  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 ..." -> "Re=
turns a local hash containing ..."


[1] https://www.postgresql.org/message-id/20180711000605.sqjik3vqe5opqz33@a=
lap3.anarazel.de

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at