Re: Parallel INSERT (INTO ... SELECT ...)

On Fri, Jan 22, 2021 at 6:21 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> Hi greg,
>
> Thanks for debugging this.
>
> May be I missed something. I am not sure about the case when rel->rd_index was NULL.
> Because, In function BuildIndexInfo, it seems does not have NULL-check for index->rd_index.
> Like the following:
> ----
> BuildIndexInfo(Relation index)
> {
> IndexInfo *ii;
> Form_pg_index indexStruct = index->rd_index;
> int i;
> int numAtts;
>
> /* check the number of keys, and copy attr numbers into the IndexInfo */
> numAtts = indexStruct->indnatts;
> ----
>
> And the patch do not have NULL-check for index->rd_index too.
> So I thought we can assume index->rd_index is not null, but it seems I may missed something ?
>
> Can you please share the test case with me ?
>
> I use the following code to replace the call of BuildIndexInfo.
> And the installcheck passed.
>
> Example:
> + Form_pg_index indexStruct = index_rel->rd_index;
> + List *ii_Expressions = RelationGetIndexExpressions(index_rel);
> + int ii_NumIndexAttrs = indexStruct->indnatts;
> + AttrNumber ii_IndexAttrNumbers[INDEX_MAX_KEYS];
>
> + for (int i = 0; i < ii_NumIndexAttrs; i++)
> + ii_IndexAttrNumbers[i] = indexStruct->indkey.values[i];

Sorry, I was looking at rel->rd_index, not index_rel->rd_index, my fault.
Your code looks OK. I've taken it and reduced some of the lines and
got rid of the C99-only intermingled variable declarations (see
https://www.postgresql.org/docs/13/source-conventions.html)
The changes are below.
The regression tests all pass, so should be OK (my test case was taken
from insert_parallel regression tests).
Thanks for your help.

- Oid index_oid = lfirst_oid(lc);
- Relation index_rel;
- IndexInfo *index_info;
+ Relation index_rel;
+ Form_pg_index indexStruct;
+ List *ii_Expressions;
+ Oid index_oid = lfirst_oid(lc);

index_rel = index_open(index_oid, lockmode);

- index_info = BuildIndexInfo(index_rel);
+ indexStruct = index_rel->rd_index;
+ ii_Expressions = RelationGetIndexExpressions(index_rel);

- if (index_info->ii_Expressions != NIL)
+ if (ii_Expressions != NIL)
{
int i;
- ListCell *index_expr_item =
list_head(index_info->ii_Expressions);
+ ListCell *index_expr_item = list_head(ii_Expressions);

- for (i = 0; i < index_info->ii_NumIndexAttrs; i++)
+ for (i = 0; i < indexStruct->indnatts; i++)
{
- int keycol = index_info->ii_IndexAttrNumbers[i];
+ int keycol = indexStruct->indkey.values[i];

if (keycol == 0)
{
@@ -912,7 +914,7 @@ index_expr_max_parallel_hazard_for_modify(Relation rel,
return true;
}

- index_expr_item =
lnext(index_info->ii_Expressions, index_expr_item);
+ index_expr_item = lnext(ii_Expressions, index_expr_item);
}
}

Regards,
Greg Nancarrow
Fujitsu Australia