Re: Pluggable Storage - Andres's take

On Tue, Jan 22, 2019 at 12:15 PM Andres Freund <andres(at)anarazel(dot)de> wrote:

> 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.
>

Thanks.

>
> > > - 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.
>
>
OK. I will work on the doc changes.

> > > 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...
>

OK.

>
> > 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.
>

OK. Changed accordingly.
But the table_scan_update_snapshot() function is moved into tableam.c,
to avoid additional header file snapmgr.h inclusion in tableam.h

Regards,
Haribabu Kommi
Fujitsu Australia