Re: transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

On Fri, Jun 9, 2017 at 12:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 8, 2017 at 11:56 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> [Adding Andrew Gierth]
>>
>> Here is a rebased version of the patch to fix transition tables with
>> inheritance. Fixes a typo in an error message ("not support on
>> partitions" -> "... supported ..."), and changes regression test
>> triggers to be single-event (only one of INSERT, UPDATE or DELETE),
>> because a later patch will not allow multi-event triggers with TTs.
>>
>> This is patch 1 of a stack of 3 patches addressing currently known
>> problems with transition tables.
>
> So, Andrew, are you running with this, or should I keep looking into it?

I have spent some time now studying this patch. I might be missing
something, but to me this appears to be in great shape. A few minor
nitpicks:

- if ((event == TRIGGER_EVENT_DELETE &&
!trigdesc->trig_delete_after_row) ||
- (event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) ||
- (event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row))
+ if (trigdesc == NULL ||
+ (event == TRIGGER_EVENT_DELETE &&
!trigdesc->trig_delete_after_row) ||
+ (event == TRIGGER_EVENT_INSERT &&
!trigdesc->trig_insert_after_row) ||
+ (event == TRIGGER_EVENT_UPDATE &&
!trigdesc->trig_update_after_row))

I suspect the whitespace changes will get reverted by pgindent, making
them pointless. But that's a trivial issue.

+ if (mtstate->mt_transition_capture != NULL)
+ {
+ if (resultRelInfo->ri_TrigDesc &&
+ (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
+ {
+ /*
+ * If there are any BEFORE or INSTEAD triggers on the
+ * partition, we'll have to be ready to convert their result
+ * back to tuplestore format.
+ */
+
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
+ mtstate->mt_transition_capture->tcs_map =
+ mtstate->mt_transition_tupconv_maps[leaf_part_index];
+ }
+ else
+ {
+ /*
+ * Otherwise, just remember the original unconverted tuple, to
+ * avoid a needless round trip conversion.
+ */
+
mtstate->mt_transition_capture->tcs_original_insert_tuple = tuple;
+ mtstate->mt_transition_capture->tcs_map = NULL;
+ }
+ }

This chunk of code gets inserted between a comment and the code to
which that comment refers. Maybe that's not the best idea.

Some other things I thought about:

* Does has_superclass have concurrency issues? After some
consideration, I decided that it's probably fine as long as you hold a
lock on the target relation sufficient to prevent it from concurrently
becoming an inheritance child - i.e. any lock at all. The caller is
CREATE TRIGGER, which certainly does.

* In AfterTriggerSaveEvent, is it a problem that the large new hunk of
code ignores trigdesc if transition_capture != NULL? If I understand
correctly, the trigdesc will be coming from the leaf relation actually
being updated, while the transition_capture will be coming from the
relation named in the query. Is the transition_capture object
guaranteed to have all the flags set, or do we also need to include
the ones from the trigdesc? This also seems to be fine, because of
the restriction that row-level triggers with tuplestores can't
participate in inheritance hierarchies. We can only need to capture
the tuples for the relation named in the query, not the leaf
partitions.

* The regression tests for this function are fairly lengthy. Given
the complexity of the behavior being tested, though, it seems like a
really good idea to have these. Otherwise, it's easy to imagine some
future patch breaking this again.

I also like the documentation update.

So, in short, +1 from me.

Regards,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company