TRAFODION-2265

[narendragoyal]

[TRAFODION-2265]

Parse ex_conv_clause::convInstrStrInfo at sql process startup
(in sqInit) into a sparsely populated index for quick reference by
ex_conv_clause::findInstruction.

[Traf-Jenkins]

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1220/

[Traf-Jenkins]

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1220/

[DaveBirdsall]

+1 Looks good, but there is a very tiny window where a thread-safety issue might cause incorrect results.

[DaveBirdsall]

I think this logic is idempotent to it probably doesn't matter but it looks like two threads could do the calloc, one of them will win in setting sv_convIndexSparse, then both of them could interleave in the initialization logic below. Could be the fast one sets the sv_instrOffsetIndexPopulated flag to true while the slow one is in the Initialize to -1 loop so theoretically we could get wrong answers. Might be safer to use a local variable here and only set sv_convIndexSparse at the very end just before setting the boolean. Then the worst thing that happens is we allocated the array twice but it will never be inconsistent.

[narendragoyal]

Thanks Dave! Agreed - will take care of it as you suggested.

I will also remove the call to ex_conv_clause::populateInstrOffsetIndex() from ex_conv_clause::getInstrOffset. This way ex_conv_clause::populateInstrOffsetIndex() will only be called from sqInit().

[Traf-Jenkins]

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1222/

[anoopsharma00]

conversion instructions are usually computed during compile of a query and
then stored in conversion clause. If an instruction of CONV_NOT_SUPPORTED
shows up in conversion clause at runtime, then need to find out why a supported
instruction was not generated during compile time. That may point to some other
issue related to instruction generation. This happens in exp/exp_fixup.cpp

Some scenarios where instructions are not generated is if expressions are not used
to do a runtime conversion. This could happen if an input value is being moved from app
to cli, or a value is being inserted into a table without the use of expression (like during
bulk load or hive insert).

In the query example of this jira, was an expression used to insert or was instruction being
computed on the fly at runtime?

This change will help in fast computation of conversion instructions but we also need
to find out if there is some other root cause that resulted in this instruction not being
generated at compile time.

Another thing: in addition to conversions, instructions are also computed for comparison
and arithmetic operations.

[selvaganesang]

Looks good. Please avoid calling ex_conv_clause::populateInstrOffsetIndex() at other places than sqInit as you have already mentioned in your response

[Traf-Jenkins]

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1222/

[selvaganesang]

Are we ready to merge this change in?. The other changes requested by @anoopsharma00 can be done in phases using the same JIRA.

[selvaganesang]

@narendragoyal , it might be good to edit add the JIRA heading in the subject line in github. This would help other reviewers to decide if he/she would want to review this PR,

[narendragoyal]

As part of the upsert (of the case in question), EXP_FIXED_OV_ADD or EXP_FIXED_OV_MUL functions are being called that then result in the findInstruction() being called many times
...
(gdb) bt
#0 EXP_FIXED_OV_ADD (op1=0, op2=2, ov=0x7ffca119879e) at ../exp/exp_arith.cpp:112
#1 0x00007f753fd4f8d5 in ex_expr::evalPCode (this=0x7f7530db6bd0, pCode32=0x7f7530db6cf0, atp1=0x7f7530d774f0, atp2=0x7f7530d781e8, rowLen=0x7ffca119a69c) at ../exp/exp_eval.cpp:4317
#2 0x00007f75420945ac in ex_expr::eval (this=0x7f7530db6bd0, atp1=0x7f7530d774f0, atp2=0x7f7530d781e8, exHeap=0x0, datalen=-1, rowLen=0x7ffca119a69c, lastFldIndex=0x0, fetchedDataPtr=0x0) at ../exp/exp_expr.h:373
#3 0x00007f754194caa8 in Cluster::insert (this=0x7f751cf5b818, newEntry=0x7f7530d774f0, moveExpr=0x7f7530db6bd0, hashValue=666, rc=0x7f7530d77b14, skipMemoryCheck=0) at ../executor/cluster.cpp:1554
....
...

The EXP_FIXED_OV_* functions call convDoIt() as follows (without passing the conv function (effectively 'unknown' ). We could get the conversion function once and set it in the call (that way 'convDoIt' does not have to compute it all the time).
rc = convDoIt(op1_data[1], 16, REC_NUM_BIG_SIGNED, 20, 0,
op1_data[0], 8, REC_BIN64_SIGNED, 0, 0,
NULL, 0, NULL, NULL)

Perhaps we can do that as part of a follow on checkin for the JIRA.

[DaveBirdsall]

@selvaganesang, @anoopsharma00: Is this change ready to merge?

[selvaganesang]

+1

[selvaganesang]

@anoopsharma00 Is it ok to merge this PR?

[selvaganesang]

I will merge this in. If there is any other fix needed further, it is good to create a new Jira