Horton

Re: Pluggable Storage - Andres's take

Haribabu Kommi <kommi.haribabu@gmail.com>

View thread View on postgr.es
From: Haribabu Kommi <kommi.haribabu@gmail.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Andres Freund <andres@anarazel.de>, Alvaro Herrera <alvherre@2ndquadrant.com>, David Rowley <david.rowley@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, Asim R P <apraveen@pivotal.io>, Pg Hackers <pgsql-hackers@postgresql.org>, Ashwin Agrawal <aagrawal@pivotal.io>, Dmitry Dolgov <9erthalion6@gmail.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2019-03-16T12:21:31Z
Lists: pgsql-hackers

Attachments

On Tue, Mar 12, 2019 at 7:28 PM Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> 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.
>
>
Thanks for the review.


>
> 0001-Reduce-the...
>
> This doesn't apply master.
>

Yes, these patches doesn't apply to the master.
These patches can only be applied to the code present in [1].


> >  TupleTableSlot *
> >  ExecStoreHeapTuple(HeapTuple tuple,
> >                                  TupleTableSlot *slot,
> > +                                Oid relid,
> >                                  bool shouldFree)
>
>   The comment for ExecStoreHeapTuple is missing the description
>   on "relid" parameter.
>

Corrected.


> > -        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?
>

Currently the passed relid is used only in the case of
history MVCC verification function. Passing the direct relation
pointer will lessen the performance impact as there is no
need of calculation to find out the relid.

Will update and share it.


>
>
> 0005-Reorganize-am-as...
>
> > +   catalog. The contents of an table are entirely under the control of
> its
>
>  "of an table" => "of a table"
>

Corrected.


> 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?
>

Changed.


>
> 0007-Doc-update-of-create-materi..
>
> > +   This clause specifies optional access method for the new materialize
> view;
>
>  "materialize view" => "materialized view"?
>

Corrected.


> > +   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.
>

Corrected as per your suggestions.


> 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"
>

Corrected above both.


> > + 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?
>

It returns the required parallel scan descriptor memory size.


>
> > +     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.
>

I updated the doc around those API's to make easy to understand.
Can you please check whether that helps?

updated patches are attached.

[1] -  https://github.com/anarazel/postgres-pluggable-storage.git

Regards,
Haribabu Kommi
Fujitsu Australia

Commits

  1. Add default_table_access_method to postgresql.conf.sample.

  2. tableam: Avoid relying on relation size to determine validity of tids.

  3. tableam: Don't assume that every AM uses md.c style storage.

  4. Allow pg_class xid & multixid horizons to not be set.

  5. Fix slot type issue for fuzzy distance index scan over out-of-core table AM.

  6. tableam: comment and formatting fixes.

  7. table: docs: fix typos and grammar.

  8. tableam: basic documentation.

  9. Only allow heap in a number of contrib modules.

  10. tableam: Add table_finish_bulk_insert().

  11. tableam: sample scan.

  12. tableam: bitmap table scan.

  13. tableam: Move heap specific logic from estimate_rel_size below tableam.

  14. tableam: VACUUM and ANALYZE support.

  15. tableam: relation creation, VACUUM FULL/CLUSTER, SET TABLESPACE.

  16. tableam: Support for an index build's initial table scan(s).

  17. tableam: Add table_get_latest_tid, to wrap heap_get_latest_tid.

  18. tableam: Add helper for indexes to check if a corresponding table tuples exist.

  19. tableam: Add and use table_fetch_row_version().

  20. tableam: Use in CREATE TABLE AS and CREATE MATERIALIZED VIEW.

  21. Ensure sufficient alignment for ParallelTableScanDescData in BTShared.

  22. Don't reuse slots between root and partition in ON CONFLICT ... UPDATE.

  23. Use a virtual rather than a heap slot in two places where that suffices.

  24. Store tuples for EvalPlanQual in slots, rather than as HeapTuples.

  25. Use slots in trigger infrastructure, except for the actual invocation.

  26. Store table oid and tuple's tid in tuple slots directly.

  27. Allow to use HeapTupleData embedded in [Buffer]HeapTupleTableSlot.

  28. Add ExecStorePinnedBufferHeapTuple.

  29. Add ArchiveOpts to pass options to ArchiveEntry

  30. Rename RelationData.rd_amroutine to rd_indam.

  31. Rephrase references to "time qualification".

  32. Move remaining code from tqual.[ch] to heapam.h / heapam_visibility.c.

  33. Move generic snapshot related code from tqual.h to snapmgr.h.

  34. Remove superfluous tqual.h includes.

  35. Don't duplicate parallel seqscan shmem sizing logic in nbtree.

  36. Move vacuumlazy.c into access/heap.

  37. Support parallel bitmap heap scans.