Date: Tue, 1 Oct 2019 13:52:46 +0900
From: Michael Paquier <michael@paquier.xyz>
To: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Add hooks for session start and session end, take two
Message-ID: <20191001045246.GF2781@paquier.xyz>
References: <E1iF8dy-0000fc-9S@gemulon.postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="9/eUdp+dLtKXvemk"
Content-Disposition: inline
In-Reply-To: <E1iF8dy-0000fc-9S@gemulon.postgresql.org>
Precedence: bulk


--9/eUdp+dLtKXvemk
Content-Type: multipart/mixed; boundary="924gEkU1VlJlwnwX"
Content-Disposition: inline


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

On Tue, Oct 01, 2019 at 03:16:06AM +0000, Michael Paquier wrote:
> Add hooks for session start and session end, take two
>=20
> These hooks can be used in loadable modules.  A simple test module is
> included.
>=20
> The first attempt was done with cd8ce3a but we lacked handling for
> NO_INSTALLCHECK in the MSVC scripts (problem solved afterwards by
> 431f1599) so the buildfarm got angry.  This also fixes a couple of
> issues noticed upon review compared to the first attempt, so the code
> has slightly changed, resulting in a more simple test module.

So the buildfarm got again angry on that with crake and thorntail:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=3Dcrake&dt=3D2019-1=
0-01%2003%3A32%3A29
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=3Dthorntail&dt=3D20=
19-10-01%2003%3A51%3A57

Both machines are using force_parallel_mode =3D regress, and I can
reproduce the module crash with that backtrace once I enforce the
parameter:
#2  0x00005609ce1f1454 in ExceptionalCondition
(conditionName=3DconditionName@entry=3D0x5609ce25cdd0
!IsParallelWorker()",
errorType=3DerrorType@entry=3D0x5609ce24501d "FailedAssertion",
fileName=3DfileName@entry=3D0x5609ce259bef "xact.c",
lineNumber=3DlineNumber@entry=3D757) at assert.c:54
#3  0x00005609cde04410 in GetCurrentCommandId
(used=3Dused@entry=3Dtrue) at xact.c:757
#4  0x00005609cdf3e620 in standard_ExecutorStart
(queryDesc=3D0x5609cf072330, eflags=3D0) at execMain.c:238
#5  0x00005609cdf7ad71 in _SPI_pquery (tcount=3D0,
fire_triggers=3Dtrue, queryDesc=3D<optimized out>) at spi.c:2519
#6  _SPI_execute_plan (plan=3D<optimized out>, paramLI=3D<optimized
out>, snapshot=3D<optimized out>, crosscheck_snapshot=3D<optimized
out>, read_only=3D<optimized out>,
fire_triggers=3D<optimized out>, tcount=3D<optimized out>) at spi.c:2297
#7  0x00005609cdf7b3ac in SPI_execute (src=3D0x5609cf06c050 "INSERT INTO
session_hook_log (dbname, username, hook_at) VALUES
('contrib_regression', 'regress_sess_hook_usr2', 'END');",
    read_only=3Dread_only@entry=3Dfalse,
tcount=3Dtcount@entry=3D0) at spi.c:514
#8  0x00005609cdf7b3ea in SPI_exec (src=3D<optimized out>,
tcount=3Dtcount@entry=3D0) at spi.c:526
#9  0x00007feefe55b2ef in register_session_hook
(hook_at=3D<optimized out>) at test_session_hooks.c:67
#10 0x00005609ce09a351 in shmem_exit
(code=3Dcode@entry=3D0) at ipc.c:239
#11 0x00005609ce09a45d in proc_exit_prepare
(code=3Dcode@entry=3D0) at ipc.c:194
#12 0x00005609ce09a502 in proc_exit
(code=3Dcode@entry=3D0) at ipc.c:107
#13 0x00005609ce02c42f in StartBackgroundWorker () at
bgworker.c:837

This indicates that it is possible to reach GetCurrentCommandId() for
a parallel worker in this context.  It is possible to get rid of the
problem by enforcing the following in the tests so as we have no
parallel plans:
SET max_parallel_workers =3D 0;
SET max_parallel_workers_per_gather =3D 0;
Or just use SetConfigOption as per the attached which would be a bit
cleaner, and both could be used as a temporary solution to cool down
the buildfarm but that does not feel completely right to me in the
long term.

Any thoughts?
--
Michael

--924gEkU1VlJlwnwX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="session-hooks-fix.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/test/modules/test_session_hooks/test_session_hooks.c b/src=
/test/modules/test_session_hooks/test_session_hooks.c
index d047c5d219..6ffd1cca08 100644
--- a/src/test/modules/test_session_hooks/test_session_hooks.c
+++ b/src/test/modules/test_session_hooks/test_session_hooks.c
@@ -44,6 +44,12 @@ register_session_hook(const char *hook_at)
 	SPI_connect();
 	PushActiveSnapshot(GetTransactionSnapshot());
=20
+	/* Disable parallelism in this context */
+	SetConfigOption("max_parallel_workers", "0",
+					PGC_SUSET, PGC_S_OVERRIDE);
+	SetConfigOption("max_parallel_workers_per_gather", "0",
+					PGC_SUSET, PGC_S_OVERRIDE);
+
 	/* Check the current user validity */
 	username =3D GetUserNameFromId(GetUserId(), false);
=20

--924gEkU1VlJlwnwX--

--9/eUdp+dLtKXvemk
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2S254ACgkQnvQgOdby
QH0nOBAAkvoGMuzpTMYQ2Ppukc/Kys05VN7zmnS+CRAsYppuBIi5aDiyr6pLBmuk
Gem5obyiGxVKPI6IGF5vP9p3XHV7NxzHQzWJnHdpER8Zjyism/4dHHGmxanzy4ND
Ll4EusH8gVmRSUrMSNjlaXAc1lku/CYJ/x+xkvKntIsUFvu3gswGcGS8lLhKyVcC
p7cGYing9J5CPIpUfXyLcvZcPDB3/BsExmrUVw7yS8F8pBw6IQtgwcFcmVIiggN6
iBJebBE8QZPi2lnvHkPTDonJ/ZzlhIwhDU6FE/blVmkSprDL+2q7K6JaevPep8rz
fymep5lw1f0j2aY2Ec5qKK12k5UhsYUNnzOvzWzXKYzJZP7m00VDzZnOvKMYz+At
YqCdPWA0WO6CcerYIi10JfRRhFDAUbVsDORg68WmqzkHtezEsftHnwoHxXgsYKpy
j0FCVqHUUdbdQBlvZgp3On18I7hKZR8APt6D2gTorWVAxmcK4VdR1cAYa/GV0Bvu
MfWSvsud3fY6H10UyJyw5fS5h5RDJrTjGnqHnqWFgih5pKMlgRDC24wQo+lEvVj4
h1TDr4J8h9mOvb63tsUZBQQKsZ5ckf7OEDYft+/kNkdLt9k2oj+OR8N+S2YntKEm
fAiWwOl9QzspsuLpUFSFdOnm6WQkQYyqrCwR2tjVNoDGd+anPLg=
=elAx
-----END PGP SIGNATURE-----

--9/eUdp+dLtKXvemk--