On Sun, Mar 24, 2019 at 11:24 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > there are 4 tests to check if planning_time is zero or not, it's quite
> > > messy. Could you refactor the code to avoid so many tests? It would
> > > probably be useful to add some asserts to check that we don't provide
> > > both planning_time == 0 and execution related values. The function's
> > > comment would also need to be adapted to mention the new rationale
> > > with planning_time.
> >
> > Fixed
>
> + /* updating counters for execute OR planning */
> + Assert(planning_time > 0 && total_time > 0);
> + if (planning_time == 0)
>
> This is obviously incorrect. The general sanity check for exclusion
> between planning_time and total_time should be at the beginning of
> pgss_store. Maybe some others asserts are needed to verify that
> planning_time cannot be provided along jstate or other conditions.
Actually, since pgss_store is now called to either:
- explicitly store a query text
- accumulate planning duration
- accumulate execution duration
and they're all mutually exclusive, It's probably better to change
pgss_store to pass an enum to describe what the call is for , and keep
a single time parameter. It should make the code simpler.