Re: Pluggable Storage - Andres's take

From: Andres Freund <andres(at)anarazel(dot)de>
To: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
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 01:15:17
Message-ID: 20190122011517.s2pg5iklh6oye4dm@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Gierth 2019-01-22 01:26:58 Re: Allowing extensions to find out the OIDs of their member objects
Previous Message Haribabu Kommi 2019-01-22 00:51:57 Re: Pluggable Storage - Andres's take