Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 17 Jul 2024 08:08:48 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Joe Conway <mail@joeconway.com>, Gurjeet Singh <gurjeet@singh.im>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: RFC: pg_stat_logmsg
Message-ID: <Zpb9gBjB2FfiD8jO@paquier.xyz>
References: <89742024-d51a-c66b-90b9-67f837072cd2@joeconway.com>
 <CAD21AoAyPVYZ_6VAQCOanvHTL3CRSqTDCFrtvaYVGwOQbrnxgQ@mail.gmail.com>
 <CABwTF4VzMJaTLrpYtBj4n9r73TFjffzqpPFeUmJPrkY6cmHEAg@mail.gmail.com>
 <3b1683bc-2630-a0d0-6083-8a45aa1b54bf@joeconway.com>
 <cda17e6d-f625-b45e-dfb9-8d18ed09ecc5@joeconway.com>
 <5dc12198-80ff-4e70-b187-11ef33418411@enterprisedb.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="fDAp1UbnQIDQnBsB"
Content-Disposition: inline
In-Reply-To: <5dc12198-80ff-4e70-b187-11ef33418411@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/Zpb9gBjB2FfiD8jO%40paquier.xyz>
Precedence: bulk


--fDAp1UbnQIDQnBsB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jul 17, 2024 at 12:14:36AM +0200, Tomas Vondra wrote:
> I noticed this patch hasn't moved since September 2023, so I wonder
> what's the main blocker / what is needed to move this?

+ /* Location of permanent stats file (valid when database is shut down) */
+ #define PGLM_DUMP_FILE	PGSTAT_STAT_PERMANENT_DIRECTORY
"/pg_stat_logmsg.stat

Perhaps this does not count as a valid reason, but does it really make
sense to implement things this way, knowing that this could be changed
to rely on a potential pluggable pgstats?  I mean this one I've
proposed:
https://www.postgresql.org/message-id/Zmqm9j5EO0I4W8dx%40paquier.xyz

One potential implementation is to stick that to be fixed-numbered,
because there is a maximum cap to the number of entries proposed by
the patch, while keeping the whole in memory.

+ logmsg_store(ErrorData *edata, Size *logmsg_offset,
+ 			 int *logmsg_len, int *gc_count)

The patch shares a lot of perks with pg_stat_statements that don't
scale well.  I'm wondering if it is a good idea to duplicate these
properties in a second, different, module, like the external file can
could be written out on a periodic basis depending on the workload.
I am not saying that the other thread is a magic solution for
everything (not looked yet at how this would plug with the cap in
entries that pg_stat_statements wants), just one option on the table.

> As for the code, I wonder if the instability of line numbers could be a
> problem - these can change (a little bit) between minor releases, so
> after an upgrade we'll read the dump file with line numbers from the old
> release, and then start adding entries with new line numbers. Do we need
> to handle this in some way?

Indeed.  Perhaps a PostgreSQL version number assigned to each entry to
know from which binary an entry comes from?  This would cost a couple
of extra bytes for each entry still that would be the best information
possible to match that with a correct code tree.  If it comes to that,
even getting down to a commit SHA1 could be useful to provide the
lowest level of granularity.  Another thing would be to give up on the
line number, stick to the uniqueness in the stats entries with the
errcode and the file name, but that won't help for things like
tablecmds.c.

> This might be partially solved by eviction of entries from the old
> release - we apply decay, so after a while their usage will be 0. But
> what if there's no pressure for space, we'll not actually evict them.
> And it'll be confusing to have a mix of old/new line numbers.

Once we know that these stats not going to be relevant anymore as of a
minor upgrade flow, resetting them could be the move that makes the
most sense, leaving the reset to the provider doing the upgrades,
while taking a snapshot of the past data before the reset?  I find the
whole problem tricky to define, TBH.
--
Michael

--fDAp1UbnQIDQnBsB
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmaW/YAACgkQnvQgOdby
QH2jkA//dfurNFRsA7temfMVEv71BcrOcwWjYw0iMoUnqiK44yoWp37MjOOob0BW
ArVKTt8hq+MdyQPO2UcJDnr3BAattkrOQUzveDsklwUVc2t+uhGU9TTIiBVVSAch
t9Bc8nvPy1lChmFlSr5rGDlmMf1ebT1v9wz6O88NtwVvQxyFhkchMMNeFt5hJBu6
2Z1GAosel0MSMYW87Fko1vZW6/dCmi3CYsXiWETmlsiQXJLFjEruV2dtzK75YDdK
4KLUifmYuIVAgL0xg7PIv3mJ7I4m+jA5LjNHFuW9etZVJbctj9R5aq5COFy6vh0e
2+Fwjs5jfXc9N6XxQ4jz6SiycsxVqJIX1ZVKJljiEQ6REp6slXUqhrqESFFTnlNy
6mHZoWQ2i0winmBxArhVFzabC8ZBSuKMnRucqDKEJcT2nG+NW2dqc4dz1YvslCRB
SMmZvlG3tQlT38XKdHMbF7fzOeXk0DW3IjYrNxkkih1ZXz4utzI0QSY+1eKKDTNa
BbnMAWs0C4mBWi+bFJQtUDc7J+uYm8zW5NYjvYJn/Cu+1S+KGw+iJlyyJP12nvKs
xfO9acbOVoFNxEn/wQ+adaMkmBbI93XWPg2YgHfkSsCqiGcJeGvy058II+p7L/9k
MsEyGaAUSzXjgc2NPGk2OYrFjU+0nP+o6BlgaT7rh4Kr0Aa+LnY=
=e+cO
-----END PGP SIGNATURE-----

--fDAp1UbnQIDQnBsB--