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
canconfirm that dealing with reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions should be
handledby the table AM specific code. The current patch does not address the issue. Yet it does make the issue easier
toaddress 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
isalready 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
