RE: [WIP]Vertical Clustered Index (columnar store extension) - take2 - Mailing list pgsql-hackers

Hi Tomas-san,


> > - Divide the patch more and more
> >   - to allow committers to consider pros and cons and push one by one
> >   - 15 patches is the maximum amount
> >   - Separate features to some committable group
> >
> 
> Yes. Both parts need to be divided into small (but meaningful) pieces. I
> don't recall where the "15 patches" came from, I don't think there's
> some sort of maximum number of patches. All I care about is that the
> patches need to be small-ish, and still make sense. Ideally, each part
> should be committable on it's own. This makes it easier to test/review,
> but also easier to agree on individual parts - in which case we may
> commit the "preparatory" pieces and not have to rebase them forever.


Yes, we are aware the patch 0002 is far too big and are taking steps to 
try to address that, firstly by removing any "dead" code, and secondly 
by extracting out any "optional" components. This is an ongoing task. 
If you have any other ideas how to split the patch please let us know.

> relation_open
> ------------------
> - Why is this change (disabling assert enforcing proper locking) needed?
> There's a comment in lock.c which mentions VCI parallel workers, but
> it's suspicious.

This comment is removed in the current posted patches.

> reloptions.c
> ------------------
> - Changes that should be moved to the contrib module Why should in-core
> reloptions know about this? See for example how "contrib/bloom" defined
> reloptions.

Thanks. We are studying how to replace this with an extensible solution
using "contrib/bloom" as guidance. Also, as Timur (28/Jun) suggests that
this might be unused code anyway. Investigating...

> heapam.c
> ------------------
> - add_index_delete_hook, not sure I understand the purpose of this,
> likely related to the consistency model, needs explanation. Why is it
> prefixed with "add_"?

Correct, this is related to maintaining consistency for the WOS
when a delete occurs. The "add_" prefix has no special meaning.
All of the VCI ad-hoc hooks have a prefix of "add_"

> - Furthermore, if we need this, why should it be a hook and not a new
> callback in IndexAmRoutine, just like aminsert()?

Agree. We intend later to implement this ad-hoc hook as a callback
in IndexAmRoutine as suggested.


> vacuum.c
> ------------------

> - Why. And what is "internal table"?

Please refer to the contrib/vci/README section 2.4 for a description
of VCI "Internal Relations"

> execGrouping.c
> ------------------
> - LookupTupleHashEntry_internal removes comment, why?

The latest post patches do not have this code.

> execUtils.c
> ------------------
> - again, disables locking check, very suspicious

Fixed in latest posted patches.

> nodeModifytable.c
> ------------------
> - add_should_index_insert_hook another strange hook

Agree. We intend later to implement this ad-hoc hook as a callback
in IndexAmRoutine as suggested.

> mctx.c
> ------------------
> - why MemoryContextMethods shouldn't be "const"?

Fixed in latest posted patches.

> mctx.c
> ------------------
> - why MemoryContextMethods shouldn't be "const"?
> 
> 
> common.c
> pg_dump.c
> pg_dump.h
> ------------------

> 
> - What is "vci view table" for, actually?

Fixed in latest posted patches.


> reloptions.h
> ------------------
> - Why does this need to add an entry into the relopt_kind enum? It
> should be extensible, see the "contrib/bloom".

Thanks. We are studying how to replace this with an extensible
solution using "contrib/bloom" as guidance.
(Timur suggests that this also unused code anyway. Investigating...)

> executor.h
> ------------------
> - How come it's suddenly OK to return entry->additional without the if
> conditions in TupleHashEntryGetAdditional?

This was an accidental revert of a master commit also reported by Timur.
The latest posted patches no longer have this change.

> 
> - What's with the struct LimitState; ? Why is it needed?

Fixed in latest posted patches.


> numeric.h
> ------------------
> - Why does this expose NumericVar, when the second patch redefines the
> struct anyway? also, isn't that fragile? although, we're unlikely to
> tweak the NumericVar definition.

Fixed in latest posted patches.

> rel.h
> ------------------

> - Doesn't AM have custom reloptions now? Like contrib/bloom?

Thanks. We are studying how to replace this StdRdOptions change
with an extensible solution using "contrib/bloom" as guidance.
(Timur suggests that this also unused code anyway. Investigating...)



Regards,
Aya Iwata
Fujitsu Limited