(2010/01/28 5:42), Robert Haas wrote: > On Wed, Jan 27, 2010 at 10:17 AM, KaiGai Kohei<kai...@kaigai.gr.jp> wrote: >> (2010/01/27 23:29), Robert Haas wrote: >>> >>> 2010/1/27 KaiGai Kohei<kai...@ak.jp.nec.com>: >>>> >>>> The attached patch is revised one based on the V3 approach. >>>> The only difference from V3 is that it also applies checks on the >>>> AT_AlterColumnType option, not only renameatt(). >>> >>> I think I was clear about what the next step was for this patch in my >>> previous email, but let me try again. >>> >>> http://archives.postgresql.org/pgsql-hackers/2010-01/msg02407.php >>> >>> See also Tom's comments here: >>> >>> http://archives.postgresql.org/pgsql-hackers/2010-01/msg00110.php >>> >>> I don't believe that either Tom or I are prepared to commit a patch >>> based on this approach, at least not unless someone makes an attempt >>> to do it the other way and finds an even more serious problem. If >>> you're not interested in rewriting the patch along the lines Tom >>> suggested, then we should just mark this as Returned with Feedback and >>> move on. >> >> The V3/V5 patch was the rewritten one based on the Tom's comment, as is. >> It counts the expected inhcount at the first find_all_inheritors() time >> at once, and it compares the pg_attribute.attinhcount. >> (In actually, find_all_inheritors() does not have a capability to count >> the number of merged from a common origin, so I newly defined the >> find_all_inheritors_with_inhcount().) >> >> Am I missing something? > > Err... I'm not sure. I thought I understood what the different > versions of this patch were doing, but apparently I'm all confused. > I'll take another look at this.
Just to be safe, I'd like to introduce the differences between revisions. * The V2 patch http://archives.postgresql.org/message-id/4b3f6353.3080...@kaigai.gr.jp It just checked whether the pg_attribute.inhcount is larger than 1, or not. But it was problematic when diamond-inheritance case. * The V3 patch http://archives.postgresql.org/message-id/4b41bb04.2070...@ak.jp.nec.com It computed an expected-inhcount for each relations when renameatt() picks up all the child relations on the top of recursion level. Its cost to walk on the inheritance tree is similar to existing code except for a case when we have many diamond-inheritance, because find_all_inheritors() does not need to walk on the child relations that was already checked, but find_all_inheritors_with_inhcount() has to walk on these children to compute multiplicity. See the following example, T2 / \ T1 T4 - T5 \ / T3 In this case, find_all_inheritors() encounter T4 with T1-T2 path and T1-T3 path. But it does not need to scan T5 on the second time, because it already chains OID of the T4 and T5 with the first walks, and list_concat_unique_oid() does not add duplicated OIDs anymore. But find_all_inheritors_with_inhcount() needs to walks on the T4-T5 path to compute the number of times being inherited, even if second walks. Your testcase emphasized this difference so much. But, in my opinion, it is quite natural because the existing code does not apply necessary checks. * The V4 patch http://archives.postgresql.org/message-id/4b4ec1f1.30...@ak.jp.nec.com I found out ALTER COLUMN TYPE also has same issue. But ATPrepAlterColumnType() did recursive scan using ATSimpleRecursion(), so it needs to modify the logic in this function. At that time, I preferred to compute an expected inhcount for each recursion level, rather than modifying the logic in ATPrepAlterColumnType(), although its cost was larger than find_all_inheritors_with_inhcount(). * The V5 patch http://archives.postgresql.org/message-id/4b5ffe4b.1060...@ak.jp.nec.com We can find out a performance matter in the V4 patch, so I reverted the base logic into V3 approach. In addition to the reverting, I also modified the ATPrepAlterColumnType() to check whether the column to be altered is inherited from multiple origin, or not. > Bernd (or anyone), feel free to take a look in parallel. More eyes > would be helpful... > > ...Robert > -- OSS Platform Development Division, NEC KaiGai Kohei <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
