Re: Pluggable Storage - Andres's take

Hi,

On May 6, 2019 3:40:55 AM PDT, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>On Tue, 9 Apr 2019 at 15:17, Heikki Linnakangas <hlinnaka@iki.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?

I assume you got compiler warmings compiling it? The API for some callbacks changed a bit.

Andred
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.