TupleTableSlot abstraction

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

TupleTableSlot abstraction

Andres Freund
Hi,

I've recently been discussing with Robert how to abstract
TupleTableSlots in the light of upcoming developments around abstracting
storage away.  Besides that aspect I think we also need to make them
more abstract to later deal with vectorized excution, but I'm more fuzzy
on the details there.

I'd previously, in a response to an early version of the pluggable
storage patch, commented [1] how I think the abstraction should roughly
look like. I'd privately started to prototype that. After my discussion
with Robert I got that prototype in a state that can run many queries,
but doesn't pass the regression tests.

The pluggable storage patch has also been updated [2] to abstract slots
more (see patch 0005).

My position is that there's a number of weaknesses with the current
TupleTableSlot API in light of multiple independent, possibly out of
core, storage implementations:


- TupleTableSlots have to contain all the necessary information for each
  type of slot. Some implementations might require a buffer pin to be
  hold (our heap), others pointers to multiple underlying points of data
  (columns store), some need additional metadata (our MinimalTuple).

  Unifying the information into one struct makes it much harder to
  provide differing implementations without modifying core postgres
  and/or increasing overhead for every user of slots.

  I think the consequence of that is that we need to allow each type of
  slot to have their own TupleTableSlot embedding struct.

  Haribabu's patch solved this by adding a tts_storage parameter that
  contains additional data, but imo that's unconvincing due to the added
  indirection in very performance critical codepaths.


- The way slots currently are implemented each new variant data is
  stored in slots adds new branches to hot code paths like
  ExecClearTuple(), and they're not extensible.

  Therefore I think we need to move to callback based implementation. In
  my tests, by moving the callback invocations into very thin inline
  functions, the branch prediction accuracy actually goes sligthly
  *up*.


- Currently we frequently convert from one way of representing a row
  into another, in the same slot. We do so fairly implicilty in
  ExecMaterializeSlot(), ExecFetchSlotMinimalTuple(), ExecCopySlot()...

  The more we abstract specific storage representation away, the worse I
  think that is. I think we should make such conversions explicit by
  requiring transfers between two slots if a specific type is required.


- We have a few codepaths that set fields on tuples that can't be
  represented in virtual slots. E.g. postgres_fdw sets ctid, oid,
  ExecProcessReturning() will set tableoid.


In my POC I turned TupleTableSlot into basically an abstract base class:
struct TupleTableSlot
{
        NodeTag type;

        uint16 flags;
        uint16 nvalid; /* # of valid values in tts_values */

        const TupleTableSlotOps *const cb;

        TupleDesc tupleDescriptor; /* slot's tuple descriptor */

        Datum   *values; /* current per-attribute values */
        bool   *nulls; /* current per-attribute null flags */

        /* can we optimize away? */
        MemoryContext mcxt; /* slot itself is in this context */
};

which then is inherited from for specific implementations of tuple
slots:

typedef struct HeapTupleTableSlot
{
        TupleTableSlot base;
        HeapTuple tuple; /* physical tuple */
        uint32 off; /* saved state for slot_deform_tuple */
} HeapTupleTableSlot;

...
typedef struct MinimalTupleTableSlot
{
        TupleTableSlot base;
        HeapTuple tuple; /* tuple wrapper */
        MinimalTuple mintuple; /* minimal tuple, or NULL if none */
        HeapTupleData minhdr; /* workspace for minimal-tuple-only case */
        uint32 off; /* saved state for slot_deform_tuple */
} MinimalTupleTableSlot;


typedef struct TupleTableSlotOps
{
        void (*init)(TupleTableSlot *slot);
        void (*release)(TupleTableSlot *slot);

        void (*clear)(TupleTableSlot *slot);

        void (*getsomeattrs)(TupleTableSlot *slot, int natts);

        void (*materialize)(TupleTableSlot *slot);

        HeapTuple (*get_heap_tuple)(TupleTableSlot *slot);
        MinimalTuple (*get_minimal_tuple)(TupleTableSlot *slot);

        HeapTuple (*copy_heap_tuple)(TupleTableSlot *slot);
        MinimalTuple (*copy_minimal_tuple)(TupleTableSlot *slot);

} TupleTableSlotOps;

when creating a slot one has to specify the type of slot one wants to
use:

typedef enum TupleTableSlotType
{
        TTS_TYPE_VIRTUAL,
        TTS_TYPE_HEAPTUPLE,
        TTS_TYPE_MINIMALTUPLE,
        TTS_TYPE_BUFFER
} TupleTableSlotType;

extern TupleTableSlot *MakeTupleTableSlot(TupleDesc desc, TupleTableSlotType st);

You might notice that I've renamed a few fields (tts_values -> values,
tts_isnull -> nulls, tts_nvalid -> nvalid) that haven't actually changed
in the above structs / the attached patch. I now think that's probably
not a good idea, unnecessarily breaking more code than necessary.


I generally like this scheme because it allows the TupleTableStruct to
be relatively lean, without knowing much about specific implementations
of slots.

After this change functions like slot_getattr are thin inline wrappers:
+static inline Datum
+slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull)
+{
+   Assert(attnum > 0);
+
+   if (attnum > slot->nvalid)
+       slot->cb->getsomeattrs(slot, attnum);
+
+   Assert(attnum <= slot->nvalid);
+
+   *isnull = slot->nulls[attnum - 1];
+   return slot->values[attnum - 1];
+}

Note that I've moved system attribute handling out of the slot_getattr
path - there were a few paths accessing them via slot_getattr, and it's
a lot of unnecessary branching.


The fact that slots of different types have different storage means that
we can't just change the type of a slot when needed. In nearly all
places that's not a problem. E.g. ExecHashTableInsert() converts the
"incoming" slot to one containing a minimal tuple with
ExecFetchSlotMinimalTuple(), but that's essentially transient as it's
just used to copy the tuple into the hashtable.  We could even gain
quite some efficiency by directly copying into the allocated space
(saving one serialization/copy step for the common case where input is
either a virtual or a heap tuple).

The slightly bigger issue is that several parts of the code currently
require heaptuples they can scribble upon. E.g. nodeModifyTable.c
materializes the tuple - those can be solved by using a local slot to
store the tuple, rather than using the incoming one. In nearly all cases
we'd still be left with the same number of tuple copy steps, as
materializing the tuple with copy into the local storage anyway.  A few
other cases, e.g. intorel_receive(), just can use ExecCopySlotTuple() or
such instead of materialize.


The biggest issue I don't have a good answer for, and where I don't like
Haribabu's solution, is how to deal with system columns like oid, ctid,
tableoid. Haribabu's patch adds storage of those to the slot, but that
seems like quite the cludge to me.  It appears to me that the best thing
would be to require projections for all accesses to system columns, at
the original level of access. Then we don't need any sort of special
codepaths dealing with them. But that's not exactly a trivial change and
has some added complications too (junkfilter type things).


I made a reference to vectorized execution above. That's because that I
found, while prototyping vectorized execution, that it's a very common
need to interact with parts of the system that aren't vectorized, and
doing so has to be extremely cheap.  With the above we can have a
TupleTableSlot type that's essentially just a cheap cursor into a batch
of tuples.  Besides making it efficiently possible to hand of slots to
part of the system that can't deal with tuple batches, it allows to do
fun things like having the first slot_deform_tuple() call for one tuple
in a batch to deform a full page's worth of tuples, yielding *much*
better cache access patterns.


I haven't done that, but I think we should split ExecStoreTuple() into
multiple versions, that only work on specific types of tuple
slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
code dealing with tuples would call ExecStoreHeapTuple().  The relevant
callers already need to know what kind of tuple they're dealing with,
therefore that's not a huge burden.


Please please note that the attached patch is *NOT* meant to be a full
proposal or even a to be reviewed patch, it's just an illustration of
the concepts I'm talking about.

Comments? Better ideas?

[1] http://archives.postgresql.org/message-id/20170815065348.afkj45dgmg22ecfh%40alap3.anarazel.de
[2] http://archives.postgresql.org/message-id/CAJrrPGdY_hgvu06R_vCibUktKRLp%3Db8nBfeZkh%3DKRc95tq4euA%40mail.gmail.com

Greetings,

Andres Freund

0001-tupleslot-rewrite.patch (162K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Alexander Korotkov
Hi!

Thank you for working on this subject.  See some my comments below.

On Wed, Feb 21, 2018 at 1:43 AM, Andres Freund <[hidden email]> wrote:
- TupleTableSlots have to contain all the necessary information for each
  type of slot. Some implementations might require a buffer pin to be
  hold (our heap), others pointers to multiple underlying points of data
  (columns store), some need additional metadata (our MinimalTuple).

  Unifying the information into one struct makes it much harder to
  provide differing implementations without modifying core postgres
  and/or increasing overhead for every user of slots.

  I think the consequence of that is that we need to allow each type of
  slot to have their own TupleTableSlot embedding struct.
 
+1, I think that it's reasonable to keep minimal common slot information in
TupleTableSlot struct and to let particular slot implementations to extend it.

  Haribabu's patch solved this by adding a tts_storage parameter that
  contains additional data, but imo that's unconvincing due to the added
  indirection in very performance critical codepaths.
 
+1

- The way slots currently are implemented each new variant data is
  stored in slots adds new branches to hot code paths like
  ExecClearTuple(), and they're not extensible.

  Therefore I think we need to move to callback based implementation. In
  my tests, by moving the callback invocations into very thin inline
  functions, the branch prediction accuracy actually goes sligthly
  *up*.

Sounds interesting.  Besides branch prediction accuracy, what can you
say about overall performance?

- Currently we frequently convert from one way of representing a row
  into another, in the same slot. We do so fairly implicilty in
  ExecMaterializeSlot(), ExecFetchSlotMinimalTuple(), ExecCopySlot()...

  The more we abstract specific storage representation away, the worse I
  think that is. I think we should make such conversions explicit by
  requiring transfers between two slots if a specific type is required.


- We have a few codepaths that set fields on tuples that can't be
  represented in virtual slots. E.g. postgres_fdw sets ctid, oid,
  ExecProcessReturning() will set tableoid.


In my POC I turned TupleTableSlot into basically an abstract base class:
struct TupleTableSlot
{
        NodeTag         type;

        uint16          flags;
        uint16          nvalid;         /* # of valid values in tts_values */

        const TupleTableSlotOps *const cb;

        TupleDesc       tupleDescriptor;        /* slot's tuple descriptor */

        Datum      *values;             /* current per-attribute values */
        bool       *nulls;              /* current per-attribute null flags */

        /* can we optimize away? */
        MemoryContext mcxt;             /* slot itself is in this context */
};

which then is inherited from for specific implementations of tuple
slots:

typedef struct HeapTupleTableSlot
{
        TupleTableSlot base;
        HeapTuple       tuple;          /* physical tuple */
        uint32          off;            /* saved state for slot_deform_tuple */
} HeapTupleTableSlot;

...
typedef struct MinimalTupleTableSlot
{
        TupleTableSlot base;
        HeapTuple       tuple;          /* tuple wrapper */
        MinimalTuple mintuple;  /* minimal tuple, or NULL if none */
        HeapTupleData minhdr;   /* workspace for minimal-tuple-only case */
        uint32          off;            /* saved state for slot_deform_tuple */
} MinimalTupleTableSlot;


typedef struct TupleTableSlotOps
{
        void (*init)(TupleTableSlot *slot);
        void (*release)(TupleTableSlot *slot);

        void (*clear)(TupleTableSlot *slot);

        void (*getsomeattrs)(TupleTableSlot *slot, int natts);

        void (*materialize)(TupleTableSlot *slot);

        HeapTuple (*get_heap_tuple)(TupleTableSlot *slot);
        MinimalTuple (*get_minimal_tuple)(TupleTableSlot *slot);

        HeapTuple (*copy_heap_tuple)(TupleTableSlot *slot);
        MinimalTuple (*copy_minimal_tuple)(TupleTableSlot *slot);

} TupleTableSlotOps;

when creating a slot one has to specify the type of slot one wants to
use:

typedef enum TupleTableSlotType
{
        TTS_TYPE_VIRTUAL,
        TTS_TYPE_HEAPTUPLE,
        TTS_TYPE_MINIMALTUPLE,
        TTS_TYPE_BUFFER
} TupleTableSlotType;

extern TupleTableSlot *MakeTupleTableSlot(TupleDesc desc, TupleTableSlotType st);
 
Can user define his own slot type?  If so, I assume that hard-coded
enum is a limitation of current prototype, and eventually we would have
an extendable array of slot types.  Is it correct?

You might notice that I've renamed a few fields (tts_values -> values,
tts_isnull -> nulls, tts_nvalid -> nvalid) that haven't actually changed
in the above structs / the attached patch. I now think that's probably
not a good idea, unnecessarily breaking more code than necessary.

For me "tss_" prefix looks a bit weird, but I agree that we probably should
refrain from renaming this in order to break as small number of things as possible.
 
I generally like this scheme because it allows the TupleTableStruct to
be relatively lean, without knowing much about specific implementations
of slots.

After this change functions like slot_getattr are thin inline wrappers:
+static inline Datum
+slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull)
+{
+   Assert(attnum > 0);
+
+   if (attnum > slot->nvalid)
+       slot->cb->getsomeattrs(slot, attnum);
+
+   Assert(attnum <= slot->nvalid);
+
+   *isnull = slot->nulls[attnum - 1];
+   return slot->values[attnum - 1];
+}

Note that I've moved system attribute handling out of the slot_getattr
path - there were a few paths accessing them via slot_getattr, and it's
a lot of unnecessary branching.


The fact that slots of different types have different storage means that
we can't just change the type of a slot when needed. In nearly all
places that's not a problem. E.g. ExecHashTableInsert() converts the
"incoming" slot to one containing a minimal tuple with
ExecFetchSlotMinimalTuple(), but that's essentially transient as it's
just used to copy the tuple into the hashtable.  We could even gain
quite some efficiency by directly copying into the allocated space
(saving one serialization/copy step for the common case where input is
either a virtual or a heap tuple).

The slightly bigger issue is that several parts of the code currently
require heaptuples they can scribble upon. E.g. nodeModifyTable.c
materializes the tuple - those can be solved by using a local slot to
store the tuple, rather than using the incoming one. In nearly all cases
we'd still be left with the same number of tuple copy steps, as
materializing the tuple with copy into the local storage anyway.  A few
other cases, e.g. intorel_receive(), just can use ExecCopySlotTuple() or
such instead of materialize.


The biggest issue I don't have a good answer for, and where I don't like
Haribabu's solution, is how to deal with system columns like oid, ctid,
tableoid. Haribabu's patch adds storage of those to the slot, but that
seems like quite the cludge to me.  It appears to me that the best thing
would be to require projections for all accesses to system columns, at
the original level of access. Then we don't need any sort of special
codepaths dealing with them. But that's not exactly a trivial change and
has some added complications too (junkfilter type things).

I also think that projections for accessing system columns is probably
the best solution for them.
 
I made a reference to vectorized execution above. That's because that I
found, while prototyping vectorized execution, that it's a very common
need to interact with parts of the system that aren't vectorized, and
doing so has to be extremely cheap.  With the above we can have a
TupleTableSlot type that's essentially just a cheap cursor into a batch
of tuples.  Besides making it efficiently possible to hand of slots to
part of the system that can't deal with tuple batches, it allows to do
fun things like having the first slot_deform_tuple() call for one tuple
in a batch to deform a full page's worth of tuples, yielding *much*
better cache access patterns.
 
Are you planning to use custom slots only for interaction between
vectorized and non-vectorized parts of executor, or also for interaction
inside vectorized part of executor.  It seems that proposed interface
isn't suitable for interaction between two vectorized nodes.  Are you
planning to introduce special kind of vectorized slots or whatever?

I haven't done that, but I think we should split ExecStoreTuple() into
multiple versions, that only work on specific types of tuple
slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
code dealing with tuples would call ExecStoreHeapTuple().  The relevant
callers already need to know what kind of tuple they're dealing with,
therefore that's not a huge burden.


Please please note that the attached patch is *NOT* meant to be a full
proposal or even a to be reviewed patch, it's just an illustration of
the concepts I'm talking about.

Comments? Better ideas?

As I get, your primary interest in this topic is vectorized execution.  When
passing data from vectorized to non-vectorized parts of executor tuples are
materialized by moving them from vectorized slot to regular slot.
But what do you thing about alternative tuple formats in custom row-oriented
table access methods?  It's likely we should try to minimize tuple format
conversion.  Thus, some executor nodes can switch their tuple format to
match table access method tuple format.  For instance, we're running
aggregate over table with custom table access method.  Then hash
aggregate node may accumulate tuples in the same format as they are
received from table access method.  Does it make sense?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Álvaro Herrera
In reply to this post by Andres Freund
Andres Freund wrote:
> Hi,
>
> I've recently been discussing with Robert how to abstract
> TupleTableSlots in the light of upcoming developments around abstracting
> storage away.  Besides that aspect I think we also need to make them
> more abstract to later deal with vectorized excution, but I'm more fuzzy
> on the details there.

Hi, is this patch proposed for pg11?

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
On 2018-03-13 15:18:34 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > Hi,
> >
> > I've recently been discussing with Robert how to abstract
> > TupleTableSlots in the light of upcoming developments around abstracting
> > storage away.  Besides that aspect I think we also need to make them
> > more abstract to later deal with vectorized excution, but I'm more fuzzy
> > on the details there.

> Hi, is this patch proposed for pg11?

I wish, but I don't see it happening unless I get a time compression
device from somewhere :(

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
In reply to this post by Andres Freund
On Wed, Feb 21, 2018 at 4:13 AM, Andres Freund <[hidden email]> wrote:

>
> In my POC I turned TupleTableSlot into basically an abstract base class:

Here's set of patches which move this POC forward towards completion,
not yet complete though.

Here's what has changed.
1. Fixed all the regression failures while running make check from
regress and postgres_fdw.
2. Added/modified a bunch of comments about the new TupleTableSlot
structure. Modified comments in existing functions working with
TupleTableSlots as per the new implementation. Added comments
explaining each callback in TupleTableSlotOps, and their
implementations.
3. Improved error messages in some TupleTableTypeSlot specific implementations.
4. Fixed some coding errors in the POC patch (which didn't show up as
a failure in regression)

>
> You might notice that I've renamed a few fields (tts_values -> values,
> tts_isnull -> nulls, tts_nvalid -> nvalid) that haven't actually changed
> in the above structs / the attached patch. I now think that's probably
> not a good idea, unnecessarily breaking more code than necessary.

Done that. All members of TupleTableSlot have their names starting with tts_

>
> After this change functions like slot_getattr are thin inline wrappers:
> +static inline Datum
> +slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull)
> +{
> +   Assert(attnum > 0);
> +
> +   if (attnum > slot->nvalid)
> +       slot->cb->getsomeattrs(slot, attnum);
> +
> +   Assert(attnum <= slot->nvalid);
> +
> +   *isnull = slot->nulls[attnum - 1];
> +   return slot->values[attnum - 1];
> +}
>
> Note that I've moved system attribute handling out of the slot_getattr
> path - there were a few paths accessing them via slot_getattr, and it's
> a lot of unnecessary branching.
The system attributes are accessed via heap_getsysattr() by passing
that function a tuple from HeapTupleTableSlot or
BufferHeapTupleTableSlot. Those places Assert that the slot being used
is either HeapTupleTableSlot or BufferHeapTupleTableSlot.
slot_getsysattr() which accessed system attributes through a lot is
now removed and replaced with heap_getsysattr as described.

>
> The slightly bigger issue is that several parts of the code currently
> require heaptuples they can scribble upon. E.g. nodeModifyTable.c
> materializes the tuple - those can be solved by using a local slot to
> store the tuple, rather than using the incoming one. In nearly all cases
> we'd still be left with the same number of tuple copy steps, as
> materializing the tuple with copy into the local storage anyway.  A few
> other cases, e.g. intorel_receive(), just can use ExecCopySlotTuple() or
> such instead of materialize.

There is a patch changing callers of ExecMaterializeSlot() to use
ExecGetHeapTupleFromSlot() or ExecSaveSlot(), new functions added by
me. But I think that needs more work. I don't think the new functions
are necessary. We could use ExecCopySlotTuple(), ExecFetchSlotTuple()
in place of ExecGetHeapTupleFromSlot() appropriately. The callers who
just want the tuple and not necessarily a writable one, can use
ExecFetchSlotTuple(). Those who want a writable copy can use
ExecCopySlotTuple(). When the slot is *not* heap or buffer heap tuple
table slot, we materialize a heap tuple every time we want to get the
heap tuple out of the slot. If we are doing this multiple times
without changing the contents of the slot, we will leak memory. I have
TODOs to check whether that's a problem in practice. May be we should
use ExecCopySlotTuple() explicitly while fetching tuples from those
kinds of slot. Same is case while fetching a minimal heap tuple from
non minimal heap tuple slots.

>
>
> The biggest issue I don't have a good answer for, and where I don't like
> Haribabu's solution, is how to deal with system columns like oid, ctid,
> tableoid. Haribabu's patch adds storage of those to the slot, but that
> seems like quite the cludge to me.  It appears to me that the best thing
> would be to require projections for all accesses to system columns, at
> the original level of access. Then we don't need any sort of special
> codepaths dealing with them. But that's not exactly a trivial change and
> has some added complications too (junkfilter type things).
>
I studied the code extensively to check whether there are any cases
where we fetch system columns using (Var nodes with varattno < 0) from
(headers of ) tuples produced by non-scan nodes (e.g. join,
aggregation). That's not the case. We do have instances of INNER,
OUTER Var with varattno < 0, but those are the cases when trigger
accesses OLD and NEW tuples. I have a patch to add tests for the same.

>
> I haven't done that, but I think we should split ExecStoreTuple() into
> multiple versions, that only work on specific types of tuple
> slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
> code dealing with tuples would call ExecStoreHeapTuple().  The relevant
> callers already need to know what kind of tuple they're dealing with,
> therefore that's not a huge burden.

I thought so too, but haven't done that change right now. Will work on
that. That's in my TODO list.

Here's description of patches.
0001, 0002 are the same patches as submitted in [1].

0003 changes tts_nvalid from int to AttrNumber. int would take at
least 4 bytes whereas the maximum value that tts_nvalid can take is
restricted by AttrNumber which 2 byte long.

0004 removed TupleDescGetSlot() function, which is obsolete, but
retained for extensions. With TupleTableSlot abstraction, I think the
extensions need to change anyway, so removing that function
altogether.

0005 packs various boolean members from TupleTableSlot structure into
a uint16 tts_flags, with each bit for each one bool member. I think we
can remove SHOULDFREEMIN since SHOULDFREE on MinimalHeapTupleTableSlot
can be used in place of SHOULDFREEMIN. I will do that in the next
version. This reduces the size of TupleTableSlot structure and can be
committed as an independent patch. There's also possibility that only
the BufferHeapTupleTableSlot will have ~SHOULDFREE but all other slots
have SHOULDFREE set, so we can get rid of that flag altogether. But I
need to verify that.

0006 This is improved version of your POC patch. I have changed the
tuple table slot type in many places which showed up as regression
failures. Added/modified a bunch of comments, improved error messages,
corrected code at some places, addressed FIXME's at some places. Also
added macros to check the TupleTableSlot's type based on
TupleTableSlotOps set.

0007 adds tts_type member indicating TupleTableSlot type readily
avaialble for debugging. We may or may not commit this patch.

0008 changes the place where we reset expression context used for
processing RETURNING and ON CONFLICT clause. If a query has both the
clauses, we reset the same expression twice while processing same
input/output tuple. This means that some memory allocated while
processing ON CONFLICT and required by RETURNING will get freed before
the later gets at it. This looks like a bug in the existing code, but
I have not been able to create a reproduction for the same. It showed
up as a regression failure with TupleTableAbstraction changes.

0009 Replaces ExecMaterializeSlot as described above. I will work on
this again and examine all the instances of ExecMaterializeSlot as
described above.

0010 Index scan uses reorder queue in some cases. Tuples are stored
reorder queue as heap tuples without any associated buffer. While
returning a tuple from reorder queue, IndexNextWithReorder() should
use TupleTableSlot of type HeapTupleTableSlot. A tuple returned
directly from an index scan has a buffer associated with it, so should
use
TupleTableSlot of type BufferHeapTupleTableSlot. So, use different
kinds of slots for an index scan.


Next steps
1. Address TODO in the code. I have listed some of those above.

2. Right now we are using TupleTableSlotType, an enum, to create slots
of required type. But extensions which want to add their own slot
types won't be able to add a type in that enum. So, they will need to
write their own MakeTupleTableSlot. That function uses the
TupleTableSlotType to set TupleTableSlotOps and calculate the minimum
size of slot. We could change the function to accept TupleTableSlotOps
and the minimum size and it just works for all the extensions. Or it
could just accept TupleTableSlotOps and there's a callback to
calculate minimum memory required for the slot (say based on the tuple
descriptor available).

3. compile with LLVM and fix any compilation and regression errors.

4. We need to do something with the name ExecStoreVirtualSlot(), which
is being (and will be) used for all kinds of TupleTableSlot type.
Right now I don't have any bright ideas.

5. ExecFetch* functions are now one liners, so we could make those
inline and keep those in header file like, say slot_getattr.

6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
and invoked on the destination slot type.

7. slot_attisnull() deforms a heap/minimal tuple if that status for
given attribute is not available tts_isnull. Should we instead add a
callback attisnull() which can use something like heap_isnull()?

There are TODOs in the patches for 4, 5, 6, 7.

I will continue to work on these steps.

[1] https://www.postgresql.org/message-id/CAFjFpRerUFX=T0nSnCoroXAJMoo-xah9J+pi7+xDUx86PtQmew@...

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches.tar.zip (63K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Mon, Jun 11, 2018 at 6:13 PM, Ashutosh Bapat
<[hidden email]> wrote:

>
>>
>> The slightly bigger issue is that several parts of the code currently
>> require heaptuples they can scribble upon. E.g. nodeModifyTable.c
>> materializes the tuple - those can be solved by using a local slot to
>> store the tuple, rather than using the incoming one. In nearly all cases
>> we'd still be left with the same number of tuple copy steps, as
>> materializing the tuple with copy into the local storage anyway.  A few
>> other cases, e.g. intorel_receive(), just can use ExecCopySlotTuple() or
>> such instead of materialize.
>
> There is a patch changing callers of ExecMaterializeSlot() to use
> ExecGetHeapTupleFromSlot() or ExecSaveSlot(), new functions added by
> me. But I think that needs more work. I don't think the new functions
> are necessary. We could use ExecCopySlotTuple(), ExecFetchSlotTuple()
> in place of ExecGetHeapTupleFromSlot() appropriately. The callers who
> just want the tuple and not necessarily a writable one, can use
> ExecFetchSlotTuple(). Those who want a writable copy can use
> ExecCopySlotTuple(). When the slot is *not* heap or buffer heap tuple
> table slot, we materialize a heap tuple every time we want to get the
> heap tuple out of the slot. If we are doing this multiple times
> without changing the contents of the slot, we will leak memory. I have
> TODOs to check whether that's a problem in practice. May be we should
> use ExecCopySlotTuple() explicitly while fetching tuples from those
> kinds of slot. Same is case while fetching a minimal heap tuple from
> non minimal heap tuple slots.
In the attached set of patches, the ExecMaterializeSlot(),
ExecFetchSlotTuple() and ExecFetchSlotMinimalTuple() have been
changed.

Here's excerpt of the commit messages included in 0006 in the attached
patch-set.
---
Before this work, the contents of the slot could be in virtual form
(in tts_datums and tts_isnull) and/or be stored as a heap tuple and/or
a minimal tuple.  When the contents were in heap or minimal tuple
form, those tuples need not be "owned" by the slot. When a tuple is
owned by a slot, it's slot's responsibility to free the memory
consumed by the tuple when the tuple is not needed. That typically
happened when the slot got a new tuple to store in it or when the slot
itself was discarded, mostly along-with the other slots in tuple
table.

Then ExecMaterializeSlot() served two purposes: 1. If the contents of
the slot were not "owned" by the slot, or if there was no heap tuple
in the slot, it created a heap tuple which it "owned" from those
contents. 2. It returned the heap tuple, which the slot "owned". This
was typically used when the caller wanted, from the slot, a heap tuple
which it can upon (esp. system attributes like tableOid) and expected
it to be visible from within the slot.

Since a single slot contained a tuple in various forms, it could "own"
multiple forms at a time and return whichever was requested when
ExecFetchSlotTuple() or ExecFetchSlotMinimalTuple() was called.  Those
functions, however, differed from ExecMaterializeSlot() in the sense
that they returned a tuple even when the slot didn't own it.

With this work, every slot contains a tuple in "virtual" form, but
only one of the other two forms. Thus ExecMaterializeSlot() can not
create a heap tuple, and let the slot "own" it, in a slot that can not
contain a heap tuple.  Neither ExecFetchSlotTuple() can return a tuple
from a slot which can not contain a heap tuple without consuming
memory (slots with minimal tuple can do some pointer swizzling and
save memory but that lump of memory is not exactly same as a heap
tuple memory lump.). Since such a heap tuple can not be "owned" by the
slot, the memory consumed by the tuple needs to be freed by the
caller. Similarly for ExecFetchSlotMinimalTuple().

Hence, in the new scheme the meaning of ExecMaterializeSlot(),
ExecFetchSlotTuple() and ExecFetchSlotMinimalTuple() needs to change.

ExecMaterializeSlot()
---------------------

ExecMaterializeSlot() should save the contents of the slot into its
native tuple format, whichever it is, so that it "owns" the tuple.

ExecFetchSlotTuple()
--------------------

ExecFetchSlotTuple() should just return the heap tuple when the
underlying slot can "own" a heap tuple and error out when slot can not
contain the requested format e.g. when ExecFetchSlotTuple() is called
on a "virtual" or "minimal" tuple slot.  This is in-line with what we
are doing with ExecStore* functions. The earlier callers of
ExecMaterializeSlot(), which expected a heap tuple to be returned,
require to call ExecMaterializeSlot() (no return value) followed by
ExecFetchSlotTuple(). Instead of that ExecFetchSlotTuple() is changed
to accept a second argument "materialize". When it's true, the
returned heap tuple is materialized and "owned" by the slot. When it's
false, the returned heap tuple may not be "owned" by the slot. All the
callers of ExecFetchSlotTuple() can be managed to pass a
HeapTupleTableSlot or a BufferHeapTupleTableSlot. Instead of relying
on the TupleTableSlotType (which may be expanded later for user
defined TupleTableSlots) we rely on the presence get_heap_tuple()
callback in the given slot. That callback is expected to return a heap
tuple as is from the slot.

In a few cases, ExecFetchSlotTuple() is called with a slot which is
filled by tuplestore_gettupleslot(). We use a separate tuple store to
store foreign table tuples for processing AFTER triggers and for
storing tuples returned by a function. They are stored in the tuple
store as minimal tuples. In both the cases, the minimal tuples are
converted to heap tuple (using earlier version of
ExecFetchSlotTuple()) before they are processed further after
obtaining those from a tuple store using tuplestore_gettupleslot().
With the tuple table store abstraction a minimal tuple can be stored
in a MinimalTupleTableSlot. Thus we have two options

1. Pass a MinimalTupleTableSlot to tuplestore_gettupleslot() and get a
minimal tuple in this slot. Convert the minimal tuple to a heap tuple
by allocating memory for the converted tuple. We have to explicitly
release the memory allocated for the heap tuple after it's been
processed. When do we do that exactly in AfterTriggerExecute() is not
clear. ExecFetchSlotTupleDatum() could do that right away.

2. Pass a HeapTupleTableSlot to tuplestore_gettupleslot() and let it
store the heap tuple crafted from minimal tuple, which can be freed
within the same function, if necessary. The heap tuple gets freed when
the slot is used to store the next tuple to be processed.

The second option looks better since it continues to use slot's
mechanism to "own" and free tuples that it "owns". Hence implemented
the same.

ExecFetchSlotMinimalTuple()
---------------------------

Before this work, a TupleTableSlot could "own" a minimal tuple as
well. Thus ExecFetchSlotMinimalTuple() returned a minimal tuple after
constructing and "owning" it if it was not readily available. When the
slot "owned" the minimal tuple, the memory consumed by the tuple was
freed when a new tuple was stored in it or the slot was cleared.

With this work, not all TupleTableSlot types can "own" a minimal
tuple.  When fetching a minimal tuple from a slot that can not "own" a
tuple, memory is allocated to construct the minimal tuple, which needs
to be freed explicitly. Hence ExecFetchSlotMinimalTuple() is modified
to flag the caller whether it should free the memory consumed by the
returned minimal tuple.

Right now only a MinimalTupleTableSlot can own a minimal tuple. But we
may change that in future or a user defined TupleTableSlot type (added
through an extension) may be able to "own" a minimal tuple in it.
Hence instead of relying upon TTS_IS_MINIMAL() macro to tell us
whether the TupleTableSlot can "own" a minimal tuple or not, we rely
on the set of callbacks.  A TupleTableSlot which can hold a minimal
tuple implements get_minimal_tuple callback. Other TupleTableSlot
types leave the callback NULL.

The minimal tuple returned by this function is usually copied into a
hash table or a file. But, unlike ExecFetchSlotTuple() it's never
written to. Hence the difference between signatures of the two
functions.

---


>
>>
>> I haven't done that, but I think we should split ExecStoreTuple() into
>> multiple versions, that only work on specific types of tuple
>> slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
>> code dealing with tuples would call ExecStoreHeapTuple().  The relevant
>> callers already need to know what kind of tuple they're dealing with,
>> therefore that's not a huge burden.
>
> I thought so too, but haven't done that change right now. Will work on
> that. That's in my TODO list.
This is still a TODO.

0001-0005 are same as the previous patch set.

>
> 0006 This is improved version of your POC patch. I have changed the
> tuple table slot type in many places which showed up as regression
> failures. Added/modified a bunch of comments, improved error messages,
> corrected code at some places, addressed FIXME's at some places. Also
> added macros to check the TupleTableSlot's type based on
> TupleTableSlotOps set.

The changes to ExecMaterializeSlot(), ExecFetchSlotTuple() and
ExecFetchSlotMinimalTuple() discussed above are included in this
patch.

0007, 0008 are same as previous patch set.

>
> 0009 Replaces ExecMaterializeSlot as described above. I will work on
> this again and examine all the instances of ExecMaterializeSlot as
> described above.

This is merged with 0006.

>
> 0010 Index scan uses reorder queue in some cases. Tuples are stored
> reorder queue as heap tuples without any associated buffer. While
> returning a tuple from reorder queue, IndexNextWithReorder() should
> use TupleTableSlot of type HeapTupleTableSlot. A tuple returned
> directly from an index scan has a buffer associated with it, so should
> use
> TupleTableSlot of type BufferHeapTupleTableSlot. So, use different
> kinds of slots for an index scan.

This is 0009

>
>
> Next steps
> 1. Address TODO in the code. I have listed some of those above.

There are still a handful of TODOs in the patches. I will work on those next.

>
> 2. Right now we are using TupleTableSlotType, an enum, to create slots
> of required type. But extensions which want to add their own slot
> types won't be able to add a type in that enum. So, they will need to
> write their own MakeTupleTableSlot. That function uses the
> TupleTableSlotType to set TupleTableSlotOps and calculate the minimum
> size of slot. We could change the function to accept TupleTableSlotOps
> and the minimum size and it just works for all the extensions. Or it
> could just accept TupleTableSlotOps and there's a callback to
> calculate minimum memory required for the slot (say based on the tuple
> descriptor available).
This is still a TODO.

>
> 3. compile with LLVM and fix any compilation and regression errors.

When I compiled server with just 0003 applied with LLVM, the
compilation went well, but there was a server crash. That patch
changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
the crash with a debug LLVM build, but couldn't complete the work.
Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
fix that crash. I think, we should make it easy to change the data
types of the members in structures shared by JIT and non-JIT code, may
be automatically create both copies of the code somehow. I will get
back to this after addressing other TODOs.

>
> 4. We need to do something with the name ExecStoreVirtualSlot(), which
> is being (and will be) used for all kinds of TupleTableSlot type.
> Right now I don't have any bright ideas.

Still a TODO.

>
> 5. ExecFetch* functions are now one liners, so we could make those
> inline and keep those in header file like, say slot_getattr.

This is still a TODO.

>
> 6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
> and invoked on the destination slot type.

This is still a TODO.

>
> 7. slot_attisnull() deforms a heap/minimal tuple if that status for
> given attribute is not available tts_isnull. Should we instead add a
> callback attisnull() which can use something like heap_isnull()?

This is still a TODO.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v2.tar.zip (70K) Download Attachment
attrnumber_llvm_type.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Thu, Jul 5, 2018 at 4:07 PM, Ashutosh Bapat
<[hidden email]> wrote:

>>> I haven't done that, but I think we should split ExecStoreTuple() into
>>> multiple versions, that only work on specific types of tuple
>>> slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
>>> code dealing with tuples would call ExecStoreHeapTuple().  The relevant
>>> callers already need to know what kind of tuple they're dealing with,
>>> therefore that's not a huge burden.
>>
>> I thought so too, but haven't done that change right now. Will work on
>> that. That's in my TODO list.

Done. Added that as a separate patch 0001 since that change adds value
by itself.

0001 in the earlier patch set got committed, 0002 in that patch set is
not required anymore.

0002 - 0004 in this patch set are same as 0003-0005 in the previous patch set.

0005 in this patch set is 0006 in the previous one with a bunch of
TODO's addressed. An important change is virtual tuple slot contents
are never required to be freed when slot is cleared.

0006-0009 are same as 0007 - 0010 in the previous patch set.


>>
>> Next steps
>> 1. Address TODO in the code. I have listed some of those above.
>
> There are still a handful of TODOs in the patches. I will work on those next.

The number of TODOs has reduced, but there are still some that I am working on.

>
>>
>> 2. Right now we are using TupleTableSlotType, an enum, to create slots
>> of required type. But extensions which want to add their own slot
>> types won't be able to add a type in that enum. So, they will need to
>> write their own MakeTupleTableSlot. That function uses the
>> TupleTableSlotType to set TupleTableSlotOps and calculate the minimum
>> size of slot. We could change the function to accept TupleTableSlotOps
>> and the minimum size and it just works for all the extensions. Or it
>> could just accept TupleTableSlotOps and there's a callback to
>> calculate minimum memory required for the slot (say based on the tuple
>> descriptor available).
This is still TODO.

>
>>
>> 3. compile with LLVM and fix any compilation and regression errors.
>
> When I compiled server with just 0003 applied with LLVM, the
> compilation went well, but there was a server crash. That patch
> changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
> the crash with a debug LLVM build, but couldn't complete the work.
> Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
> fix that crash. I think, we should make it easy to change the data
> types of the members in structures shared by JIT and non-JIT code, may
> be automatically create both copies of the code somehow. I will get
> back to this after addressing other TODOs.
>
This is still a TODO

>>
>> 4. We need to do something with the name ExecStoreVirtualSlot(), which
>> is being (and will be) used for all kinds of TupleTableSlot type.
>> Right now I don't have any bright ideas.
>

Done and added as a separate patch 0010. Separate patch so that we can
discard this change, if we don't agree on it.

>>
>> 5. ExecFetch* functions are now one liners, so we could make those
>> inline and keep those in header file like, say slot_getattr.
>

Done.

>
>>
>> 6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
>> and invoked on the destination slot type.

This is still a TODO

>
>>
>> 7. slot_attisnull() deforms a heap/minimal tuple if that status for
>> given attribute is not available tts_isnull. Should we instead add a
>> callback attisnull() which can use something like heap_isnull()?
>

This is still a TODO.


--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v3.tar.zip (84K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Fri, Jul 13, 2018 at 3:45 PM, Ashutosh Bapat
<[hidden email]> wrote:
>
>
>>>
>>> Next steps
>>> 1. Address TODO in the code. I have listed some of those above.
>>
>> There are still a handful of TODOs in the patches. I will work on those next.
>
> The number of TODOs has reduced, but there are still some that I am working on.

The attached patch set has all the TODOs fixed.

>
>>
>>>
>>> 2. Right now we are using TupleTableSlotType, an enum, to create slots
>>> of required type. But extensions which want to add their own slot
>>> types won't be able to add a type in that enum. So, they will need to
>>> write their own MakeTupleTableSlot. That function uses the
>>> TupleTableSlotType to set TupleTableSlotOps and calculate the minimum
>>> size of slot. We could change the function to accept TupleTableSlotOps
>>> and the minimum size and it just works for all the extensions. Or it
>>> could just accept TupleTableSlotOps and there's a callback to
>>> calculate minimum memory required for the slot (say based on the tuple
>>> descriptor available).
>
> This is still TODO.
Done.

>
>>
>>>
>>> 3. compile with LLVM and fix any compilation and regression errors.
>>
>> When I compiled server with just 0003 applied with LLVM, the
>> compilation went well, but there was a server crash. That patch
>> changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
>> the crash with a debug LLVM build, but couldn't complete the work.
>> Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
>> fix that crash. I think, we should make it easy to change the data
>> types of the members in structures shared by JIT and non-JIT code, may
>> be automatically create both copies of the code somehow. I will get
>> back to this after addressing other TODOs.
>>
>
> This is still a TODO
Still a TODO.

>
>>
>>>
>>> 6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
>>> and invoked on the destination slot type.
>
> This is still a TODO

Still a TODO.

>
>>
>>>
>>> 7. slot_attisnull() deforms a heap/minimal tuple if that status for
>>> given attribute is not available tts_isnull. Should we instead add a
>>> callback attisnull() which can use something like heap_isnull()?
>>
>
> This is still a TODO.

Done. I also noticed that slot_getattr() optimizes the cases when the
requested attributes is NULL or is missing from a tuple. Given that a
custom TupleTableSlot type can have its own optimizations for the
same, added a new call back getattr() to obtain value of a given
attribute from slot. The callback is called from slot_getattr().

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v4.tar.zip (85K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
Hi,

Working on rebasing the pluggable storage patch on the current version
of this.

On 2018-07-26 17:09:08 +0530, Ashutosh Bapat wrote:
> Done. I also noticed that slot_getattr() optimizes the cases when the
> requested attributes is NULL or is missing from a tuple. Given that a
> custom TupleTableSlot type can have its own optimizations for the
> same, added a new call back getattr() to obtain value of a given
> attribute from slot. The callback is called from slot_getattr().

I'm quite against this. This is just proliferation of callbacks without
much use.  Why do you think this is helpful?  I think it's much better
to go back to a single callback to deform here.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Sun, Aug 5, 2018 at 3:49 PM, Andres Freund <[hidden email]> wrote:
> Hi,
>
> Working on rebasing the pluggable storage patch on the current version
> of this.

Thanks. Please let me know if you see any issues.

>
> On 2018-07-26 17:09:08 +0530, Ashutosh Bapat wrote:
>> Done. I also noticed that slot_getattr() optimizes the cases when the
>> requested attributes is NULL or is missing from a tuple. Given that a
>> custom TupleTableSlot type can have its own optimizations for the
>> same, added a new call back getattr() to obtain value of a given
>> attribute from slot. The callback is called from slot_getattr().
>
> I'm quite against this. This is just proliferation of callbacks without
> much use.  Why do you think this is helpful?  I think it's much better
> to go back to a single callback to deform here.
>

I think, I explained why getattr() needs to be a separate callback.
There's a reason why slot_getattr() more code than just calling
slot_getsomeattrs() and return the required one - the cases when the
requested attribute is NULL or is missing from a tuple.  Depending
upon the tuple layout access to a simple attribute can be optimized
without spending cycles to extract attributes prior to that one.
Columnar store (I haven't seen Haribabu's patch), for example, stores
columns from multiple rows together and thus doesn't have a compact
version of tuple. In such cases extracting an individual attribute
directly is far cheaper than extracting all the previous attributes.
Why should we force the storage API to extract all the attributes in
such a case?

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
Hi,

On 2018-08-06 10:08:24 +0530, Ashutosh Bapat wrote:

> I think, I explained why getattr() needs to be a separate callback.
> There's a reason why slot_getattr() more code than just calling
> slot_getsomeattrs() and return the required one - the cases when the
> requested attribute is NULL or is missing from a tuple.  Depending
> upon the tuple layout access to a simple attribute can be optimized
> without spending cycles to extract attributes prior to that one.
> Columnar store (I haven't seen Haribabu's patch), for example, stores
> columns from multiple rows together and thus doesn't have a compact
> version of tuple. In such cases extracting an individual attribute
> directly is far cheaper than extracting all the previous attributes.

OTOH, it means we continually access the null bitmap instead of just
tts_isnull[i].

Your logic about not deforming columns in this case would hold for *any*
deforming of previous columns as well. That's an optimization that we
probably want to implement at some point (e.g. by building a bitmap of
needed columns in the planner), but I don't think we should do it
together with this already large patchset.


> Why should we force the storage API to extract all the attributes in
> such a case?

Because there's no need yet, and it complicates the API without
corresponding benefit.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Mon, Aug 6, 2018 at 10:15 AM, Andres Freund <[hidden email]> wrote:

> Hi,
>
> On 2018-08-06 10:08:24 +0530, Ashutosh Bapat wrote:
>> I think, I explained why getattr() needs to be a separate callback.
>> There's a reason why slot_getattr() more code than just calling
>> slot_getsomeattrs() and return the required one - the cases when the
>> requested attribute is NULL or is missing from a tuple.  Depending
>> upon the tuple layout access to a simple attribute can be optimized
>> without spending cycles to extract attributes prior to that one.
>> Columnar store (I haven't seen Haribabu's patch), for example, stores
>> columns from multiple rows together and thus doesn't have a compact
>> version of tuple. In such cases extracting an individual attribute
>> directly is far cheaper than extracting all the previous attributes.
>
> OTOH, it means we continually access the null bitmap instead of just
> tts_isnull[i].

Nope. The slot_getattr implementation in these patches as well as in
the current master return from tts_isnull if the array has the
information.

>
> Your logic about not deforming columns in this case would hold for *any*
> deforming of previous columns as well. That's an optimization that we
> probably want to implement at some point (e.g. by building a bitmap of
> needed columns in the planner), but I don't think we should do it
> together with this already large patchset.

Agree about optimizing using a separate bitmap indicating validity of
a particular value.

>
>
>> Why should we force the storage API to extract all the attributes in
>> such a case?
>
> Because there's no need yet, and it complicates the API without
> corresponding benefit.

The earlier version of slot_getattr() was optimized to access a given
attribute. That must have been done for some reason. Leaving that
optimization aside for this work will cause regression in the cases
where that optimization matters.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
In reply to this post by Ashutosh Bapat
On Thu, Jul 26, 2018 at 5:09 PM, Ashutosh Bapat
<[hidden email]> wrote:

>>
>>>
>>>>
>>>> 3. compile with LLVM and fix any compilation and regression errors.
>>>
>>> When I compiled server with just 0003 applied with LLVM, the
>>> compilation went well, but there was a server crash. That patch
>>> changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
>>> the crash with a debug LLVM build, but couldn't complete the work.
>>> Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
>>> fix that crash. I think, we should make it easy to change the data
>>> types of the members in structures shared by JIT and non-JIT code, may
>>> be automatically create both copies of the code somehow. I will get
>>> back to this after addressing other TODOs.
>>>
Still a TODO.

>
>>
>>>
>>>>
>>>> 6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
>>>> and invoked on the destination slot type.
>>

Done.

With this set of patches, make check-world passes clean.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v5.tar.zip (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Wed, Aug 8, 2018 at 5:07 PM, Ashutosh Bapat
<[hidden email]> wrote:

Amit Khandekar offlist told me that the previous patch-set doesn't
apply cleanly on the latest head.

PFA patches rebased on 31380bc7c204e7cfa9c9e1c62947909e2b38577c

>>>>>
>>>>> 3. compile with LLVM and fix any compilation and regression errors.
>>>>
>>>> When I compiled server with just 0003 applied with LLVM, the
>>>> compilation went well, but there was a server crash. That patch
>>>> changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
>>>> the crash with a debug LLVM build, but couldn't complete the work.
>>>> Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
>>>> fix that crash. I think, we should make it easy to change the data
>>>> types of the members in structures shared by JIT and non-JIT code, may
>>>> be automatically create both copies of the code somehow. I will get
>>>> back to this after addressing other TODOs.
>>>>
>
still a TODO

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v6.tar.zip (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
PFA patches rebased on ee80124811908ef1d4679296c46e36bd8a32b9de.

On Thu, Aug 9, 2018 at 5:12 PM, Ashutosh Bapat
<[hidden email]> wrote:
>>>>>>
>>>>>> 3. compile with LLVM and fix any compilation and regression errors.
>>>>>
>>>>> When I compiled server with just 0003 applied with LLVM, the
>>>>> compilation went well, but there was a server crash. That patch
>>>>> changes type of tts_nvalid from int32 to AttrNumber.

Done. Fixed the crash in the attached patch set. Thanks Andres for offlist help.

I also tried compiling the whole patch-set with LLVM. I have fixed
compilation errors in llvm_compile_expr().

The compilation errors in slot_compile_deform() requite more work.
That function accesses tuple, slow and offset properties from a
TupleTableSlot. The tuple and offset properties are now moved to
HeapTupleTableSlot so they can't be accessed from a TupleTableSlot
unless it's HeapTupleTableSlot or BufferHeapTupleTableSlot and even
then TupleTableSlot needs to be casted into a HeapTupleTableSlot for
accessing those properties. I do not know yet as to how casting works
in LLVM code. slow property of a TupleTableSlot is now available
through tts_flags. We need to add LLVM code to fetch tts_flags and
perform bit operation on it to get or set slow property. I haven't
found any precedence for LLVM bit operations in postgresql's JIT code.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v7.tar.zip (90K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
Hi,

On 2018-08-17 12:10:20 +0530, Ashutosh Bapat wrote:
> We need to add LLVM code to fetch tts_flags and
> perform bit operation on it to get or set slow property. I haven't
> found any precedence for LLVM bit operations in postgresql's JIT code.

There are several, look for the infomask accesses in
slot_compiler_deform.

I'll try to do the adaption later today.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
On 2018-08-17 01:07:06 -0700, Andres Freund wrote:

> Hi,
>
> On 2018-08-17 12:10:20 +0530, Ashutosh Bapat wrote:
> > We need to add LLVM code to fetch tts_flags and
> > perform bit operation on it to get or set slow property. I haven't
> > found any precedence for LLVM bit operations in postgresql's JIT code.
>
> There are several, look for the infomask accesses in
> slot_compiler_deform.
>
> I'll try to do the adaption later today.
Attached is a series of patches doing so.  The previous implementation
of sysvar accesses wasn't actually working - the slot argument was
uninitialized.

I also noticed an independent issue in your changes to
ExecInitScanTupleSlot(): You can't assume that the plan belonging to the
ScanState have a Scan node in their plan. Look e.g. at Material, Sort
etc. So currently your scanrelid access is often just uninitialized
data.

Greetings,

Andres Freund

0001-Fix-slot-type-used-in-subqueryscan.patch (1008 bytes) Download Attachment
0002-XXX-Copy-slot-in-nodeMaterial.c.patch (1K) Download Attachment
0003-Fix-JIT-calls-to-ExecEvalSysVar.patch (3K) Download Attachment
0004-First-attempt-at-support-JITing-of-tuple-deforming-w.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Sat, Aug 18, 2018 at 8:53 PM, Andres Freund <[hidden email]> wrote:

> On 2018-08-17 01:07:06 -0700, Andres Freund wrote:
>> Hi,
>>
>> On 2018-08-17 12:10:20 +0530, Ashutosh Bapat wrote:
>> > We need to add LLVM code to fetch tts_flags and
>> > perform bit operation on it to get or set slow property. I haven't
>> > found any precedence for LLVM bit operations in postgresql's JIT code.
>>
>> There are several, look for the infomask accesses in
>> slot_compiler_deform.
>>
>> I'll try to do the adaption later today.
>
> Attached is a series of patches doing so.  The previous implementation
> of sysvar accesses wasn't actually working - the slot argument was
> uninitialized.
Sorry for that. I couldn't test it since the code wasn't compiling.
The changes to slot_compile_deform changes you provided in the patch
fix the compilation errors. Thanks for those changes.

>
> I also noticed an independent issue in your changes to
> ExecInitScanTupleSlot(): You can't assume that the plan belonging to the
> ScanState have a Scan node in their plan. Look e.g. at Material, Sort
> etc. So currently your scanrelid access is often just uninitialized
> data.

I have incorporated changes in your patches into the relevant patches
in the updated patch-set. With this patch-set make check-world passes
for me.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v8.tar.zip (93K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Andres Freund
On 2018-08-20 17:51:38 +0530, Ashutosh Bapat wrote:
> > I also noticed an independent issue in your changes to
> > ExecInitScanTupleSlot(): You can't assume that the plan belonging to the
> > ScanState have a Scan node in their plan. Look e.g. at Material, Sort
> > etc. So currently your scanrelid access is often just uninitialized
> > data.
>
> I have incorporated changes in your patches into the relevant patches
> in the updated patch-set. With this patch-set make check-world passes
> for me.

Have you addressed the issue I commented on above?

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: TupleTableSlot abstraction

Ashutosh Bapat
On Mon, Aug 20, 2018 at 5:58 PM, Andres Freund <[hidden email]> wrote:

> On 2018-08-20 17:51:38 +0530, Ashutosh Bapat wrote:
>> > I also noticed an independent issue in your changes to
>> > ExecInitScanTupleSlot(): You can't assume that the plan belonging to the
>> > ScanState have a Scan node in their plan. Look e.g. at Material, Sort
>> > etc. So currently your scanrelid access is often just uninitialized
>> > data.
>>
>> I have incorporated changes in your patches into the relevant patches
>> in the updated patch-set. With this patch-set make check-world passes
>> for me.
>
> Have you addressed the issue I commented on above?
Sorry, forgot about that. Here's the patch set with that addressed.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pg_abstract_tts_patches_v9.tar.zip (95K) Download Attachment
12