Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 18 Mar 2024 08:02:24 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Autogenerate some wait events code and documentation
Message-ID: <Zfd2gKf00dfxwi4d@paquier.xyz>
References: <ZGM1U5D5EuBUbasi@paquier.xyz>
 <976f4eb1-cbc0-9616-ec26-ebb4a252315c@gmail.com>
 <ZGSM8oOpXsXrXy/v@paquier.xyz>
 <69588eaf-02d7-95a8-0da4-3e120de16a59@gmail.com>
 <ZGWrTn//v+WAqMpN@paquier.xyz>
 <ZKJxZrnFTxoCx8a5@paquier.xyz>
 <ZKJ0tzXSH1LyqI9V@paquier.xyz>
 <0b3cdac7-f078-61a6-76e0-99db7aae78ce@gmail.com>
 <ZKTN//NfDeP1egLW@paquier.xyz>
 <20240317183114.16@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="VczgSuiIbYBCIHHr"
Content-Disposition: inline
In-Reply-To: <20240317183114.16@rfd.leadboat.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Zfd2gKf00dfxwi4d%40paquier.xyz>
Precedence: bulk


--VczgSuiIbYBCIHHr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 17, 2024 at 11:31:14AM -0700, Noah Misch wrote:
> I like the new developer experience of adding a wait event.  After releas=
e of
> v17, how should we approach back-patching an event, like was done in comm=
its
> 8fa4a1a 1396b5c 78c0f85?  Each of those commits put the new event at the =
end
> of its released-branch wait_event.h enum.  In v17,
> generate-wait_event_types.pl sorts events to position them.

Indeed, that would be a bad idea.

> Adding an event
> will renumber others, which can make an extension report the wrong event =
until
> recompiled.  Extensions citus, pg_bulkload, and vector refer to static ev=
ents.
> If a back-patch added WAIT_EVENT_MESSAGE_QUEUE_SOMETHING_NEW, an old-build
> pg_bulkload report of WAIT_EVENT_PARALLEL_CREATE_INDEX_SCAN would show up=
 in
> pg_stat_activity as WAIT_EVENT_PARALLEL_BITMAP_SCAN.  (WAIT_EVENT_EXTENSI=
ON is
> not part of a generated enum, fortunately.)  Some options:
>=20
> 1. Don't back-patch wait events to v17+.  Use the closest existing event.
> 2. Let wait_event_names.txt back-patches control the enum order.  For exa=
mple,
>    a line could have an annotation that controls its position relative to=
 the
>    auto-sorted lines.  For another example, the generator could stop sort=
ing.
> 3. Accept the renumbering, because the consequence isn't that horrible.
>=20
> Option (3) is worse than (1), but I don't have a recommendation between (=
1)
> and (2).  I tend to like (1), a concern being the ease of accidental
> violations.  If we had the ABI compliance checking that
> https://postgr.es/m/flat/CAH2-Wzk7tvgLXzOZ8a22aF-gmO5gHojWTYRvAk5ZgOvTrcE=
Qeg@mail.gmail.com
> explored, (1) would be plenty safe.  Should anything change here, or not?

(1) would be annoying, we have backpatched scaling problems in the
past even if that does not happen often.  And in some cases I can
understand why one would want to add a new wait event to track that
a patch does what is expected of it.  (2) to stop the automated
sorting would bring back the problems that this thread has spent time
to solve: people tend to not add wait events correctly, so I would
suspect issues on HEAD.  I've seen that too many times on older
branches.

I see an option (4), similar to your (2) without the per-line
annotation: add a new magic keyword like the existing "Section" that
is used in the first lines of generate-wait_event_types.pl where we
generate tab-separated lines with the section name as prefix of each
line.  So I can think of something like:
Section: ClassName - WaitEventFoo
FOO_1	"Waiting in foo1"
FOO_2	"Waiting in foo2"
Backpatch:
BAR_1	"Waiting in bar1"
BAR_2	"Waiting in bar2"

Then force the ordering for the docs and keep the elements in the
backpatch section at the end of the enums in the order in the txt.
One thing that could make sense is to enforce that "Backpatch" is at
the end of a section, meaning that we would need a second keyword like
a "Section: EndBackpatch" or something like that.  That's not strictly
necessary IMO as the format of the txt is easy to follow.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmX3doAACgkQnvQgOdby
QH2S7Q//f8OHyetGui5Gkze02ryW+RHl6IxwWR0FvgenKi/Y8YqmG8FjWYsZQ5U9
LuIPGzWBsmYsPphBFT41w/t1N+qKaZlfTR4o1niNKGjLf7rv2f3sUFFIG5ZKRL02
bFkVtvgMdE7gb9aI4yUggJIx7oUA5ez2lWOQg1Nns3MMRNE9BtfBT8G9osecvSwB
KG5iz5JDaNKmGj7TqyHr5SQB/JRlwaxtkItNORxM9fmDNupc8dmKZPoARn5uTe5n
60Zp62wHtG76zPq2X74iMUpsD/ZT11qvKbaMNrJSktCNFF0dpZVQl1/2U+0N0tuc
INlDdSqZOqLFzUN9qacmejfeoGiElRd86uL8g6jDeW5HaQXza6aPX2NoevC6r1KC
/zAqAm8mED66p9L688mBNHixymXuTXBRJZO099PD5TyAwaMg8Vl9FsDOv/2Oywp+
EpBnSqCxk2A2nLE98V2CCm22nwVj3G+zdgFRGsK9EqTxOEo9J73fvScHrOJIHC7y
5MepnWJCs326XdqlQL7if4J/yTpIkyf10mDPc3UK0VpWBGV2tAuSYBe1s3yZdKi1
YvPeN58SJIAMCHLv3nsb254rt2DX8+g0KE5wmisAPRnimbwAELRJDUhj/UTKwQdN
TA5zad1OXryxMVVkBulQVz5lJPzeh1VeB/2v20VLGHn1MvGM5bA=
=4q5i
-----END PGP SIGNATURE-----

--VczgSuiIbYBCIHHr--