Re: Pluggable Storage - Andres's take

From: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Asim R P <apraveen(at)pivotal(dot)io>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Ashwin Agrawal <aagrawal(at)pivotal(dot)io>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
Subject: Re: Pluggable Storage - Andres's take
Date: 2019-01-22 02:43:36
Message-ID: CAJrrPGfEUCObcUfNGp9ZsT90cDTmQ3DQ9WMJxWOBiX6oLdw=OA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0002-Removal-of-scan_update_snapshot-callback.patch application/octet-stream 4.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-01-22 02:45:31 Re: Allowing extensions to find out the OIDs of their member objects
Previous Message Imai, Yoshikazu 2019-01-22 02:32:39 RE: speeding up planning with partitions