Re: Problem with parallel_workers option (Was Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead)

В письме от понедельник, 7 января 2019 г. 13:56:48 MSK пользователь Alvaro
Herrera написал:

> > Asserts are cool thing. I found some unexpected stuff.
> >
> > parallel_workers option is claimed to be heap-only option.
> >
> > But in src/backend/optimizer/util/plancat.c in get_relation_info
> > RelationGetParallelWorkers is being called for both heap and toast tables
> > (and not only for them).
>
> Ugh.
>
> I wonder if it makes sense for a toast table to have parallel_workers.
> I suppose it's not useful, since a toast table is not supposed to be
> scanned in bulk, only accessed through the tuptoaster interface. But on
> the other hand, you *can* do "select * from pg_toast_NNN", and it almost
> all respects a toast table is just like a regular heap table.

If parallel_workers is not intended to be used anywhere except heap and
matview, then may be better to make fix more relaxed. Like

if (relation->rd_rel->relkind == RELKIND_RELATION ||
relation->rd_rel->relkind == RELKIND_MATVIEW )
rel->rel_parallel_workers =
RelationGetParallelWorkers(relation,-1);
else
rel->rel_parallel_workers = -1;

> > Because usually there are no reloptions for toast, it returns default -1
> > value. If some reloptions were set for toast table,
> > RelationGetParallelWorkers will return a value from uninitialized memory.
>
> Well, if it returns a negative number or zero, the rest of the server
> should behave identically to it returning the -1 that was intended. And
> if it returns a positive number, the worst that will happen is that a
> Path structure somewhere will have a positive number of workers, but
> since queries on toast tables are not planned in the regular way, most
> likely those Paths will never exist anyway.
>
> So while I agree that this is a bug, it seems pretty benign.
It is mild until somebody introduce PartitionedlRelOptions. Then
PartitionedlRelOptions * will be converted to StdRdOptions* and we will get
segmentation fault...

So may be there is no need for back fix, but better to fix it now :-)
May be with the patch for StdRdOptions removal.