Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
> В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
>
> Yet another git rebase. This patch can be applied to master branch again.
>
> For this patch no review needed now, as I will offer part of it (one that
> refuses to set toast reloptions when there is no TOAST table) as a separate
> patch.

This patch needs a rebase, there are conflicts with HEAD.

-\set VERBOSITY terse
CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0);
ERROR: value 0 out of bounds for option "length"
+DETAIL: Valid values are between "1" and "4096".
+CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=4097);
+ERROR: value 4097 out of bounds for option "length"
+DETAIL: Valid values are between "1" and "4096".
The basic set of relopt tests have been committed 4b95cc1. Why
insisting on them?

+ if (validate)
+ for (i = 0; i < INDEX_MAX_KEYS; i++)
+ {
The lack of brackets here is not project-like. You should use some for clarity.

There is still no API equivalent for add_int_reloption(),
add_real_reloption(), which is a problem as extension should be
allowed to still use that. Except if I am missing something you could
just have a compatibility API as for example optionCatalogAddItem()
and add_reloption() share really a lot.

}
-
return lockmode;
[...]
#include "utils/typcache.h"
-
+#include "utils/memutils.h"
+#include "utils/guc.h"
Much noise diffs.

LOCKMODE
-AlterTableGetRelOptionsLockLevel(List *defList)
+AlterTableGetRelOptionsLockLevel(Relation rel, List *defList)
{
This could just be static within tablecmds.c.

-#define RelationGetTargetPageFreeSpace(relation, defaultff) \
- (BLCKSZ * (100 - RelationGetFillFactor(relation, defaultff)) / 100)
+#define HeapGetTargetPageFreeSpace(relation, defaultff) \
+ (BLCKSZ * (100 - HeapGetFillFactor(relation, defaultff)) / 100)
-1 for this kind of renames. OK, fillfactor cannot be set for toast
tables but that's not a reason to break current APIs and give more
noise to extension authors.

+/* optionsDefListToRawValues
+ * Converts option values that came from syntax analyzer (DefList) into
+ * Values List.
Should be careful about comment block format.

+ case RELKIND_PARTITIONED_TABLE:
+ catalog = NULL; /* No options for parted table for now */
+ break;
s/parted/partitioned/.

+ amrelopt_catalog_function amrelopt_catalog; /* can be NULL */
Or amoptionlist? Catalog is confusing and refers to system catalogs for example.

- int bufferingModeOffset; /* use buffering build? */
+ int buffering_mode; /* use buffering build? */
I would suggest avoiding unnecessary renames to keep the patch simple.
--
Michael