Subject: Re: Pluggable Storage - Andres's take
To: Andres Freund <andres@anarazel.de>,
 Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Asim R P <apraveen@pivotal.io>, Pg Hackers
 <pgsql-hackers@postgresql.org>, aagrawal@pivotal.io,
 Dmitry Dolgov <9erthalion6@gmail.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>
References: 
 <CAJrrPGcpAH60ut6hAtFMi3ewymMJnEXvA=1QKK+LR7vJy4ZRXA@mail.gmail.com>
 <CA+q6zcW=st1p9R1piOV6sgAEC3oDWYi9-96sPKJ0-4uTigGw1A@mail.gmail.com>
 <CAJrrPGfHrQeGzqZwd=Bj96P9aubDH67kO8gf5mvynvRxXucq2g@mail.gmail.com>
 <CA+q6zcXwJ8m0EvGONjnwE=xESJe1z5NPh43Q8kS8j+1o2M0agg@mail.gmail.com>
 <CAJrrPGc5xE_sJbGNZJ7h8d5kWwS-0SspUKa2Vmi+nwEaBUYQFw@mail.gmail.com>
 <CAJrrPGfS-ax=dySxD_B5Vg6sjXCSqN-9g7kmV8Oq_vsEST7uAw@mail.gmail.com>
 <CANXE4TfZUZCg+afo+gpqiHF6O=rDfqt5a+3jTH2SD8FZ9Yd2pA@mail.gmail.com>
 <CAJrrPGc66Bcv5_Lkw+6yqSzptEw2mh8o=JH05poNh4BZYuGN2w@mail.gmail.com>
 <20181127015557.unah7opawwrvr3us@alap3.anarazel.de>
 <20181211021340.mqaown4njtcgrjvr@alap3.anarazel.de>
 <20190121030115.unoxypsx3dxosyeg@alap3.anarazel.de>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <cb45bbcd-6b5b-e71d-d9cf-f113d57b7498@iki.fi>
Date: Wed, 27 Feb 2019 18:00:12 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
MIME-Version: 1.0
In-Reply-To: <20190121030115.unoxypsx3dxosyeg@alap3.anarazel.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

I haven't been following this thread closely, but I looked briefly at 
some of the patches posted here:

On 21/01/2019 11:01, Andres Freund wrote:
> The patchset is now pretty granularly split into individual pieces.

Wow, 42 patches, very granular indeed! That's nice for reviewing, but 
are you planning to squash them before committing? Seems a bit excessive 
for the git history.

Patches 1-4:

* v12-0001-WIP-Introduce-access-table.h-access-relation.h.patch
* v12-0002-Replace-heapam.h-includes-with-relation.h-table..patch
* v12-0003-Replace-uses-of-heap_open-et-al-with-table_open-.patch
* v12-0004-Remove-superfluous-tqual.h-includes.patch

These look good to me. I think it would make sense to squash these 
together, and commit now.


Patches 20 and 21:
* v12-0020-WIP-Slotified-triggers.patch
* v12-0021-WIP-Slotify-EPQ.patch

I like this slotification of trigger and EPQ code. It seems like a nice 
thing to do, independently of the other patches. You said you wanted to 
polish that up to committable state, and commit separately: +1 on that. 
Perhaps do that even before patches 1-4.

> --- a/src/include/commands/trigger.h
> +++ b/src/include/commands/trigger.h
> @@ -35,8 +35,8 @@ typedef struct TriggerData
>  	HeapTuple	tg_trigtuple;
>  	HeapTuple	tg_newtuple;
>  	Trigger    *tg_trigger;
> -	Buffer		tg_trigtuplebuf;
> -	Buffer		tg_newtuplebuf;
> +	TupleTableSlot *tg_trigslot;
> +	TupleTableSlot *tg_newslot;
>  	Tuplestorestate *tg_oldtable;
>  	Tuplestorestate *tg_newtable;
>  } TriggerData;

Do we still need tg_trigtuple and tg_newtuple? Can't we always use the 
corresponding slots instead? Is it for backwards-compatibility with 
user-defined triggers written in C? (Documentation also needs to be 
updated for the changes in this struct)


I didn't look a the rest of the patches yet...

- Heikki