Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

В письме от понедельник, 18 марта 2019 г. 17:00:24 MSK пользователь Kyotaro
HORIGUCHI написал:

> > So I change status to "Waiting for Author".
> That seems to be a good oppotunity. I have some comments.
>
> rel.h:
> -#define RelationGetToastTupleTarget(relation, defaulttarg) \
> - ((relation)->rd_options ? \
> - ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target :
> (defaulttarg)) +#define RelationGetToastTupleTarget(relation, defaulttarg)
> \ + (AssertMacro(relation->rd_rel->relkind ==
> RELKIND_RELATION || \ + relation->rd_rel->relkind ==
> RELKIND_MATVIEW), \ + ((relation)->rd_options ?
> \ + ((HeapRelOptions *)
> (relation)->rd_options)->toast_tuple_target : \ +
> (defaulttarg)))
>
> Index AMs parse reloptions by their handler
> functions. HeapRelOptions in this patch are parsed in
> relation_reloptions calling heap_reloptions. Maybe at least it
> should be called as table_options. But I'm not sure what is the
> desirable shape of relation_reloptions for the moment.

If we create some TableOptions, or table_options we will create a bigger
mess, then we have now. Table == relation, and index is also relation.
So better to follow the rule: we have heap relation, toast relation, and all
variety of index relations. Each kind of relation have it's own options set,
each kind of relation has own marcoses to access it's options. This will make
the situation more structured.

> - saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
> -
> HEAP_DEFAULT_FILLFACTOR); + if (IsToastRelation(relation))
> + saveFreeSpace = ToastGetTargetPageFreeSpace();
> + else
> + saveFreeSpace = HeapGetTargetPageFreeSpace(relation);
>
> This locution appears four times in the patch and that's the all
> where the two macros appear. And it might not be good that
> RelationGetBufferForTuple identifies a heap directly since I
> suppose that currently tost is a part of heap. Thus it'd be
> better that HeapGetTargetPageFreeSpace handles the toast case.
> Similary, it doesn't looks good that RelationGetBufferForTuple
> consults HeapGetTargretPageFreeSpace() directly. Perhaps it
> should be called via TableGetTargetPageFreeSpace(). I'm not sure
> what is the right shape of the relationship among a relation and
> a table and other kinds of relation. extract_autovac_opts
> penetrates through the modularity boundary of toast/heap in a
> similar way.
So I started from the idea, I said above: each relation kind has it's own
macroses to access it's option.
If some options are often used combined, it might be good to create some
helper-macros, that will do this combination. But Alvaro is against adding any
new macroses (I understand why), and keep old one as intact as possible. So
here I can suggest only one thing: keep to the rule: "Each reloption kind has
it's own option access macroses" and let the developers of heap-core make
their live more comfortable with a helpers function the way they need it.

So I would leave this decision to Alvaro... He knows better...

> plancat.c:
> + 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;
>
> rel.h:
> #define RelationGetParallelWorkers(relation, defaultpw) \
> (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION || \
> relation->rd_rel->relkind == RELKIND_MATVIEW), \
> ((relation)->rd_options ? \
> ((HeapRelOptions *) (relation)->rd_options)->parallel_workers : \
> (defaultpw)))
>
> These function/macros are doing the same check twice at a call.
No. The first check is actual check, that does in runtime in production, and it
decides if we need to fetch some options or just use default value.

The second check is AssertMacro check. If you look into AssertMacro code you
will see that in production builds it do not perform any real check, just an
empty function.
AssertMacro is needed for developer builds, where it would crash if check is
not passed. It is needed to make sure that function is always used in proper
context. You should build postgres with --enable-cassert options to get it to
work. So there is no double checks here...