Re: [HACKERS] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Apr 11, 2017 at 5:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think the fact that we see this problem at all may indicate an
>> oversight in commit 5e6d8d2bbbcace304450b309e79366c0da4063e4 ("Allow
>> parallel workers to execute subplans").  If the worker were to actually
>> run the initplan, bad things would happen (the worker would create its
>> own remote connection, which we don't want).  Now, in the problem plan
>> the InitPlan is actually attached to the topmost Gather, which I think
>> is safe because it'll be run by the master, but I wonder if we're being
>> careful enough about non-parallel-safe plans for initplans/subplans.

> Initplans are never marked parallel safe, only subplans that are
> generated for parallel safe paths are marked as parallel safe.

OK ...

>> Also, even if the worker never actually executes the plan node, just
>> doing ExecInitNode on it in a parallel worker might be more than a
>> non-parallel-safe FDW is prepared to cope with.

> I think there is a possibility of doing ExecInitNode in a parallel
> worker for a parallel-unsafe subplan, because we pass a list of all
> the sublans stored in planned statement.

It's more than a possibility.  If you run Andreas' test case against
HEAD, now that the can't-transmit-RestrictInfo failure is fixed,
you will find that the parallel worker actually creates and immediately
destroys an FDW remote connection.  (The easiest way to prove that is
to turn on log_connections/log_disconnections.  BTW, is there any
convenient way to attach a debugger to a parallel worker process as it's
being launched?  I couldn't manage to catch the worker in the act.)

So I think this is clearly a Bad Thing and we'd better do something
about it.  The consequences for postgres_fdw aren't so awful perhaps,
but other non-parallel-safe FDWs might have bigger problems with this
behavior.

> However, the worker will
> never execute such a plan as we don't generate a plan where unsafe
> sublan/initplan is referenced in the node passed to the worker.  If we
> want to avoid passing parallel-unsafe subplans to workers, then I
> think we can maintain a list of parallel safe subplans along with
> subplans in PlannerGlobal and PlannedStmt or maybe keep a parallel
> safe flag in Plan so that we can pass only parallel safe plans to
> workers.

Right, we could, say, leave a hole in the subplan list corresponding
to any subplan that's not parallel-safe.  That seems like a good idea
anyway because right now there's clearly no cross-check preventing
a worker from trying to run such a subplan.

Anyone want to draft a patch for this?
        regards, tom lane