Thank you for the labor for polishing this patch. I have no obvious objection at a glance on this new patch.
I agree to commit this if you this is pertinent to commit except for the issue newly revealed by this patch. Though could you let me have some more time to examine this by myself and fully understand the changes what you made? At Wed, 26 Feb 2014 23:29:35 -0500, Noah Misch wrote > > Finally, the result we got has proved not to be a problem. > > The first union branch should return two rows, and the second union branch > should return one row, for a total of three. In any case, I see later in your > mail that you fixed this. I got it. I confirmed that *after* fixing duplicate rows, then misunderstood your point. > The larger point is that this patch has no business > changing the output rows of *any* query. Its goal is to pick a more-efficient > plan for arriving at the same answer. If there's a bug in our current output > for some query, that's a separate discussion from the topic of this thread. Totally agreed. > > Second, about the crash in this sql, > > > > > select parent from parent union all select parent from parent; > > > > It is ignored whole-row reference (=0) which makes the index of > > child translated-vars list invalid (-1). I completely ignored it > > in spite that myself referred to before. > > > > Unfortunately ConvertRowtypeExpr prevents appendrels from being > > removed currently, and such a case don't seem to take place so > > often, so I decided to exclude the case. > > > + /* > > + * Appendrels which does whole-row-var > > conversion cannot be > > + * removed. ConvertRowtypeExpr can convert only > > RELs which can > > + * be referred to using relid. > > We have parent and child relids, so it is not clear to me how imposing that > restriction helps us. I replaced transvars_merge_mutator() with a call to > adjust_appendrel_attrs(). This reduces code duplication, and it handles > whole-row references. Thank you, I didn't grasp them as a whole.. > (I don't think the other nodes adjust_appendrel_attrs() > can handle matter to this caller. translated_vars will never contain join > tree nodes, and I doubt it could contain a PlaceHolderVar with phrels > requiring translation.) > > The central design question for this patch seems to be how to represent, in > the range table, the fact that we expanded an inheritance parent despite its > children ending up as appendrel children of a freestanding UNION ALL. The v6 > patch detaches the original RTE from the join tree and clears its "inh" flag. > This breaks sepgsql_dml_privileges(), which looks for RTE_RELATION with inh = > true and consults selinux concerning every child table. Mmm. It was not in my sight. A bit more time is needed to understand this. > We could certainly > change the way sepgsql discovers inheritance hierarchies, but nothing clearly > better comes to mind. I selected the approach of preserving the RTE's "inh" > flag, removing the AppendRelInfo connecting that RTE to its enclosing UNION > ALL, and creating no AppendRelInfo children for that RTE. Ok, I did that because I was not certain whether removing "detached" AppendRelInfos are safe or not. It is far smarter than mine. > An alternative was > to introduce a new RTE flag, say "append". An inheritance parent under a > UNION ALL would have append = false, inh = true; other inheritance parent RTEs > would have append = true, inh = true; an RTE for UNION ALL itself would have > append = true, inh = false. I think that kind of complexity is not necessary for this issue. > > > > > > > The attached two patches are rebased to current 9.4dev HEAD and > > > > > > > make check at the topmost directory and src/test/isolation are > > > > > > > passed without error. One bug was found and fixed on the way. It > > > > > > > was an assertion failure caused by probably unexpected type > > > > > > > conversion introduced by collapse_appendrels() which leads > > > > > > > implicit whole-row cast from some valid reltype to invalid > > > > > > > reltype (0) into adjust_appendrel_attrs_mutator(). > > > > > > > > > > > > What query demonstrated this bug in the previous type 2/3 patches? > > > > > > > > I would still like to know the answer to the above question. > > > > I rememberd after some struggles. It failed during 'make check', > > on the following query in inherit.sql. > > > [details] > > Interesting. Today, the parent_reltype and child_reltype fields of > AppendRelInfo are either both valid or both invalid. Your v6 patch allowed us > to have a valid child_reltype with an invalid parent_reltype. At the moment, > we can't benefit from just one valid reltype. I restored the old invariant. Ok, I understood it. > If the attached patch version looks reasonable, I will commit it. > > > Incidentally, I tried adding an assertion that append_rel_list does not show > one appendrel as a direct child of another. The following query, off-topic > for the patch at hand, triggered that assertion: > > SELECT 0 FROM (SELECT 0 UNION ALL SELECT 0) t0 > UNION ALL > SELECT 0 FROM (SELECT 0 UNION ALL SELECT 0) t0; This seems not to crash unless this patch is applied, but itself doesn't seem to be a bug. I think it should be cured along with this patch even if it is not the issue of this patch. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
