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: <20181005.173017.151015949.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20180926.095509.182252925.horiguchi.kyotaro@lab.ntt.co.jp>
 <20180927.220049.168546206.horiguchi.kyotaro@lab.ntt.co.jp>
 <20181002.160651.117284090.horiguchi.kyotaro@lab.ntt.co.jp>
 <20181005.173017.151015949.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Fri, 05 Oct 2018 17:30:17 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Mon, 29 Oct 2018 15:10:10 +0100
Message-ID: <28855.1540822210@localhost>
Precedence: bulk

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.
>=20
> - stats collector has been removed.
>=20
> - modified dshash further so that deletion is allowed during
>   sequential scan.
>=20
> - I'm not sure about the following existing comment at the
>   beginning of pgstat.c
>=20
>   *	- 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 c=
ode
  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].

* if dshash_seq_init() is passed consistent=3Dfalse, 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 release=
d,
  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() se=
ems
  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=3Dtrue 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=3Dfalse to dshash_seq_init(), so the number of entries can cha=
nge
  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 individ=
ual
  entries.


[1] https://www.postgresql.org/message-id/CA+TgmobQVbz4K_+RSmiM9HeRKpy3vS5x=
nbkL95gSEnWijzprKQ@mail.gmail.com

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