The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi,
thank you for the patch. It applies cleanly, compiles and passes check,
check-world.
I feel as per the discussion, this is a step to the right direction yet it does
not get far enough. From experience, I can confirm that dealing with reloptions
in a new table AM is somewhat of a pain. Ultimately, reloptions should be
handled by the table AM specific code. The current patch does not address the
issue. Yet it does make the issue easier to address by clearing up the current
state.
If you allow me, I have a couple of comments.
- saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
-
HEAP_DEFAULT_FILLFACTOR);
+ if (IsToastRelation(relation))
+ saveFreeSpace = ToastGetTargetPageFreeSpace();
+ else
+ saveFreeSpace = HeapGetTargetPageFreeSpace(relation);
For balance, it does make some sense for ToastGetTargetPageFreeSpace() to get
relation as an argument, similarly to HeapGetTargetPageFreeSpace().
Also, this pattern is repeated in four places, maybe the branch can be moved
inside a macro or static inline instead?
- /* Retrieve the parallel_workers reloption, or -1 if not set. */
- rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1);
+ /*
+ * Retrieve the parallel_workers for heap and mat.view relations.
+ * Use -1 if not set, or if we are dealing with other relation kinds
+ */
+ 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;
If the comment above is agreed upon, then it makes a bit of sense to apply the
same here. The expression in the branch is already asserted for in macro, why
not switch there and remove the responsibility from the caller?
Any thoughts on the above?
Cheers,
Georgios
The new status of this patch is: Waiting on Author
