Horton

Re: TupleTableSlot abstraction

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Amit Khandekar <amitdkhan.pg@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Robert Haas <robertmhaas@gmail.com>, Haribabu Kommi <kommi.haribabu@gmail.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2018-10-17T00:02:42Z
Lists: pgsql-hackers 
Hi,

On 2018-10-15 12:12:03 +0530, Amit Khandekar wrote:
> On Sat, 13 Oct 2018 at 04:02, Andres Freund <andres@anarazel.de> wrote:
> > I'd just have ExecInterpExpr() continue calling ExecEvalSysVar. There's
> > no reason for it to be inline.

> Can you explain more why you think there should be a ExecEvalSysVar()
> definition ? As I mentioned earlier it would just call
> slot_getsysattr() and do nothing else.

I think it's superflous to have three opcodes for this, and we should
instead just have one sysvar instruction that chooses the slot based on
a parameter of the opcode.  Inline code in the interpreter isn't free.


> > And it's simpler for JIT than the alternative.
> 
> You mean it would be simpler for JIT to call ExecEvalSysVar() than
> slot_getsysattr() ? I didn't get why it is simpler.

Because there'd be no special code at all. We can just use the code that
existing ExecEval* routines use.


> Or are you talking considering build_ExecEvalSysVar() ? I am ok with
> retaining build_ExecEvalSysVar() , but I was saying even inside this
> function, we could do :
> LLVMBuildCall(.... , llvm_get_decl(mod, FuncSlotGetsysattr) , .....)
> rather than:
> LLVMFunctionType(,...)
> LLVMAddFunction("ExecEvalSysVar", ....)
> LLVMBuildCall(...)

That'd probably generate more work than it'd save, because llvm_get_decl
requires that the function is present in llvmjit_types.c.


> > > I went ahead and did these changes, but for now, I haven't replaced
> > > ExecFetchSlotTuple() with ExecFetchSlotHeapTuple(). Instead, I
> > > retained ExecFetchSlotTuple() to be called for heap tuples, and added
> > > a new ExecFetchGenericSlotTuple() to be used with shouldFree. I don't
> > > like these names, but until we have concluded, I don't want to go
> > > ahead and replace all the numerous ExecFetchSlotTuple() calls with
> > > ExecFetchSlotHeapTuple().
> >
> > Why not?
> 
> I haven't gone ahead because I wanted to know if you are ok with the names.

Just renaming a function is cheap, it's just a oneliner script ;)

FWIW, I dislike ExecFetchGenericSlotTuple() as well. It's still a
HeapTuple, there's absolutely nothing Generic about it.  I don't see why
we'd not just use ExecFetchSlotHeapTuple() with a new shouldFree
parameter?

Greetings,

Andres Freund

Commits

  1. Fix memory leak when inserting tuple at relation creation for CTAS

  2. Build HashState's hashkeys expression with the correct parent.

  3. Move TupleTableSlots boolean member into one flag variable.

  4. Move generic slot support functions from heaptuple.c into execTuples.c.

  5. Remove absolete function TupleDescGetSlot().

  6. Remove function list from prologue of execTuples.c.

  7. Split ExecStoreTuple into ExecStoreHeapTuple and ExecStoreBufferHeapTuple.

  8. Change TupleTableSlot->tts_nvalid to type AttrNumber.

  9. Deduplicate code between slot_getallattrs() and slot_getsomeattrs().

  10. Mention ownership requirements for REFRESH MATERIALIZED VIEW in docs

  11. Spell "partitionwise" consistently.

  12. Fix WHERE CURRENT OF when the referenced cursor uses an index-only scan.