Re: [HACKERS] transition table behavior with inheritance appearsbroken (was: Declarative partitioning - another take) - Mailing list pgsql-hackers

On 2017/05/18 7:13, Thomas Munro wrote:
> On Wed, May 17, 2017 at 7:42 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> On Wed, May 17, 2017 at 6:04 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> targetRelInfo should instead be set to mtstate->rootResultRelInfo that was
>>> set in ExecInitModifyTable() as described above, IOW, as follows:
>>>
>>>     /* Partitioned table. */
>>>     if (mtstate->rootResultRelInfo != NULL)
>>>         targetRelInfo = mtstate->rootResultRelInfo;
>>
>> Ah, I see.  Thank you.  Fixed in the attached.
> 
> Here's a post-pgindent rebase.

I read through the latest patch.  Some comments:

Do we need to update documentation?  Perhaps, some clarification on the
inheritance/partitioning behavior somewhere.

+typedef struct TriggerTransitionState
+{
...
+    bool        ttf_delete_old_table;

Just curious: why ttf_?  TriggerTransition field?

-    Assert((enrmd->reliddesc == InvalidOid) != (enrmd->tupdesc == NULL));
+    Assert((enrmd->reliddesc == InvalidOid) !=
+           (enrmd->tupdesc == NULL));

Perhaps, unintentional change?

+        original_tuple = tuple;        map = mtstate->mt_partition_tupconv_maps[leaf_part_index];        if (map)
 {
 
@@ -570,8 +572,17 @@ ExecInsert(ModifyTableState *mtstate,        setLastTid(&(tuple->t_self));    }

+    /*
+     * If we inserted into a partitioned table, then insert routing logic may
+     * have converted the tuple to a partition's format.  Make the original
+     * unconverted tuple available for transition tables.
+     */
+    if (mtstate->mt_transition_state != NULL)
+        mtstate->mt_transition_state->original_insert_tuple = original_tuple;

I'm not sure if it's significant for transition tables, but what if a
partition's BR trigger modified the tuple?  Would we want to include the
modified version of the tuple in the transition table or the original as
the patch does?  Same for the code in CopyFrom().
 * 'tup_conv_maps' receives an array of TupleConversionMap objects with one *      entry for every leaf partition
(requiredto convert input tuple based *      on the root table's rowtype to a leaf partition's rowtype after tuple
 
- *      routing is done
+ *      routing is done)

Oh, thanks! :)

Other than the above minor nitpicks, patch looks good to me.

Thanks,
Amit