Re: [HACKERS] [PATCH] Move all am-related reloption code intosrc/backend/access/[am-name] and get rid of relopt_kind for custom AM - Mailing list pgsql-hackers

On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan@nataraj.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 terseCREATE 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