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