Re: Pluggable Storage - Andres's take

On Tue, 9 Apr 2019 at 15:17, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 08/04/2019 20:37, Andres Freund wrote:
> > On 2019-04-08 15:34:46 +0300, Heikki Linnakangas wrote:
> >> There's a little bug in index-only scan executor node, where it mixes up the
> >> slots to hold a tuple from the index, and from the table. That doesn't cause
> >> any ill effects if the AM uses TTSOpsHeapTuple, but with my toy AM, which
> >> uses a virtual slot, it caused warnings like this from index-only scans:
> >
> > Hm. That's another one that I think I had fixed previously :(, and then
> > concluded that it's not actually necessary for some reason. Your fix
> > looks correct to me. Do you want to commit it? Otherwise I'll look at
> > it after rebasing zheap, and checking it with that.
>
> I found another slot type confusion bug, while playing with zedstore. In
> an Index Scan, if you have an ORDER BY key that needs to be rechecked,
> so that it uses the reorder queue, then it will sometimes use the
> reorder queue slot, and sometimes the table AM's slot, for the scan
> slot. If they're not of the same type, you get an assertion:
>
> TRAP: FailedAssertion("!(op->d.fetch.kind == slot->tts_ops)", File:
> "execExprInterp.c", Line: 1905)
>
> Attached is a test for this, again using the toy table AM, extended to
> be able to test this. And a fix.
>
> >> Attached is a patch with the toy implementation I used to test this. I'm not
> >> suggesting we should commit that - although feel free to do that if you
> >> think it's useful - but it shows how I bumped into these issues.
> >
> > Hm, probably not a bad idea to include something like it. It seems like
> > we kinda would need non-stub implementation of more functions for it to
> > test much / and to serve as an example. I'm mildy inclined to just do
> > it via zheap / externally, but I'm not quite sure that's good enough.
>
> Works for me.
>
> >> +static Size
> >> +toyam_parallelscan_estimate(Relation rel)
> >> +{
> >> + ereport(ERROR,
> >> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> >> + errmsg("function %s not implemented yet", __func__)));
> >> +}
> >
> > The other stubbed functions seem like we should require them, but I
> > wonder if we should make the parallel stuff optional?
>
> Yeah, that would be good. I would assume it to be optional.
>
I was trying the toyam patch and on make check it failed with
segmentation fault at

static void
toyam_relation_set_new_filenode(Relation rel,
char persistence,
TransactionId *freezeXid,
MultiXactId *minmulti)
{
*freezeXid = InvalidTransactionId;

Basically, on running create table t (i int, j int) using toytable,
leads to this segmentation fault.

Am I missing something here?

--
Regards,
Rafia Sabih