Re: Pluggable Storage - Andres's take - Mailing list pgsql-hackers

Hello.

I had a look on the patch set. I cannot see the thread structure
due to the depth and cannot get the picture on the all patches,
but I have some comments. I apologize in advance for possible
duplicate with upthread.


0001-Reduce-the...

This doesn't apply master.

>  TupleTableSlot *
>  ExecStoreHeapTuple(HeapTuple tuple,
>                     TupleTableSlot *slot,
> +                   Oid relid,
>                     bool shouldFree)

  The comment for ExecStoreHeapTuple is missing the description
  on "relid" parameter.


> -        if (HeapTupleSatisfiesVisibility(tuple, &SnapshotDirty, hscan->rs_cbuf))
> +        if (HeapTupleSatisfiesVisibility(tuple, RelationGetRelid(hscan->rs_scan.rs_rd),
> +                                &SnapshotDirty, hscan->rs_cbuf))

The second parameter seems to be always
RelationGetRelid(...). Actually only relid is required but isn't
it better to take Relation instead of Oid as the second
parameter?



0005-Reorganize-am-as...

> +   catalog. The contents of an table are entirely under the control of its

 "of an table" => "of a table"

0006-Doc-update-of-Create-access..

> +      be <type>index_am_handler</type> and for <literal>TABLE</literal>
> +      access methods, it must be <type>table_am_handler</type>.
> +      The C-level API that the handler function must implement varies
> +      depending on the type of access method. The index access method API
> +      is described in <xref linkend="index-access-methods"/> and the table access method
> +      API is described in <xref linkend="table-access-methods"/>.

If table is the primary object, talbe-am should precede index-am?


0007-Doc-update-of-create-materi..

> +   This clause specifies optional access method for the new materialize view;

 "materialize view" => "materialized view"?

> +   If this option is not specified, then the default table access method

 I'm not sure the 'then' is needed.

> +   is chosen for the new materialized view. see <xref linkend="guc-default-table-access-method"/>

 "see" => "See"


0008-Doc-update-of-CREATE_TABLE..

> +[ USING <replaceable class="parameter">method</replaceable> ]

 *I* prefer access_method to just method.

> +  If this option is not specified, then the default table access method

 Same to 0007. "I'm not sure the 'then' is needed.".

> +  is chosen for the new table. see <xref linkend="guc-default-table-access-method"/>

 Same to 0007. " "see" => "See" "
"

0009-Doc-of-CREATE-TABLE-AS

 Same as 0008.


0010-Table-access-method-API-

> +    Any new <literal>TABLE ACCSESS METHOD</literal> developers can refer the exisitng <literal>HEAP</literal>


> +    There are differnt type of API's that are defined and those details are below.

  "differnt" => "different"

> +     by the AM, in case if it support parallel scan.

  "support" => "supports"

> + This API to return the total size that is required for the AM to perform

  Total size of what? Shared memory chunk? Or parallel scan descriptor?


> +     the parallel table scan. The minimum size that is required is 
> +     <structname>ParallelBlockTableScanDescData</structname>.

  I don't get what the "The minimum size" tells. Just reading
  this I would always return the minimum size...


> +     This API to perform the initialization of the <literal>parallel_scan</literal>
> +     that is required for the parallel scan to be performed by the AM and also return
> +     the total size that is required for the AM to perform the parallel table scan.

 (Note: I'm not good at English..) Similar to the above. I cannot
 read what the "size" is for.

 In the code it is used as:

  > Size snapshot_off = rel->rd_tableam->parallelscan_initialize(rel, pscan);

  (The varialbe name should be snapshot_offset) It is the offset
  from the beginning of parallel scan descriptor but it should be
  described in other representation, which I'm not sure of..

  Something like this?

  > This API to initialize a parallel scan by the AM and also
  > return the consumed size so far of parallel scan descriptor.

(Sorry for not finishing. Time's up today.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center