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