Re: pgsql: Add hooks for session start and session end, take two

On Tue, Oct 01, 2019 at 03:16:06AM +0000, Michael Paquier wrote:
> Add hooks for session start and session end, take two
>
> These hooks can be used in loadable modules.  A simple test module is
> included.
>
> 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=crake&dt=2019-10-01%2003%3A32%3A29
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thorntail&dt=2019-10-01%2003%3A51%3A57

Both machines are using force_parallel_mode = regress, and I can
reproduce the module crash with that backtrace once I enforce the
parameter:
#2  0x00005609ce1f1454 in ExceptionalCondition
(conditionName=conditionName@entry=0x5609ce25cdd0
!IsParallelWorker()",
errorType=errorType@entry=0x5609ce24501d "FailedAssertion",
fileName=fileName@entry=0x5609ce259bef "xact.c",
lineNumber=lineNumber@entry=757) at assert.c:54
#3  0x00005609cde04410 in GetCurrentCommandId
(used=used@entry=true) at xact.c:757
#4  0x00005609cdf3e620 in standard_ExecutorStart
(queryDesc=0x5609cf072330, eflags=0) at execMain.c:238
#5  0x00005609cdf7ad71 in _SPI_pquery (tcount=0,
fire_triggers=true, queryDesc=<optimized out>) at spi.c:2519
#6  _SPI_execute_plan (plan=<optimized out>, paramLI=<optimized
out>, snapshot=<optimized out>, crosscheck_snapshot=<optimized
out>, read_only=<optimized out>,
fire_triggers=<optimized out>, tcount=<optimized out>) at spi.c:2297
#7  0x00005609cdf7b3ac in SPI_execute (src=0x5609cf06c050 "INSERT INTO
session_hook_log (dbname, username, hook_at) VALUES
('contrib_regression', 'regress_sess_hook_usr2', 'END');",
    read_only=read_only@entry=false,
tcount=tcount@entry=0) at spi.c:514
#8  0x00005609cdf7b3ea in SPI_exec (src=<optimized out>,
tcount=tcount@entry=0) at spi.c:526
#9  0x00007feefe55b2ef in register_session_hook
(hook_at=<optimized out>) at test_session_hooks.c:67
#10 0x00005609ce09a351 in shmem_exit
(code=code@entry=0) at ipc.c:239
#11 0x00005609ce09a45d in proc_exit_prepare
(code=code@entry=0) at ipc.c:194
#12 0x00005609ce09a502 in proc_exit
(code=code@entry=0) 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 = 0;
SET max_parallel_workers_per_gather = 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