On Mon, Sep 11, 2017 at 2:16 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2017/09/11 16:23, Ashutosh Bapat wrote: >> On Sat, Sep 9, 2017 at 6:28 AM, Robert Haas <robertmh...@gmail.com> wrote: >>> I'm a bit suspicious about the fact that there are now executor >>> changes related to the PlanRowMarks. If the rowmark's prti is now the >>> intermediate parent's RT index rather than the top-parent's RT index, >>> it'd seem like that'd matter somehow. Maybe it doesn't, because the >>> code that cares about prti seems to only care about whether it's >>> different from rti. But if that's true everywhere, then why even >>> change this? I think we might be well off not to tinker with things >>> that don't need to be changed. >> >> In the definition of ExecRowMark, I see >> Index prti; /* parent range table index, if child */ >> It just says parent, by which I take as direct parent. For >> inheritance, which earlier flattened inheritance hierarchy, direct >> parent was top parent. So, probably nobody thought whether a parent is >> direct parent or top parent. But now that we have introduced that >> concept we need to interpret this comment anew. And I think >> interpreting it as direct parent is non-lossy. > > But then we also don't have anything to say about why we're making that > change. If you could describe what non-lossy is in this context, then > fine.
By setting prti to the topmost parent RTI we are loosing information that this child may be an intermediate child similar to what we did earlier to AppendRelInfo. That's the lossy-ness in this context. > But that we'd like to match with what we're going to do for > AppendRelInfos does not seem to be a sufficient explanation for this change. The purpose of this patch is to change the parent-child linkages for partitioned table and prti is one of them. So, in fact, I am wondering why not to change that along with AppendRelInfo. > >> If we set top parent's >> index, parent RTI in AppendRelInfo and PlanRowMark would not agree. >> So, it looks quite natural that we set the direct parent's index in >> PlanRowMark. > > They would not agree, yes, but aren't they unrelated? If we have a reason > for them to agree, (for example, row-locking breaks in the inherited table > case if we didn't), then we should definitely make them agree. > > Updating the comment for prti definition might be something that this > patch could (should?) do, but I'm not quite sure about that too. > To me that looks backwards again for the reasons described above. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
