Re: assessing parallel-safety - Mailing list pgsql-hackers

On Wed, Feb 18, 2015 at 12:23:05PM -0500, Robert Haas wrote:
> On Sun, Feb 15, 2015 at 6:24 PM, Noah Misch <noah@leadboat.com> wrote:
> > It's important for parallelism to work under the extended query protocol, and
> > that's nontrivial.  exec_parse_message() sets cursorOptions.
> > exec_bind_message() runs the planner.  We don't know if a parallel plan is
> > okay before seeing max_rows in exec_execute_message().
> 
> Yes, that's a problem.  One could require users of the extended query
> protocol to indicate their willingness to accept a parallel query plan
> when sending the bind message by setting the appropriate cursor
> option; and one could, when that option is specified, refuse execute
> messages with max_rows != 0.  However, that has the disadvantage of
> forcing all clients to be updated for the new world order.

Right; that would imply a protocol version bump.

> Or, one
> could assume a parallel plan is OK and re-plan if we choose one and
> then a non-zero max_rows shows up.  In the worst case, though, that
> could require every query to be planned twice.

That sounds reasonable as a first cut.  It is perfect for libpq clients, which
always use zero max_rows.  The policy deserves wider scrutiny before release,
but it's the sort of thing we can tweak without wide-ranging effects on the
rest of the parallelism work.  One might use the Bind message portal name as
an additional heuristic.  libpq usually (always?) uses the empty portal name.
A caller specifying a non-empty portal name is far more likely to have a
non-zero max_rows on its way.  More radical still: upon receiving the Bind
message, peek ahead to see if an Execute message is waiting.  If so, let the
max_rows of the Execute inform Bind-associated planning.

> > The CREATE OR REPLACE FUNCTION calls in src/backend/catalog/system_views.sql
> > reset proparallel=u for some built-in functions, such as make_interval.
> 
> I can fix that if I add a PARALLEL { SAFE | RESTRICTED | UNSAFE }
> clause to CREATE FUNCTION.

Eventually that, but adding "UPDATE pg_proc" after the CORF is a fine stopgap.

> Or are you saying that CORF shouldn't
> change the existing value of the flag?  I'm not sure exactly what our
> policy is here, but I would expect that the CORF is doing the right
> thing and we need to add the necessary syntax to CREATE FUNCTION and
> then add that clause to those calls.

I agree that CREATE OR REPLACE FUNCTION is doing the right thing.

> > - Several functions, such as array_in and anytextcat, can call arbitrary I/O
> >   functions.  This implies a policy that I/O functions must be
> >   PROPARALLEL_SAFE.  I agree with that decision, but it should be documented.
> 
> Where?

On further thought, it can wait for the CREATE FUNCTION syntax additions.  At
that time, xtypes.sgml is the place.  Perhaps just adding PARALLEL SAFE to the
example CREATE FUNCTION calls will suffice.

> > - All selectivity estimator functions are PROPARALLEL_SAFE.  That implies, for
> >   example, that any operator using eqjoinsel ought to be PROPARALLEL_SAFE or

(I meant to write "eqsel", not "eqjoinsel".  eqjoinsel is not a restriction
selectivity estimator function, but the rest of the argument applies to both
functions.)  That argument does not apply to all possible selectivity
estimator functions.  It is common for an estimator to call the opercode on
MCV and/or histogram values, which makes the estimator function no more
parallel safe than the weakest associated opercode.  eqsel, eqjoinsel and
scalarltsel all do that.

> >   define its own restriction selectivity estimator function.  On the other
> >   hand, the planner need not check the parallel safety of selectivity
> >   estimator functions.  If we're already in parallel mode at the moment we
> >   enter the planner, we must be planning a query called within a
> >   PROPARALLEL_SAFE function.  If the query uses an unsafe operator, the
> >   containing function was mismarked.
> 
> This seems backwards to me.  If some hypothetical selectivity
> estimator were PROPARALLEL_UNSAFE, then any operator that uses that
> function would also need to be PROPARALLEL_UNSAFE.

It's fair to have a PROPARALLEL_UNSAFE estimator for a PROPARALLEL_SAFE
function, because the planning of a parallel query is often not itself done in
parallel mode.  In that case, "SELECT * FROM tablename WHERE colname OP 0"
might use a parallel seqscan but fail completely if called from inside a
function running in parallel mode.  That is to say, an affected query can
itself use parallelism, but placing the query in a function makes the function
PROPARALLEL_UNSAFE.  Surprising, but not wrong.

Rereading my previous message, I failed to make the bottom line clear: I
recommend marking eqsel etc. PROPARALLEL_UNSAFE but *not* checking an
estimator's proparallel before calling it in the planner.

> The reverse is not
> true.  The operator can be as unsafe as it likes and still use a safe
> estimator.

Agreed.  "contsel" is PROPARALLEL_SAFE no matter what operator uses it.

> > - Assuming you don't want to propagate XactLastRecEnd from the slave back to
> >   the master, restrict XLogInsert() during parallel mode.  Restrict functions
> >   that call it, including pg_create_restore_point, pg_switch_xlog and
> >   pg_stop_backup.
> 
> Hmm.  Altogether prohibiting XLogInsert() in parallel mode seems
> unwise, because it would foreclose heap_page_prune_opt() in workers.
> I realize there's separate conversation about whether pruning during
> SELECT queries is good policy, but in the interested of separating
> mechanism from policy, and in the sure knowledge that allowing at
> least some writes in parallel mode is certainly going to be something
> people will want, it seems better to look into propagating
> XactLastRecEnd.

Good points; that works for me.