Hi,
On 2020-03-17 19:49:43 +0900, Amit Langote wrote:
> On Sun, Mar 15, 2020 at 9:36 AM Andres Freund <andres@anarazel.de> wrote:
> > I don't think it's ok for ExecConstraint() to overwrite the tuple
> > descriptor of a slot "owned" by nodeResult.c. Am I missing something, or
> > is that broken?
>
> Looking into it, that partitioning code in ExecConstraint() would not
> "normally" overwrite a slot that is not managed by partitioning.
> Normally, there would be a dedicated "partition" slot that would be
> used, but only if tuple conversion from root parent to leaf partition
> is necessary before routing the tuple.
> What is not "normal" in this case is that tuple conversion is deemed
> necessary when converting from leaf partition to root parent whereas
> not in the other direction. It's because one of the partition's
> attributes has atthasmissing set to true, which triggers this code in
> convert_tuples_by_name():
Hm. I don't think we generally reach this path only with a "partition"
slot? I'm looking at 11, for the purpose of this bug. The INSERT case
"normally" is only reached with the slot returned by
ExecPrepareTupleRouting(), true. But ExecConstraint() is also
e.g. called from ExecUpdate() - and as far as I can tell that will be
either the slot from the plan, or the slot from the junkfilter.
> > It seems fairly insane how many places have this approximate code,
> > btw. In 11 we have a copy of nearly the same ~40 lines each in at least
> > ExecPartitionCheckEmitError(), ExecConstraints (twice) and
> > ExecWitchCheckOption().
>
> That is certainly bad. I think we should get that code in one place
> and inside ExecBuildSlotValueDescription() seems like a good place,
> because that's what needs a converted tuple to build a string from it.
> I have tried that in the attached -- need different patches for
> different branches as there have been a lot of small changes in this
> code over releases.
That certainly looks better.
Greetings,
Andres Freund