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

On Sat, Jun 10, 2017 at 6:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.

Not just a whitespace change: added "trigdesc == NULL" and put the
existing stuff into parens, necessarily changing indentation level.

> + 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.

Fixed. Similar code existed in copy.c, so fixed there too.

> * 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.

I added a comment to make that clear.

> * 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.

Yeah. I think that's right.

Note that in this patch I was trying to cater to execReplication.c so
that it wouldn't have to construct a TransitionCaptureState. It
doesn't actually support hierarchies (it doesn't operate on
partitioned tables, and doesn't have the smarts to direct updates to
traditional inheritance children), and doesn't fire statement
triggers.

In the #2 patch in the other thread about wCTEs, I changed this to
make a TransitionCaptureState object the *only* way to cause
transition tuple capture, because in that patch it owns the
tuplestores without which you can't capture anything. So in that
patch trigdesc is no longer relevant at all for the tuple capture.
Someone extending execReplication.c to support partitions and
statement triggers etc will need to think about creating a
TransitionCaptureState but I decided that was out of scope for the
transition table rescue project. In the new version of the #2 patch
that I'm about to post on the other there there is now a comment in
execReplication.c to explain.

> So, in short, +1 from me.

Thanks for the review. New version of patch #1 attached.

--
Thomas Munro
http://www.enterprisedb.com