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
