Re: Logical decoding for operations on zheap tables

Hi,

On 2019-01-04 08:54:34 +0530, Amit Kapila wrote:
> On Thu, Jan 3, 2019 at 11:30 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2018-12-31 09:56:48 +0530, Amit Kapila wrote:
> > > To support logical decoding for zheap operations, we need a way to
> > > ensure zheap tuples can be registered as change streams. One idea
> > > could be that we make ReorderBufferChange aware of another kind of
> > > tuples as well, something like this:
> > >
> > > @@ -100,6 +123,20 @@ typedef struct ReorderBufferChange
> > > ReorderBufferTupleBuf *newtuple;
> > > } tp;
> > > + struct
> > > + {
> > > + /* relation that has been changed */
> > > + RelFileNode relnode;
> > > +
> > > + /* no previously reassembled toast chunks are necessary anymore */
> > > + bool clear_toast_afterwards;
> > > +
> > > + /* valid for DELETE || UPDATE */
> > > + ReorderBufferZTupleBuf *oldtuple;
> > > + /* valid for INSERT || UPDATE */
> > > + ReorderBufferZTupleBuf *newtuple;
> > > + } ztp;
> > > +
> > >
> > >
> > > +/* an individual zheap tuple, stored in one chunk of memory */
> > > +typedef struct ReorderBufferZTupleBuf
> > > +{
> > > ..
> > > + /* tuple header, the interesting bit for users of logical decoding */
> > > + ZHeapTupleData tuple;
> > > ..
> > > +} ReorderBufferZTupleBuf;
> > >
> > > Apart from this, we need to define different decode functions for
> > > zheap operations as the WAL data is different for heap and zheap, so
> > > same functions can't be used to decode.
> >
> > I'm very strongly opposed to that. We shouldn't have expose every
> > possible storage method to output plugins, that'll make extensibility
> > a farce. I think we'll either have to re-form a HeapTuple or decide
> > to bite the bullet and start exposing tuples via slots.
> >
>
> To be clear, you are against exposing different format of tuples to
> plugins, not having different decoding routines for other storage
> engines, because later part is unavoidable due to WAL format.

Correct.

> Now,
> about tuple format, I guess it would be a lot better if we expose via
> slots, but won't that make existing plugins to change the way they
> decode the tuple, maybe that is okay?

I think one-off API changes are ok. What I'm strictly against is
primarily that output plugins will have to deal with more and more
different tuple formats.

> OTOH, re-forming the heap tuple
> has a cost which might be okay for the time being or first version,
> but eventually, we want to avoid that.

Right.

> The other reason why I refrained from tuple conversion was that I
> was not sure if we anywhere rely on the transaction information in
> the tuple during decode process, because that will be tricky to
> mimic, but I guess we don't check that.

Shouldn't be necessary - in fact, most of that information isn't in
the heap wal records in the first place.

> The only point for exposing a different tuple format via plugin was a
> performance which I think can be addressed if we expose via slots. I
> don't want to take up exposing slots instead of tuples for plugins as
> part of this project and I think if we want to go with that, it is
> better done as part of pluggable API?

No, I don't think it makes sense to address this is as part of
pluggable storage. That patchset is already way too invasive and
large.

Greetings,

Andres Freund