Re: Pluggable Storage - Andres's take

Hi,

Thanks!

On 2019-01-22 11:51:57 +1100, Haribabu Kommi wrote:
> Attached the patch for removal of scan_update_snapshot
> and also the rebased patch of reduction in use of t_tableOid.

I'll soon look at the latter.

> > - consider removing table_gimmegimmeslot()
> > - add substantial docs for every callback
> >
>
> Will work on the above two.

I think it's easier if I do the first, because I can just do it while
rebasing, reducing unnecessary conflicts.

> > While I saw an initial attempt at writing smgl docs for the table AM
> > API, I'm not convinced that's the best approach. I think it might make
> > more sense to have high-level docs in sgml, but then do all the
> > per-callback docs in tableam.h.
> >
>
> OK, I will update the sgml docs accordingly.
> Index AM has per callback docs in the sgml, refactor them also?

I don't think it's a good idea to tackle the index docs at the same time
- this patchset is already humongously large...

> diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
> index 62c5f9fa9f..3dc1444739 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2308,7 +2308,6 @@ static const TableAmRoutine heapam_methods = {
> .scan_begin = heap_beginscan,
> .scan_end = heap_endscan,
> .scan_rescan = heap_rescan,
> - .scan_update_snapshot = heap_update_snapshot,
> .scan_getnextslot = heap_getnextslot,
>
> .parallelscan_estimate = table_block_parallelscan_estimate,
> diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
> index 59061c746b..b48ab5036c 100644
> --- a/src/backend/executor/nodeBitmapHeapscan.c
> +++ b/src/backend/executor/nodeBitmapHeapscan.c
> @@ -954,5 +954,9 @@ ExecBitmapHeapInitializeWorker(BitmapHeapScanState *node,
> node->pstate = pstate;
>
> snapshot = RestoreSnapshot(pstate->phs_snapshot_data);
> - table_scan_update_snapshot(node->ss.ss_currentScanDesc, snapshot);
> + Assert(IsMVCCSnapshot(snapshot));
> +
> + RegisterSnapshot(snapshot);
> + node->ss.ss_currentScanDesc->rs_snapshot = snapshot;
> + node->ss.ss_currentScanDesc->rs_temp_snap = true;
> }

I was rather thinking that we'd just move this logic into
table_scan_update_snapshot(), without it invoking a callback.

Greetings,

Andres Freund