Re: Pluggable Storage - Andres's take

Hi,

Thanks for these changes. I've merged a good chunk of them.

On 2018-11-16 12:05:26 +1100, Haribabu Kommi wrote:
> diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
> index c3960dc91f..3254e30a45 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -1741,7 +1741,7 @@ heapam_scan_analyze_next_tuple(TableScanDesc sscan, TransactionId OldestXmin, do
>  {
>      HeapScanDesc scan = (HeapScanDesc) sscan;
>      Page        targpage;
> -    OffsetNumber targoffset = scan->rs_cindex;
> +    OffsetNumber targoffset;
>      OffsetNumber maxoffset;
>      BufferHeapTupleTableSlot *hslot;
>  
> @@ -1751,7 +1751,9 @@ heapam_scan_analyze_next_tuple(TableScanDesc sscan, TransactionId OldestXmin, do
>      maxoffset = PageGetMaxOffsetNumber(targpage);
>  
>      /* Inner loop over all tuples on the selected page */
> -    for (targoffset = scan->rs_cindex; targoffset <= maxoffset; targoffset++)
> +    for (targoffset = scan->rs_cindex ? scan->rs_cindex : FirstOffsetNumber;
> +            targoffset <= maxoffset;
> +            targoffset++)
>      {
>          ItemId        itemid;
>          HeapTuple    targtuple = &hslot->base.tupdata;

I thought it was better to fix the initialization for rs_cindex - any
reason you didn't go for that?


> diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
> index 8233475aa0..7bad246f55 100644
> --- a/src/backend/access/heap/heapam_visibility.c
> +++ b/src/backend/access/heap/heapam_visibility.c
> @@ -1838,8 +1838,10 @@ HeapTupleSatisfies(HeapTuple stup, Snapshot snapshot, Buffer buffer)
>          case NON_VACUUMABLE_VISIBILTY:
>              return HeapTupleSatisfiesNonVacuumable(stup, snapshot, buffer);
>              break;
> -        default:
> +        case END_OF_VISIBILITY:
>              Assert(0);
>              break;
>      }
> +
> +    return false; /* keep compiler quiet */

I don't understand why END_OF_VISIBILITY is good idea?  I now removed
END_OF_VISIBILITY, and the default case.



> @@ -593,6 +594,10 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
>      if (myState->rel->rd_rel->relhasoids)
>          slot->tts_tupleOid = InvalidOid;
>  
> +    /* Materialize the slot */
> +    if (!TTS_IS_VIRTUAL(slot))
> +        ExecMaterializeSlot(slot);
> +
>      table_insert(myState->rel,
>                   slot,
>                   myState->output_cid,

What's the point of adding materialization here?

> @@ -570,6 +563,9 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>                  Assert(TTS_IS_HEAPTUPLE(scanslot) ||
>                         TTS_IS_BUFFERTUPLE(scanslot));
>  
> +                if (hslot->tuple == NULL)
> +                    ExecMaterializeSlot(scanslot);
> +
>                  d = heap_getsysattr(hslot->tuple, attnum,
>                                      scanslot->tts_tupleDescriptor,
>                                      op->resnull);

Same?


> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> index e055c0a7c6..34ef86a5bd 100644
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -2594,7 +2594,7 @@ EvalPlanQual(EState *estate, EPQState *epqstate,
>       * datums that may be present in copyTuple).  As with the next step, this
>       * is to guard against early re-use of the EPQ query.
>       */
> -    if (!TupIsNull(slot))
> +    if (!TupIsNull(slot) && !TTS_IS_VIRTUAL(slot))
>          ExecMaterializeSlot(slot);


Same?


>  #if FIXME
> @@ -2787,16 +2787,7 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate)
>              if (isNull)
>                  continue;
>  
> -            elog(ERROR, "frak, need to implement ROW_MARK_COPY");
> -#ifdef FIXME
> -            // FIXME: this should just deform the tuple and store it as a
> -            // virtual one.
> -            tuple = table_tuple_by_datum(erm->relation, datum, erm->relid);
> -
> -            /* store tuple */
> -            EvalPlanQualSetTuple(epqstate, erm->rti, tuple);
> -#endif
> -
> +            ExecForceStoreHeapTupleDatum(datum, slot);
>          }
>      }
>  }

Cool.


> diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
> index 56880e3d16..36ca07beb2 100644
> --- a/src/backend/executor/nodeBitmapHeapscan.c
> +++ b/src/backend/executor/nodeBitmapHeapscan.c
> @@ -224,6 +224,18 @@ BitmapHeapNext(BitmapHeapScanState *node)
>  
>              BitmapAdjustPrefetchIterator(node, tbmres);
>  
> +            /*
> +             * Ignore any claimed entries past what we think is the end of the
> +             * relation.  (This is probably not necessary given that we got at
> +             * least AccessShareLock on the table before performing any of the
> +             * indexscans, but let's be safe.)
> +             */
> +            if (tbmres->blockno >= scan->rs_nblocks)
> +            {
> +                node->tbmres = tbmres = NULL;
> +                continue;
> +            }
> +

I moved this into the storage engine, there just was a minor bug
preventing the already existing check from taking effect. I don't think
we should expose this kind of thing to the outside of the storage
engine.


> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 54382aba88..ea48e1d6e8 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -4037,7 +4037,6 @@ CreateStatsStmt:
>   *
>   *****************************************************************************/
>  
> -// PBORKED: storage option
>  CreateAsStmt:
>          CREATE OptTemp TABLE create_as_target AS SelectStmt opt_with_data
>                  {
> @@ -4068,14 +4067,16 @@ CreateAsStmt:
>          ;
>  
>  create_as_target:
> -            qualified_name opt_column_list OptWith OnCommitOption OptTableSpace
> +            qualified_name opt_column_list table_access_method_clause
> +            OptWith OnCommitOption OptTableSpace
>                  {
>                      $$ = makeNode(IntoClause);
>                      $$->rel = $1;
>                      $$->colNames = $2;
> -                    $$->options = $3;
> -                    $$->onCommit = $4;
> -                    $$->tableSpaceName = $5;
> +                    $$->accessMethod = $3;
> +                    $$->options = $4;
> +                    $$->onCommit = $5;
> +                    $$->tableSpaceName = $6;
>                      $$->viewQuery = NULL;
>                      $$->skipData = false;        /* might get changed later */
>                  }
> @@ -4125,14 +4126,15 @@ CreateMatViewStmt:
>          ;
>  
>  create_mv_target:
> -            qualified_name opt_column_list opt_reloptions OptTableSpace
> +            qualified_name opt_column_list table_access_method_clause opt_reloptions OptTableSpace
>                  {
>                      $$ = makeNode(IntoClause);
>                      $$->rel = $1;
>                      $$->colNames = $2;
> -                    $$->options = $3;
> +                    $$->accessMethod = $3;
> +                    $$->options = $4;
>                      $$->onCommit = ONCOMMIT_NOOP;
> -                    $$->tableSpaceName = $4;
> +                    $$->tableSpaceName = $5;
>                      $$->viewQuery = NULL;        /* filled at analysis time */
>                      $$->skipData = false;        /* might get changed later */
>                  }

Cool. I wonder if we should also somehow support SELECT INTO w/ USING?
You've apparently started to do so with?


> diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
> index 47dd885c4e..a4094ca3f1 100644
> --- a/src/test/regress/expected/create_am.out
> +++ b/src/test/regress/expected/create_am.out
> @@ -99,3 +99,81 @@ HINT:  Use DROP ... CASCADE to drop the dependent objects too.
>  -- Drop access method cascade
>  DROP ACCESS METHOD gist2 CASCADE;
>  NOTICE:  drop cascades to index grect2ind2
> +-- Create a heap2 table am handler with heapam handler
> +CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
> +SELECT * FROM pg_am where amtype = 't';
> + amname |      amhandler       | amtype 
> +--------+----------------------+--------
> + heap   | heap_tableam_handler | t
> + heap2  | heap_tableam_handler | t
> +(2 rows)
> +
> +CREATE TABLE tbl_heap2(f1 int, f2 char(100)) using heap2;
> +INSERT INTO tbl_heap2 VALUES(generate_series(1,10), 'Test series');
> +SELECT count(*) FROM tbl_heap2;
> + count 
> +-------
> +    10
> +(1 row)
> +
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tbl_heap2';
> +  relname  | relkind | amname 
> +-----------+---------+--------
> + tbl_heap2 | r       | heap2
> +(1 row)
> +
> +-- create table as using heap2
> +CREATE TABLE tblas_heap2 using heap2 AS select * from tbl_heap2;
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tblas_heap2';
> +   relname   | relkind | amname 
> +-------------+---------+--------
> + tblas_heap2 | r       | heap2
> +(1 row)
> +
> +--
> +-- select into doesn't support new syntax, so it should be
> +-- default access method.
> +--
> +SELECT INTO tblselectinto_heap from tbl_heap2;
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tblselectinto_heap';
> +      relname       | relkind | amname 
> +--------------------+---------+--------
> + tblselectinto_heap | r       | heap
> +(1 row)
> +
> +DROP TABLE tblselectinto_heap;
> +-- create materialized view using heap2
> +CREATE MATERIALIZED VIEW mv_heap2 USING heap2 AS
> +        SELECT * FROM tbl_heap2;
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'mv_heap2';
> + relname  | relkind | amname 
> +----------+---------+--------
> + mv_heap2 | m       | heap2
> +(1 row)
> +
> +-- Try creating the unsupported relation kinds with using syntax
> +CREATE VIEW test_view USING heap2 AS SELECT * FROM tbl_heap2;
> +ERROR:  syntax error at or near "USING"
> +LINE 1: CREATE VIEW test_view USING heap2 AS SELECT * FROM tbl_heap2...
> +                              ^
> +CREATE SEQUENCE test_seq USING heap2;
> +ERROR:  syntax error at or near "USING"
> +LINE 1: CREATE SEQUENCE test_seq USING heap2;
> +                                 ^
> +-- Drop table access method, but fails as objects depends on it
> +DROP ACCESS METHOD heap2;
> +ERROR:  cannot drop access method heap2 because other objects depend on it
> +DETAIL:  table tbl_heap2 depends on access method heap2
> +table tblas_heap2 depends on access method heap2
> +materialized view mv_heap2 depends on access method heap2
> +HINT:  Use DROP ... CASCADE to drop the dependent objects too.
> +-- Drop table access method with cascade
> +DROP ACCESS METHOD heap2 CASCADE;
> +NOTICE:  drop cascades to 3 other objects
> +DETAIL:  drop cascades to table tbl_heap2
> +drop cascades to table tblas_heap2
> +drop cascades to materialized view mv_heap2
> diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
> index 3e0ac104f3..0472a60f20 100644
> --- a/src/test/regress/sql/create_am.sql
> +++ b/src/test/regress/sql/create_am.sql
> @@ -66,3 +66,49 @@ DROP ACCESS METHOD gist2;
>  
>  -- Drop access method cascade
>  DROP ACCESS METHOD gist2 CASCADE;
> +
> +-- Create a heap2 table am handler with heapam handler
> +CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
> +
> +SELECT * FROM pg_am where amtype = 't';
> +
> +CREATE TABLE tbl_heap2(f1 int, f2 char(100)) using heap2;
> +INSERT INTO tbl_heap2 VALUES(generate_series(1,10), 'Test series');
> +SELECT count(*) FROM tbl_heap2;
> +
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tbl_heap2';
> +
> +-- create table as using heap2
> +CREATE TABLE tblas_heap2 using heap2 AS select * from tbl_heap2;
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tblas_heap2';
> +
> +--
> +-- select into doesn't support new syntax, so it should be
> +-- default access method.
> +--
> +SELECT INTO tblselectinto_heap from tbl_heap2;
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'tblselectinto_heap';
> +
> +DROP TABLE tblselectinto_heap;
> +
> +-- create materialized view using heap2
> +CREATE MATERIALIZED VIEW mv_heap2 USING heap2 AS
> +        SELECT * FROM tbl_heap2;
> +
> +SELECT r.relname, r.relkind, a.amname from pg_class as r, pg_am as a
> +        where a.oid = r.relam AND r.relname = 'mv_heap2';
> +
> +-- Try creating the unsupported relation kinds with using syntax
> +CREATE VIEW test_view USING heap2 AS SELECT * FROM tbl_heap2;
> +
> +CREATE SEQUENCE test_seq USING heap2;
> +
> +
> +-- Drop table access method, but fails as objects depends on it
> +DROP ACCESS METHOD heap2;
> +
> +-- Drop table access method with cascade
> +DROP ACCESS METHOD heap2 CASCADE;
> -- 
> 2.18.0.windows.1

Nice!

Greetings,

Andres Freund