Hi,
On 2019-04-22 14:48:26 +0900, Michael Paquier wrote:
> On Fri, Apr 19, 2019 at 09:43:01AM -0500, Justin Pryzby wrote:
> > Thanks for committing those portions.
>
> I have done an extra pass on your patch set to make sure that I am
> missing nothing, and the last two remaining places which need some
> tweaks are the comments from the JIT code you pointed out. Attached
> is a patch with these adjustments.
> --
> Michael
> diff --git a/src/backend/jit/llvm/llvmjit_deform.c
> b/src/backend/jit/llvm/llvmjit_deform.c
> index 94b4635218..e7aa92e274 100644
> --- a/src/backend/jit/llvm/llvmjit_deform.c
> +++ b/src/backend/jit/llvm/llvmjit_deform.c
> @@ -298,9 +298,9 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc
> desc,
> }
>
> /*
> - * Check if's guaranteed the all the desired attributes are available in
> - * tuple. If so, we can start deforming. If not, need to make sure to
> - * fetch the missing columns.
> + * Check if all the desired attributes are available in the tuple. If
> so,
> + * we can start deforming. If not, we need to make sure to fetch the
> + * missing columns.
> */
That's imo not an improvement. The guaranteed bit is actually
relevant. What this block is doing is eliding the check against the
tuple header for the number of attributes, if NOT NULL attributes for
later columns guarantee that the desired columns are present in the NULL
bitmap. But the rephrasing makes it sound like we're actually checking
against the tuple.
I think it'd be better just to fix s/the all/that all/.
> if ((natts - 1) <= guaranteed_column_number)
> {
> @@ -383,7 +383,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc
> desc,
>
> /*
> * If this is the first attribute, slot->tts_nvalid was 0.
> Therefore
> - * reset offset to 0 to, it be from a previous execution.
> + * reset offset to 0 too, as it may be from a previous
> execution.
> */
> if (attnum == 0)
> {
That obviously makes sense.
Greetings,
Andres Freund
