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