From: Andres Freund <andres@anarazel.de>
To: Ashwin Agrawal <aagrawal@pivotal.io>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	Haribabu Kommi <kommi.haribabu@gmail.com>,
	Asim R P <apraveen@pivotal.io>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>
Subject: Re: Pluggable Storage - Andres's take
In-Reply-To: 
 <CALfoeis=ha=oyqHoBd0vqBpi+7pgyBmyh89ABbvWmS00wBgz8w@mail.gmail.com>
Reply-To: 
Date: Tue, 05 Mar 2019 18:33:54 -0800
Message-ID: <874l8gn33h.fsf@alap4.lan>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi,

Thanks for looking!

On 2019-03-05 18:27:45 -0800, Ashwin Agrawal wrote:
> While playing with the tableam, usage of which starts with commit
> v12-0023-tableam-Introduce-and-use-begin-endscan-and-do-i.patch, should we
> check for NULL function pointer before actually calling the same and ERROR
> out instead as NOT_SUPPORTED or something on those lines.

Scans seem like absolutely required part of the functionality, so I
don't think there's much point in that. It'd just bloat code and
runtime.


> Understand its kind of think which should get caught during development.
> But still currently it segfaults if missing to define some AM function,

The segfault iself doesn't bother me at all, it's just a NULL pointer
dereference. If we were to put Asserts somewhere it'd crash very
similarly. I think you have a point in that:

> might be easier for iterative development to error instead in common place.

Would make it a tiny bit easier to implement a new AM.  We could
probably add a few asserts to GetTableAmRoutine(), to check that
required functions are implemted.  Don't think that'd make a meaningful
difference for something like the scan functions, but it'd probably make
it easier to forward port AMs to the next release - I'm pretty sure
we're going to add required callbacks in the next few releases.

Greetings,

Andres Freund