On Thu, Dec 10, 2020 at 3:50 PM vignesh C <vignesh21@gmail.com> wrote:
> Few comments:
> + Node *index_expr;
> +
> + if (index_expr_item == NULL)
> /* shouldn't happen */
> + elog(ERROR, "too few
> entries in indexprs list");
> +
> + index_expr = (Node *)
> lfirst(index_expr_item);
>
> We can change this elog to below to maintain consistency:
> if (index_expr_item == NULL) /* shouldn't happen */
> {
> context->max_hazard = PROPARALLEL_UNSAFE;
> return context->max_hazard;
> }
>
Thanks. I think you pointed out something similar to this before, but
somehow I must have missed updating this as well (I was just following
existing error handling for this case in the Postgres code).
I'll update it as you suggest, in the next version of the patch I post.
> static HeapTuple
> heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
> CommandId cid, int options)
> {
> /*
> * To allow parallel inserts, we need to ensure that they are safe to be
> * performed in workers. We have the infrastructure to allow parallel
> * inserts in general except for the cases where inserts generate a new
> * CommandId (eg. inserts into a table having a foreign key column).
> */
> I felt we could remove the above comments or maybe rephrase it.
>
That is Amit's comment, and I'm reluctant to change it because it is
still applicable even after application of this patch.
Amit has previously suggested that I add an Assert here, to match the
comment (to replace the original Parallel-worker error-check that I
removed), so I am looking into that.
Regards,
Greg Nancarrow
Fujitsu Australia