Re: BUG #15324: Non-deterministic behaviour from parallelised sub-query

On Mon, Sep 3, 2018 at 12:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>
> >> + * Treat window functions as parallel-restricted as the row ordering
> >> + * induced by them is non-deterministic.  We can relax this condition for
> >> + * cases where the row ordering can be deterministic like when there is
> >> + * an ORDER BY on the primary key, but those cases don't seem to be
> >> + * interesting enough to have additional checks.
>
> This comment seems fairly confused.  I'd say something like
>
>  * Treat window functions as parallel-restricted because we aren't sure
>  * whether the input row ordering is fully deterministic, and the output
>  * of window functions might vary across workers if not.  (In some cases,
>  * like where the window frame orders by a primary key, we could relax
>  * this restriction.  But it doesn't currently seem worth expending extra
>  * effort to do so.)
>

Changed.

> >> In addition to the above, I have marked all built-in window functions
> >> as parallel-restricted.  I think even if we don't do that something
> >> like above check should be sufficient, but OTOH, I don't see any
> >> reason to keep the marking of such functions as parallel-safe.  Is
> >> there a reason, why we shouldn't mark them as parallel-restricted?
>
> I am *strongly* against this.  It's unnecessary catalog churn that we
> might need to undo someday, and it confuses a property of the window
> function infrastructure with a property of individual window functions.
> As a counterexample, if a window function were parallel-unsafe for
> some reason, we'd surely need to honor that.  More realistically,
> someone might add a window function that actually needs to be
> parallel-restricted for reasons of its own, but then there would be
> no obvious distinction between such a function and one that you'd
> hacked up to be marked parallel-restricted even though it's safe in
> itself.  If we then do make the sort of optimization suggested in the
> comment, it's likely that someone would just s/r/s/g for all the window
> functions and thereby break such a function.
>

Sounds sensible, I have removed that part from the patch.

You haven't mentioned anything about backpatching, but I don't see any
problem with backpatching this fix.  So, I have prepared patches for
back branches wherever required.  For 10, it is mostly a cosmetic
change (the patch didn't apply cleanly), but for 9.6 the prohibition
check is slightly different.

I will commit the attached patches in a day or so unless somebody sees
any problem.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com