From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Asim R P <apraveen@pivotal.io>
Cc: pgsql-hackers@postgresql.org, Ashwin Agrawal <aagrawal@pivotal.io>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Subject: Re: Pluggable Storage - Andres's take
Date: Tue, 9 Apr 2019 16:16:54 +0300
Message-ID: <e71c4da4-3e82-cc4f-32cc-ede387fac8b0@iki.fi> (raw)
In-Reply-To: <20190408173723.wnfni5t3g7ia7jit@alap3.anarazel.de>
References: <20190312.172757.17614556.horiguchi.kyotaro@lab.ntt.co.jp>
<CAJrrPGeUqNy0BxWuDd1n5cLQoWUwQ5jhKnDQ6Ty3wt=KsyOoPQ@mail.gmail.com>
<20190401231810.arpvmhw66bwsyobj@alap3.anarazel.de>
<CAJrrPGfSEeqdZve2X8zkRo2Bm=M-bQf8WiJEMBshVvtrZjO0Gg@mail.gmail.com>
<20190402005352.64hk76gddvs36uwl@alap3.anarazel.de>
<CAJrrPGcB0=UteRJwGMzESdRecc-DUAQBSXfwExmVU1TKwMb5=A@mail.gmail.com>
<20190402215743.cfiwzlk2szwq4jp3@alap3.anarazel.de>
<20190404055138.GA24864@telsasoft.com>
<20190405165101.mosiflz6slokm2oo@alap3.anarazel.de>
<9a7fb9cc-2419-5db7-8840-ddc10c93f122@iki.fi>
<20190408173723.wnfni5t3g7ia7jit@alap3.anarazel.de>
On 08/04/2019 20:37, Andres Freund wrote:
> On 2019-04-08 15:34:46 +0300, Heikki Linnakangas wrote:
>> There's a little bug in index-only scan executor node, where it mixes up the
>> slots to hold a tuple from the index, and from the table. That doesn't cause
>> any ill effects if the AM uses TTSOpsHeapTuple, but with my toy AM, which
>> uses a virtual slot, it caused warnings like this from index-only scans:
>
> Hm. That's another one that I think I had fixed previously :(, and then
> concluded that it's not actually necessary for some reason. Your fix
> looks correct to me. Do you want to commit it? Otherwise I'll look at
> it after rebasing zheap, and checking it with that.
I found another slot type confusion bug, while playing with zedstore. In
an Index Scan, if you have an ORDER BY key that needs to be rechecked,
so that it uses the reorder queue, then it will sometimes use the
reorder queue slot, and sometimes the table AM's slot, for the scan
slot. If they're not of the same type, you get an assertion:
TRAP: FailedAssertion("!(op->d.fetch.kind == slot->tts_ops)", File:
"execExprInterp.c", Line: 1905)
Attached is a test for this, again using the toy table AM, extended to
be able to test this. And a fix.
>> Attached is a patch with the toy implementation I used to test this. I'm not
>> suggesting we should commit that - although feel free to do that if you
>> think it's useful - but it shows how I bumped into these issues.
>
> Hm, probably not a bad idea to include something like it. It seems like
> we kinda would need non-stub implementation of more functions for it to
> test much / and to serve as an example. I'm mildy inclined to just do
> it via zheap / externally, but I'm not quite sure that's good enough.
Works for me.
>> +static Size
>> +toyam_parallelscan_estimate(Relation rel)
>> +{
>> + ereport(ERROR,
>> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> + errmsg("function %s not implemented yet", __func__)));
>> +}
>
> The other stubbed functions seem like we should require them, but I
> wonder if we should make the parallel stuff optional?
Yeah, that would be good. I would assume it to be optional.
- Heikki
Attachments:download all (4 files, .tar.gz)0001-Fix-confusion-on-different-kinds-of-slots-in-IndexOn.patch (text/x-patch, 2.5K) download
From e8854c876927b32e21f485337dd2335f4bfebd32 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 8 Apr 2019 15:18:19 +0300
Subject: [PATCH 1/4] Fix confusion on different kinds of slots in
IndexOnlyScans.
We used the same slot, to store a tuple from the index, and to store a
tuple from the table. That's not OK. It worked with the heap, because
heapam_getnextslot() stores a HeapTuple to the slot, and doesn't care how
large the tts_values/nulls arrays are. But when I played with a toy table
AM implementation that used a virtual tuple, it caused memory overruns.
---
src/backend/executor/nodeIndexonlyscan.c | 16 +++++++++++++---
src/include/nodes/execnodes.h | 1 +
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.cindex 7711728495c..5833d683b38 100644--- a/src/backend/executor/nodeIndexonlyscan.c+++ b/src/backend/executor/nodeIndexonlyscan.c@@ -166,10 +166,10 @@ IndexOnlyNext(IndexOnlyScanState *node)
* Rats, we have to visit the heap to check visibility.
*/
InstrCountTuples2(node, 1);
- if (!index_fetch_heap(scandesc, slot))+ if (!index_fetch_heap(scandesc, node->ioss_TableSlot))
continue; /* no visible tuple, try next index entry */
- ExecClearTuple(slot);+ ExecClearTuple(node->ioss_TableSlot);
/*
* Only MVCC snapshots are supported here, so there should be no
@@ -528,7 +528,17 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
*/
tupDesc = ExecTypeFromTL(node->indextlist);
ExecInitScanTupleSlot(estate, &indexstate->ss, tupDesc,
- table_slot_callbacks(currentRelation));+ &TTSOpsVirtual);++ /*+ * We need another slot, in a format that's suitable for the table AM,+ * for when we need to fetch a tuple from the table for rechecking+ * visibility.+ */+ indexstate->ioss_TableSlot =+ ExecAllocTableSlot(&estate->es_tupleTable,+ RelationGetDescr(currentRelation),+ table_slot_callbacks(currentRelation));
/*
* Initialize result type and projection info. The node's targetlist will
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.hindex a5e4b7ef2e0..108dee61e24 100644--- a/src/include/nodes/execnodes.h+++ b/src/include/nodes/execnodes.h@@ -1424,6 +1424,7 @@ typedef struct IndexOnlyScanState
struct IndexScanDescData *ioss_ScanDesc;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ TupleTableSlot *ioss_TableSlot;
} IndexOnlyScanState;
/* ----------------
--
2.20.1
0002-Add-a-toy-table-AM-implementation-to-play-with.patch (text/x-patch, 20.3K) download
From 309a773e09aa3d1256a431f2030f0f5819e6b32d Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 8 Apr 2019 15:16:53 +0300
Subject: [PATCH 2/4] Add a toy table AM implementation to play with.
It returns a constant data set. No insert/update/delete. But you can
create indexes.
---
src/test/modules/toytable/Makefile | 25 +
.../modules/toytable/expected/toytable.out | 41 ++
src/test/modules/toytable/sql/toytable.sql | 17 +
src/test/modules/toytable/toytable--1.0.sql | 12 +
src/test/modules/toytable/toytable.control | 4 +
src/test/modules/toytable/toytableam.c | 630 ++++++++++++++++++
6 files changed, 729 insertions(+)
create mode 100644 src/test/modules/toytable/Makefile
create mode 100644 src/test/modules/toytable/expected/toytable.out
create mode 100644 src/test/modules/toytable/sql/toytable.sql
create mode 100644 src/test/modules/toytable/toytable--1.0.sql
create mode 100644 src/test/modules/toytable/toytable.control
create mode 100644 src/test/modules/toytable/toytableam.c
diff --git a/src/test/modules/toytable/Makefile b/src/test/modules/toytable/Makefilenew file mode 100644index 00000000000..142ef2d23e6--- /dev/null+++ b/src/test/modules/toytable/Makefile@@ -0,0 +1,25 @@+# src/test/modules/toytable/Makefile++MODULE_big = toytable+OBJS = toytableam.o $(WIN32RES)+PGFILEDESC = "A dummy implementantation of the table AM API"++EXTENSION = toytable+DATA = toytable--1.0.sql++REGRESS = toytable++ifdef USE_PGXS+PG_CONFIG = pg_config+PGXS := $(shell $(PG_CONFIG) --pgxs)+include $(PGXS)+else+subdir = src/test/modules/toytable+top_builddir = ../../../..+include $(top_builddir)/src/Makefile.global+include $(top_srcdir)/contrib/contrib-global.mk+endif++OBJS = toytableam.o++include $(top_srcdir)/src/backend/common.mkdiff --git a/src/test/modules/toytable/expected/toytable.out b/src/test/modules/toytable/expected/toytable.outnew file mode 100644index 00000000000..3e8598e284c--- /dev/null+++ b/src/test/modules/toytable/expected/toytable.out@@ -0,0 +1,41 @@+CREATE EXTENSION toytable;+create table toytab (i int4, j int4, k int4) using toytable;+select * from toytab;+ i | j | k +----+----+----+ 1 | 1 | 1+ 2 | 2 | 2+ 3 | 3 | 3+ 4 | 4 | 4+ 5 | 5 | 5+ 6 | 6 | 6+ 7 | 7 | 7+ 8 | 8 | 8+ 9 | 9 | 9+ 10 | 10 | 10+(10 rows)++create index toyidx on toytab(i);+-- test index scan+set enable_seqscan=off;+set enable_indexscan=on;+select i, j from toytab where i = 4;+ i | j +---+---+ 4 | 4+(1 row)++-- index only scan+explain (costs off) select i from toytab where i = 4;+ QUERY PLAN +----------------------------------------+ Index Only Scan using toyidx on toytab+ Index Cond: (i = 4)+(2 rows)++select i from toytab where i = 4 ;+ i +---+ 4+(1 row)+diff --git a/src/test/modules/toytable/sql/toytable.sql b/src/test/modules/toytable/sql/toytable.sqlnew file mode 100644index 00000000000..8d9bac41bbf--- /dev/null+++ b/src/test/modules/toytable/sql/toytable.sql@@ -0,0 +1,17 @@+CREATE EXTENSION toytable;++create table toytab (i int4, j int4, k int4) using toytable;++select * from toytab;++create index toyidx on toytab(i);++-- test index scan+set enable_seqscan=off;+set enable_indexscan=on;++select i, j from toytab where i = 4;++-- index only scan+explain (costs off) select i from toytab where i = 4;+select i from toytab where i = 4 ;diff --git a/src/test/modules/toytable/toytable--1.0.sql b/src/test/modules/toytable/toytable--1.0.sqlnew file mode 100644index 00000000000..52085d27f4a--- /dev/null+++ b/src/test/modules/toytable/toytable--1.0.sql@@ -0,0 +1,12 @@+/* src/test/modules/toyam/toyam--1.0.sql */++-- complain if script is sourced in psql, rather than via CREATE EXTENSION+\echo Use "CREATE EXTENSION toytable" to load this file. \quit++CREATE FUNCTION toytableam_handler(internal)+RETURNS pg_catalog.table_am_handler STRICT+AS 'MODULE_PATHNAME' LANGUAGE C;++CREATE ACCESS METHOD toytable TYPE TABLE HANDLER toytableam_handler++diff --git a/src/test/modules/toytable/toytable.control b/src/test/modules/toytable/toytable.controlnew file mode 100644index 00000000000..8f613e58d6e--- /dev/null+++ b/src/test/modules/toytable/toytable.control@@ -0,0 +1,4 @@+comment = 'Dummy implementation of table AM api'+default_version = '1.0'+module_pathname = '$libdir/toytable'+relocatable = truediff --git a/src/test/modules/toytable/toytableam.c b/src/test/modules/toytable/toytableam.cnew file mode 100644index 00000000000..4cb2b5d75db--- /dev/null+++ b/src/test/modules/toytable/toytableam.c@@ -0,0 +1,630 @@+/*-------------------------------------------------------------------------+ *+ * toyam_handler.c+ * a toy table access method code+ *+ * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ *+ * IDENTIFICATION+ * src/backend/access/toytable/toyam_handler.c+ *+ *-------------------------------------------------------------------------+ */+#include "postgres.h"++#include <math.h>++#include "miscadmin.h"++#include "access/multixact.h"+#include "access/relscan.h"+#include "access/tableam.h"+#include "catalog/catalog.h"+#include "catalog/storage.h"+#include "catalog/index.h"+#include "catalog/pg_type.h"+#include "executor/executor.h"+#include "utils/builtins.h"+#include "utils/rel.h"+#include "storage/bufmgr.h"++PG_MODULE_MAGIC;++PG_FUNCTION_INFO_V1(toytableam_handler);++typedef struct+{+ TableScanDescData scan;++ int tupidx;+} ToyScanDescData;+typedef ToyScanDescData *ToyScanDesc;++static const TupleTableSlotOps *+toyam_slot_callbacks(Relation relation)+{+ return &TTSOpsVirtual;+}++static TableScanDesc toyam_scan_begin(Relation rel,+ Snapshot snapshot,+ int nkeys, struct ScanKeyData *key,+ ParallelTableScanDesc pscan,+ bool allow_strat,+ bool allow_sync,+ bool allow_pagemode,+ bool is_bitmapscan,+ bool is_samplescan,+ bool temp_snap)+{+ ToyScanDesc tscan;++ tscan = palloc0(sizeof(ToyScanDescData));+ tscan->scan.rs_rd = rel;+ tscan->scan.rs_snapshot = snapshot;+ tscan->scan.rs_nkeys = nkeys;+ tscan->scan.rs_bitmapscan = is_bitmapscan;+ tscan->scan.rs_samplescan = is_samplescan;+ tscan->scan.rs_allow_strat = allow_strat;+ tscan->scan.rs_allow_sync = allow_sync;+ tscan->scan.rs_temp_snap = temp_snap;+ tscan->scan.rs_parallel = pscan;++ tscan->tupidx = 0;++ return &tscan->scan;+}++static void+toyam_scan_end(TableScanDesc scan)+{+ pfree(scan);+}++static void+toyam_scan_rescan(TableScanDesc scan, struct ScanKeyData *key,+ bool set_params, bool allow_strat,+ bool allow_sync, bool allow_pagemode)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static Datum+invent_toy_data(Oid atttypid, int idx, bool *isnull_p)+{+ Datum d;+ bool isnull;++ switch (atttypid)+ {+ case INT4OID:+ d = Int32GetDatum(idx);+ isnull = false;+ break;++ case BOXOID:+ d = DirectFunctionCall1(box_in,+ CStringGetDatum(psprintf("(%d,%d),(%d,%d)",+ idx - 10, idx - 10, idx+10, idx+10)));+ isnull = false;+ break;++ case POLYGONOID:+ d = DirectFunctionCall1(poly_in,+ CStringGetDatum(psprintf("%d,%d,%d,%d", 0,0, idx, idx)));+ isnull = false;+ break;++ default:+ d = (Datum) 0;+ isnull = true;+ break;+ }++ *isnull_p = isnull;+ return d;+}++static bool+toyam_scan_getnextslot(TableScanDesc scan,+ ScanDirection direction,+ TupleTableSlot *slot)+{+ ToyScanDesc tscan = (ToyScanDesc) scan;++ slot->tts_nvalid = 0;+ slot->tts_flags |= TTS_FLAG_EMPTY;++ /*+ * Return a constant 1 rows. Every int4 attribute gets+ * a running count, everything else is NULL.+ */+ if (tscan->tupidx < 10)+ {+ TupleDesc desc = RelationGetDescr(tscan->scan.rs_rd);++ tscan->tupidx++;++ for (AttrNumber attno = 1; attno <= desc->natts; attno++)+ {+ Form_pg_attribute att = &desc->attrs[attno - 1];+ Datum d;+ bool isnull;++ d = invent_toy_data(att->atttypid, tscan->tupidx, &isnull);++ slot->tts_values[attno - 1] = d;+ slot->tts_isnull[attno - 1] = isnull;+ }++ ItemPointerSet(&slot->tts_tid, 1, tscan->tupidx);+ slot->tts_nvalid = slot->tts_tupleDescriptor->natts;+ slot->tts_flags &= ~TTS_FLAG_EMPTY;++ return true;+ }+ else+ return false;+}++static Size+toyam_parallelscan_estimate(Relation rel)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static Size+toyam_parallelscan_initialize(Relation rel,+ ParallelTableScanDesc pscan)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_parallelscan_reinitialize(Relation rel,+ ParallelTableScanDesc pscan)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static struct IndexFetchTableData *+toyam_index_fetch_begin(Relation rel)+{+ IndexFetchTableData *tfetch = palloc0(sizeof(IndexFetchTableData));++ tfetch->rel = rel;++ return tfetch;+}++static void+toyam_index_fetch_reset(struct IndexFetchTableData *data)+{+}++static void+toyam_index_fetch_end(struct IndexFetchTableData *data)+{+ pfree(data);+}++static bool+toyam_index_fetch_tuple(struct IndexFetchTableData *scan,+ ItemPointer tid,+ Snapshot snapshot,+ TupleTableSlot *slot,+ bool *call_again, bool *all_dead)+{+ TupleDesc desc = RelationGetDescr(scan->rel);+ int tupidx;++ if (ItemPointerGetBlockNumber(tid) != 1)+ return false;++ tupidx = ItemPointerGetOffsetNumber(tid);+ if (tupidx < 1 || tupidx > 10)+ return false;++ slot->tts_nvalid = 0;+ slot->tts_flags |= TTS_FLAG_EMPTY;++ /* Return same data as toyam_scan_getnextslot does */+ for (AttrNumber attno = 1; attno <= desc->natts; attno++)+ {+ Form_pg_attribute att = &desc->attrs[attno - 1];+ Datum d;+ bool isnull;++ d = invent_toy_data(att->atttypid, tupidx, &isnull);++ slot->tts_values[attno - 1] = d;+ slot->tts_isnull[attno - 1] = isnull;+ }++ ItemPointerSet(&slot->tts_tid, 1, tupidx);+ slot->tts_nvalid = slot->tts_tupleDescriptor->natts;+ slot->tts_flags &= ~TTS_FLAG_EMPTY;++ return true;+}++static bool+toyam_tuple_fetch_row_version(Relation rel,+ ItemPointer tid,+ Snapshot snapshot,+ TupleTableSlot *slot)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_tuple_get_latest_tid(Relation rel,+ Snapshot snapshot,+ ItemPointer tid)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static bool+toyam_tuple_satisfies_snapshot(Relation rel,+ TupleTableSlot *slot,+ Snapshot snapshot)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static TransactionId+toyam_compute_xid_horizon_for_tuples(Relation rel,+ ItemPointerData *items,+ int nitems)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_tuple_insert(Relation rel, TupleTableSlot *slot,+ CommandId cid, int options,+ struct BulkInsertStateData *bistate)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_tuple_insert_speculative(Relation rel,+ TupleTableSlot *slot,+ CommandId cid,+ int options,+ struct BulkInsertStateData *bistate,+ uint32 specToken)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_tuple_complete_speculative(Relation rel,+ TupleTableSlot *slot,+ uint32 specToken,+ bool succeeded)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static TM_Result+toyam_tuple_delete(Relation rel,+ ItemPointer tid,+ CommandId cid,+ Snapshot snapshot,+ Snapshot crosscheck,+ bool wait,+ TM_FailureData *tmfd,+ bool changingPart)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_multi_insert(Relation rel, TupleTableSlot **slots, int nslots,+ CommandId cid, int options, struct BulkInsertStateData *bistate)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static TM_Result+toyam_tuple_update(Relation rel,+ ItemPointer otid,+ TupleTableSlot *slot,+ CommandId cid,+ Snapshot snapshot,+ Snapshot crosscheck,+ bool wait,+ TM_FailureData *tmfd,+ LockTupleMode *lockmode,+ bool *update_indexes)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static TM_Result+toyam_tuple_lock(Relation rel,+ ItemPointer tid,+ Snapshot snapshot,+ TupleTableSlot *slot,+ CommandId cid,+ LockTupleMode mode,+ LockWaitPolicy wait_policy,+ uint8 flags,+ TM_FailureData *tmfd)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_finish_bulk_insert(Relation rel, int options)+{+ return;+}++static void+toyam_relation_set_new_filenode(Relation rel,+ char persistence,+ TransactionId *freezeXid,+ MultiXactId *minmulti)+{+ *freezeXid = InvalidTransactionId;+ *minmulti = InvalidMultiXactId;++ /*+ * FIXME: We don't need this for anything. But index build calls+ * RelationGetNumberOfBlocks, from index_update_stats(), and that+ * fails if the underlying file doesn't exist.+ */+ RelationCreateStorage(rel->rd_node, persistence);+}++static void+toyam_relation_nontransactional_truncate(Relation rel)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_relation_copy_data(Relation rel, RelFileNode newrnode)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_relation_copy_for_cluster(Relation NewHeap,+ Relation OldHeap,+ Relation OldIndex,+ bool use_sort,+ TransactionId OldestXmin,+ TransactionId FreezeXid,+ MultiXactId MultiXactCutoff,+ double *num_tuples,+ double *tups_vacuumed,+ double *tups_recently_dead)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_relation_vacuum(Relation onerel,+ struct VacuumParams *params,+ BufferAccessStrategy bstrategy)+{+ /* we've got nothing to do */+}++static bool+toyam_scan_analyze_next_block(TableScanDesc scan,+ BlockNumber blockno,+ BufferAccessStrategy bstrategy)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static bool+toyam_scan_analyze_next_tuple(TableScanDesc scan,+ TransactionId OldestXmin,+ double *liverows,+ double *deadrows,+ TupleTableSlot *slot)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static double+toyam_index_build_range_scan(Relation heap_rel,+ Relation index_rel,+ struct IndexInfo *index_nfo,+ bool allow_sync,+ bool anyvisible,+ bool progress,+ BlockNumber start_blockno,+ BlockNumber end_blockno,+ IndexBuildCallback callback,+ void *callback_state,+ TableScanDesc scan)+{+ TupleTableSlot *slot;+ EState *estate;++ estate = CreateExecutorState();+ slot = table_slot_create(heap_rel, NULL);++ if (!scan)+ scan = toyam_scan_begin(heap_rel,+ SnapshotAny,+ 0, NULL,+ NULL,+ false,+ false,+ false,+ false,+ false,+ false);++ while (toyam_scan_getnextslot(scan, ForwardScanDirection, slot))+ {+ Datum values[INDEX_MAX_KEYS];+ bool isnull[INDEX_MAX_KEYS];+ HeapTuple heapTuple;++ FormIndexDatum(index_nfo, slot, estate, values, isnull);++ /* Call the AM's callback routine to process the tuple */+ heapTuple = ExecCopySlotHeapTuple(slot);+ heapTuple->t_self = slot->tts_tid;+ callback(index_rel, heapTuple, values, isnull, true,+ callback_state);+ pfree(heapTuple);+ }++ toyam_scan_end(scan);+ ExecDropSingleTupleTableSlot(slot);+ FreeExecutorState(estate);++ return 10;+}++static void+toyam_index_validate_scan(Relation heap_rel,+ Relation index_rel,+ struct IndexInfo *index_info,+ Snapshot snapshot,+ struct ValidateIndexState *state)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static void+toyam_relation_estimate_size(Relation rel, int32 *attr_widths,+ BlockNumber *pages, double *tuples,+ double *allvisfrac)+{+ *pages = 1;+ *tuples = 1;+ *allvisfrac = 1.0;+}++static bool+toyam_scan_sample_next_block(TableScanDesc scan,+ struct SampleScanState *scanstate)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static bool+toyam_scan_sample_next_tuple(TableScanDesc scan,+ struct SampleScanState *scanstate,+ TupleTableSlot *slot)+{+ ereport(ERROR,+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),+ errmsg("function %s not implemented yet", __func__)));+}++static const TableAmRoutine toyam_methods = {+ .type = T_TableAmRoutine,++ .slot_callbacks = toyam_slot_callbacks,++ .scan_begin = toyam_scan_begin,+ .scan_end = toyam_scan_end,+ .scan_rescan = toyam_scan_rescan,+ .scan_getnextslot = toyam_scan_getnextslot,++ .parallelscan_estimate = toyam_parallelscan_estimate,+ .parallelscan_initialize = toyam_parallelscan_initialize,+ .parallelscan_reinitialize = toyam_parallelscan_reinitialize,++ .index_fetch_begin = toyam_index_fetch_begin,+ .index_fetch_reset = toyam_index_fetch_reset,+ .index_fetch_end = toyam_index_fetch_end,+ .index_fetch_tuple = toyam_index_fetch_tuple,++ .tuple_fetch_row_version = toyam_tuple_fetch_row_version,+ .tuple_get_latest_tid = toyam_tuple_get_latest_tid,+ .tuple_satisfies_snapshot = toyam_tuple_satisfies_snapshot,+ .compute_xid_horizon_for_tuples = toyam_compute_xid_horizon_for_tuples,++ .tuple_insert = toyam_tuple_insert,+ .tuple_insert_speculative = toyam_tuple_insert_speculative,+ .tuple_complete_speculative = toyam_tuple_complete_speculative,+ .multi_insert = toyam_multi_insert,+ .tuple_delete = toyam_tuple_delete,+ .tuple_update = toyam_tuple_update,+ .tuple_lock = toyam_tuple_lock,+ .finish_bulk_insert = toyam_finish_bulk_insert,++ .relation_set_new_filenode = toyam_relation_set_new_filenode,+ .relation_nontransactional_truncate = toyam_relation_nontransactional_truncate,+ .relation_copy_data = toyam_relation_copy_data,+ .relation_copy_for_cluster = toyam_relation_copy_for_cluster,+ .relation_vacuum = toyam_relation_vacuum,++ .scan_analyze_next_block = toyam_scan_analyze_next_block,+ .scan_analyze_next_tuple = toyam_scan_analyze_next_tuple,+ .index_build_range_scan = toyam_index_build_range_scan,+ .index_validate_scan = toyam_index_validate_scan,++ .relation_estimate_size = toyam_relation_estimate_size,++ .scan_bitmap_next_block = NULL,+ .scan_bitmap_next_tuple = NULL,+ .scan_sample_next_block = toyam_scan_sample_next_block,+ .scan_sample_next_tuple = toyam_scan_sample_next_tuple,+};++Datum+toytableam_handler(PG_FUNCTION_ARGS)+{+ PG_RETURN_POINTER(&toyam_methods);+}--
2.20.1
0003-Add-test-for-bug-with-index-reorder-queue-slot-type-.patch (text/x-patch, 5.4K) download
From 56bd50a63afc4c2c3f31f26a0411bf22c54bbb6f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 9 Apr 2019 15:36:59 +0300
Subject: [PATCH 3/4] Add test for bug with index reorder queue slot type
confusion.
This currently fails an assertion.
---
src/test/modules/toytable/Makefile | 2 +-
.../toytable/expected/index-reorder-slot.out | 25 ++++++++++++
.../toytable/sql/index-reorder-slot.sql | 15 +++++++
src/test/modules/toytable/toytableam.c | 40 ++++++++++++++++---
4 files changed, 76 insertions(+), 6 deletions(-)
create mode 100644 src/test/modules/toytable/expected/index-reorder-slot.out
create mode 100644 src/test/modules/toytable/sql/index-reorder-slot.sql
diff --git a/src/test/modules/toytable/Makefile b/src/test/modules/toytable/Makefileindex 142ef2d23e6..a763ddacc89 100644--- a/src/test/modules/toytable/Makefile+++ b/src/test/modules/toytable/Makefile@@ -7,7 +7,7 @@ PGFILEDESC = "A dummy implementantation of the table AM API"
EXTENSION = toytable
DATA = toytable--1.0.sql
-REGRESS = toytable+REGRESS = toytable index-reorder-slot
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/toytable/expected/index-reorder-slot.out b/src/test/modules/toytable/expected/index-reorder-slot.outnew file mode 100644index 00000000000..52a1ccec850--- /dev/null+++ b/src/test/modules/toytable/expected/index-reorder-slot.out@@ -0,0 +1,25 @@+create extension toytable;+ERROR: extension "toytable" already exists+set toytable.num_tuples = 10000;+-- This is a variant of the test in 'polygon' regression test, but using+-- the toy table. It used to fail an assertion, because index scan would+-- sometimes use the reorderqueue's slot, which is a heaptuple slot, and+-- sometimes the table's scan slot, which is a virtual slot with toytable+CREATE TABLE quad_poly_toy (id int, p polygon) USING toytable;+CREATE INDEX quad_poly_toy_idx ON quad_poly_toy USING spgist(p);+set enable_seqscan=off;+SELECT * FROM quad_poly_toy ORDER BY p <-> point '123,456' LIMIT 10;+ id | p +-----+-----------------------+ 284 | ((284,284),(294,294))+ 283 | ((283,283),(293,293))+ 287 | ((287,287),(297,297))+ 286 | ((286,286),(296,296))+ 280 | ((280,280),(290,290))+ 289 | ((289,289),(299,299))+ 288 | ((288,288),(298,298))+ 285 | ((285,285),(295,295))+ 281 | ((281,281),(291,291))+ 282 | ((282,282),(292,292))+(10 rows)+diff --git a/src/test/modules/toytable/sql/index-reorder-slot.sql b/src/test/modules/toytable/sql/index-reorder-slot.sqlnew file mode 100644index 00000000000..7154639aa27--- /dev/null+++ b/src/test/modules/toytable/sql/index-reorder-slot.sql@@ -0,0 +1,15 @@+create extension toytable;++set toytable.num_tuples = 10000;++-- This is a variant of the test in 'polygon' regression test, but using+-- the toy table. It used to fail an assertion, because index scan would+-- sometimes use the reorderqueue's slot, which is a heaptuple slot, and+-- sometimes the table's scan slot, which is a virtual slot with toytable+CREATE TABLE quad_poly_toy (id int, p polygon) USING toytable;++CREATE INDEX quad_poly_toy_idx ON quad_poly_toy USING spgist(p);++set enable_seqscan=off;++SELECT * FROM quad_poly_toy ORDER BY p <-> point '123,456' LIMIT 10;diff --git a/src/test/modules/toytable/toytableam.c b/src/test/modules/toytable/toytableam.cindex 4cb2b5d75db..35ce6ce4709 100644--- a/src/test/modules/toytable/toytableam.c+++ b/src/test/modules/toytable/toytableam.c@@ -30,10 +30,39 @@
#include "utils/rel.h"
#include "storage/bufmgr.h"
+/*+ * Number of rows the fake table data will include.+ * Can be set with the toytable.num_tuples GUC.+ */+int num_toy_tuples;+
PG_MODULE_MAGIC;
PG_FUNCTION_INFO_V1(toytableam_handler);
+void _PG_init(void);+++/*+ * Module Load Callback+ */+void+_PG_init(void)+{+ /* Define custom GUC variables */+ DefineCustomIntVariable("toytable.num_tuples",+ "Number of tuples the in the fake table data",+ NULL,+ &num_toy_tuples,+ 10,+ 0, INT_MAX / 1000,+ PGC_USERSET,+ 0,+ NULL,+ NULL,+ NULL);+}+
typedef struct
{
TableScanDescData scan;
@@ -109,13 +138,14 @@ invent_toy_data(Oid atttypid, int idx, bool *isnull_p)
case BOXOID:
d = DirectFunctionCall1(box_in,
CStringGetDatum(psprintf("(%d,%d),(%d,%d)",
- idx - 10, idx - 10, idx+10, idx+10)));+ idx, idx, idx+10, idx+10)));
isnull = false;
break;
case POLYGONOID:
d = DirectFunctionCall1(poly_in,
- CStringGetDatum(psprintf("%d,%d,%d,%d", 0,0, idx, idx)));+ CStringGetDatum(psprintf("%d,%d,%d,%d",+ idx,idx, idx+10, idx+10)));
isnull = false;
break;
@@ -143,7 +173,7 @@ toyam_scan_getnextslot(TableScanDesc scan,
* Return a constant 1 rows. Every int4 attribute gets
* a running count, everything else is NULL.
*/
- if (tscan->tupidx < 10)+ if (tscan->tupidx < num_toy_tuples)
{
TupleDesc desc = RelationGetDescr(tscan->scan.rs_rd);
@@ -232,7 +262,7 @@ toyam_index_fetch_tuple(struct IndexFetchTableData *scan,
return false;
tupidx = ItemPointerGetOffsetNumber(tid);
- if (tupidx < 1 || tupidx > 10)+ if (tupidx < 1 || tupidx > num_toy_tuples)
return false;
slot->tts_nvalid = 0;
@@ -527,7 +557,7 @@ toyam_index_build_range_scan(Relation heap_rel,
ExecDropSingleTupleTableSlot(slot);
FreeExecutorState(estate);
- return 10;+ return num_toy_tuples;
}
static void
--
2.20.1
0004-Fix-confusion-on-type-of-slot-used-for-Index-Scan-s-.patch (text/x-patch, 1.9K) download
From 41ee331aeb7e81d367f6dcf6796170e896b71d6d Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 9 Apr 2019 15:37:29 +0300
Subject: [PATCH 4/4] Fix confusion on type of slot used for Index Scan's
projections.
If there are ORDER BY keys, so that the index scan node needs to use the
reorder queue, then it wil sometimes use the underlying table's slot
directly, and sometimes the reorder queue's slot, for evaluating
projections and quals. If they are not both of the same type, then we
must not set the 'scanopsfixed' flag.
---
src/backend/executor/nodeIndexscan.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.cindex 399ac0109c3..21744b219c9 100644--- a/src/backend/executor/nodeIndexscan.c+++ b/src/backend/executor/nodeIndexscan.c@@ -901,6 +901,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
{
IndexScanState *indexstate;
Relation currentRelation;
+ const TupleTableSlotOps *table_slot_ops;
LOCKMODE lockmode;
/*
@@ -927,11 +928,19 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
indexstate->ss.ss_currentScanDesc = NULL; /* no heap scan here */
/*
- * get the scan type from the relation descriptor.+ * Initialize the scan slot.+ *+ * With the reorder queue, we will sometimes use the reorderqueue's slot,+ * which uses heap ops, and sometimes the table AM's slot directly. We+ * have to set scanopsfixed to false, unless the table AM also uses heap+ * ops.
*/
+ table_slot_ops = table_slot_callbacks(currentRelation);
ExecInitScanTupleSlot(estate, &indexstate->ss,
RelationGetDescr(currentRelation),
- table_slot_callbacks(currentRelation));+ table_slot_ops);+ if (node->indexorderby && table_slot_ops != &TTSOpsHeapTuple)+ indexstate->ss.ps.scanopsfixed = false;
/*
* Initialize result type and projection.
--
2.20.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, andres@anarazel.de, pryzby@telsasoft.com, kommi.haribabu@gmail.com, horiguchi.kyotaro@lab.ntt.co.jp, alvherre@2ndquadrant.com, david.rowley@2ndquadrant.com, robertmhaas@gmail.com, apraveen@pivotal.io, aagrawal@pivotal.io, 9erthalion6@gmail.com, a.korotkov@postgrespro.ru
Subject: Re: Pluggable Storage - Andres's take
In-Reply-To: <e71c4da4-3e82-cc4f-32cc-ede387fac8b0@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox