Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Jan-07, Tom Lane wrote:
>> ... and, having now looked at the patch, I'm not surprised.
>> Breaking stmtStartTimestamp, which is what you did, seems like
>> an awfully side-effect-filled route to the goal. If you want
>> to prevent monitoring from showing this, why didn't you just
>> prevent monitoring from showing it? That is, I'd have expected
>> some am_walsender logic in or near pgstat.c, not here.
> That seems a pretty simple patch; attached (untested).
I think you want && not ||, but otherwise that looks about right.
> However, my
> patch seemed a pretty decent way to achieve the goal, and I don't
> understand why it causes the failure, or indeed why we care about
> stmtStartTimestamp at all. I'll look into this again tomorrow.
I'm not 100% sure why the failure either. The assertion is in
code that should only be reached in a parallel worker, and surely
walsenders don't launch parallel queries? But it looks to me
that all the critters using force_parallel_mode are unhappy.
In any case, my larger point is that stmtStartTimestamp is globally
accessible state (via GetCurrentStatementStartTimestamp()) and you
can have little idea which corners of our code are using it, let
alone what extensions might expect about it. Plus it feeds into
xactStartTimestamp (cf StartTransaction()), increasing the footprint
for unwanted side-effects even more. Redefining its meaning
to fix this problem is a really bad idea IMO.
regards, tom lane
