Re: UPDATE of partition key

On 25 March 2017 at 01:34, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
I am yet to handle all of your comments, but meanwhile , attached is
> an updated patch, that handles RETURNING.
>
> Earlier it was not working because ExecInsert() did not return any
> RETURNING clause. This is because the setup needed to create RETURNIG
> projection info for leaf partitions is done in ExecInitModifyTable()
> only in case of INSERT. But because it is an UPDATE operation, we have
> to do this explicitly as a one-time operation when it is determined
> that row-movement is required. This is similar to how we do one-time
> setup of mt_partition_dispatch_info. So in the patch, I have moved
> this code into a new function ExecInitPartitionReturningProjection(),
> and now this is called in ExecInitModifyTable() as well as during row
> movement for ExecInsert() processing the returning clause.

> Basically we need to do all that is done in ExecInitModifyTable() for
> INSERT. There are a couple of other things that I suspect that might
> need to be done as part of the missing initialization for Execinsert()
> during row-movement :
> 1. Junk filter handling
> 2. WITH CHECK OPTION

Attached is an another updated patch v4 which does WITH-CHECK-OPTION
related initialization.

So we now have below two function calls during row movement :
/* Build WITH CHECK OPTION constraints for leaf partitions */
ExecInitPartitionWithCheckOptions(mtstate, root_rel);

/* Build a projection for each leaf partition rel. */
ExecInitPartitionReturningProjection(mtstate, root_rel);

And these functions are now re-used at two places : In
ExecInitModifyTable() and in row-movement code.
Basically whatever was not being initialized in ExecInitModifyTable()
is now done in row-movement code.

I have added relevant scenarios in sql/update.sql.

I checked the junk filter handling. I think there isn't anything that
needs to be done, because for INSERT, all that is needed is
ExecCheckPlanOutput(). And this function is anyway called even in
ExecInitModifyTable() even for UPDATE, so we don't have to initialize
anything additional.

> Yet, ExecDelete() during row-movement is still returning the RETURNING
> result redundantly, which I am yet to handle this.
Done above. Now we have a new parameter in ExecDelete() which tells
whether to skip RETURNING.

On 23 March 2017 at 07:04, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Would it be better to have at least some new tests?
Added some more scenarios in update.sql. Also have included scenarios
for WITH-CHECK-OPTION for updatable views.

> Also, there are a few places in the documentation mentioning that such updates cause error,
> which will need to be updated. Perhaps also add some explanatory notes
> about the mechanism (delete+insert), trigger behavior, caveats, etc.
> There were some points discussed upthread that could be mentioned in the
> documentation.
Yeah, I agree. Documentation needs some important changes. I am still
working on them.

> + if (!mtstate->mt_partition_dispatch_info)
> + {
>
> The if (pointer == NULL) style is better perhaps.
>
> + /* root table RT index is at the head of partitioned_rels */
> + if (node->partitioned_rels)
> + {
> + Index root_rti;
> + Oid root_oid;
> +
> + root_rti = linitial_int(node->partitioned_rels);
> + root_oid = getrelid(root_rti, estate->es_range_table);
> + root_rel = heap_open(root_oid, NoLock); /* locked by
> InitPlan */
> + }
> + else
> + root_rel = mtstate->resultRelInfo->ri_RelationDesc;
>
> Some explanatory comments here might be good, for example, explain in what
> situations node->partitioned_rels would not have been set and/or vice versa.
Added some more comments in the relevant if conditions.

>
>> Now, for
>> UPDATE, ExecSetupPartitionTupleRouting() will be called only if row
>> movement is needed.
>>
>> We have to open an extra relation for the root partition, and keep it
>> opened and its handle stored in
>> mt_partition_dispatch_info[0]->reldesc. So ExecEndModifyTable() closes
>> this if it is different from node->resultRelInfo->ri_RelationDesc. If
>> it is same as node->resultRelInfo, it should not be closed because it
>> gets closed as part of ExecEndPlan().
>
> I guess you're referring to the following hunk. Some comments:
>
> @@ -2154,10 +2221,19 @@ ExecEndModifyTable(ModifyTableState *node)
> * Close all the partitioned tables, leaf partitions, and their indices
> *
> * Remember node->mt_partition_dispatch_info[0] corresponds to the root
> - * partitioned table, which we must not try to close, because it is the
> - * main target table of the query that will be closed by ExecEndPlan().
> - * Also, tupslot is NULL for the root partitioned table.
> + * partitioned table, which should not be closed if it is the main target
> + * table of the query, which will be closed by ExecEndPlan().
>
> The last part could be written as: because it will be closed by ExecEndPlan().

Actually I later realized that the relation is not required to be kept
open until ExecEndmodifyTable(). So I reverted the above changes. Now
it is immediately closed once all the row-movement-related setup is
done.
>
> Also, tupslot
> + * is NULL for the root partitioned table.
> */
> + if (node->mt_num_dispatch > 0)
> + {
> + Relation root_partition;
>
> root_relation?
>
> +
> + root_partition = node->mt_partition_dispatch_info[0]->reldesc;
> + if (root_partition != node->resultRelInfo->ri_RelationDesc)
> + heap_close(root_partition, NoLock);
> + }
>
> It might be a good idea to Assert inside the if block above that
> node->operation != CMD_INSERT. Perhaps, also reflect that in the comment
> above so that it's clearer.

This does not apply now since I reverted as mentioned above.

>
>> Looking at that, I found that in the current patch, if there is no
>> row-movement happening, ExecPartitionCheck() effectively gets called
>> twice : First time when ExecPartitionCheck() is explicitly called for
>> row-movement-required check, and second time in ExecConstraints()
>> call. May be there should be 2 separate functions
>> ExecCheckConstraints() and ExecPartitionConstraints(), and also
>> ExecCheckConstraints() that just calls both. This way we can call the
>> appropriate functions() accordingly in row-movement case, and the
>> other callers would continue to call ExecConstraints().
>
> One random idea: we could add a bool ri_PartitionCheckOK which is set to
> true after it is checked in ExecConstraints(). And modify the condition
> in ExecConstraints() as follows:
>
> if (resultRelInfo->ri_PartitionCheck &&
>+ !resultRelInfo->ri_PartitionCheckOK &&
> !ExecPartitionCheck(resultRelInfo, slot, estate))

I have taken out the part in ExecConstraints where it forms and emits
partition constraint error message, and put in new function
ExecPartitionCheckEmitError(), and this is called in ExecConstraints()
as well as in ExecUpdate() when it finds that it is not a partitioned
table. This happens when the UPDATE has been run on a leaf partition,
and when ExecPartitionCheck() fails for the leaf partition. Here, we
just need to emit the same error message that ExecConstraint() emits.