Prev: [HACKERS] More frame options in window functions
Next: [HACKERS] psql tab completion for DO blocks
From: KaiGai Kohei on 2 Jan 2010 10:16
(2009/12/30 10:38), Robert Haas wrote:
> 2009/12/16 KaiGai Kohei<kaigai(a)ak.jp.nec.com>:
>> It is a patch for the matter which I reported before.
>>
>> When a column is inherited from multiple relations, ALTER TABLE with
>> RENAME TO option is problematic.
>> This patch fixes the matter. In correctly, it prevent to rename columns
>> inherited from multiple relations and merged.
>
> No longer applies. Can you rebase?

The attached patch is the rebased revision.

Thanks,
--
KaiGai Kohei <kaigai(a)kaigai.gr.jp>
From: KaiGai Kohei on 23 Jan 2010 22:48
(2010/01/24 12:29), Robert Haas wrote:
> On Sat, Jan 23, 2010 at 1:45 PM, Bernd Helmle<mailings(a)oopsware.de> wrote:
>> --On 14. Januar 2010 16:04:17 +0900 KaiGai Kohei<kaigai(a)ak.jp.nec.com>
>> wrote:
>>> This patch adds:
>>>
>>> List *find_column_origin(Oid relOid, const char *colName)
>>>
>>> It returns the list of relation OIDs which originally defines the given
>>> column. In most cases, it returns a list with an element. But, if the
>>> column is inherited from multiple parent relations and merged during the
>>> inheritance tree, the returned list contains multiple OIDs.
>>> In this case, we have to forbid changing type and renaming to keep
>>> correctness of the table definition.
>>
>> Here's a slightly edited version of this patch from reviewing, fixing the
>> following:
>>
>> * Fix a compiler warning by passing a pointer to skey to
>> systable_beginscan() (it's an array already)
>>
>> * Edit some comments
>>
>> The patch works as expected (at least, i don't see any remaining issues).
>> I'm going to mark this ready for committer.
>
> I don't think this is ready for committer, becauseTom previously
> objected to the approach taken by this patch here, and no one has
> answered his objections:
>
> http://archives.postgresql.org/pgsql-hackers/2010-01/msg00144.php
>
> I think someone needs to figure out what the worst-case scenario for
> this is performance-wise and submit a reproducible test case with
> benchmark results. In the meantime, I'm going to set this to Waiting
> on Author.

Basically, I don't think it is not a performance focused command,
because we may be able to assume ALTER TABLE RENAME TO is not much
frequent operations.

However, I'm interested in between two approaches.
I'll check both of them. Isn't is necessary to compare the vanilla code base,
because the matter is a bug to be fixed?

http://archives.postgresql.org/message-id/4B41BB04.2070609(a)ak.jp.nec.com
http://archives.postgresql.org/message-id/A7739F610FB0BD89E310D85E@[172.26.14.62]

Please wait for weekday, because I don't have physical (not virtual) machine
in my home.

Thanks,
--
KaiGai Kohei <kaigai(a)kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Bernd Helmle on 24 Jan 2010 07:13


--On 23. Januar 2010 22:29:23 -0500 Robert Haas <robertmhaas(a)gmail.com>
wrote:

> I don't think this is ready for committer, becauseTom previously
> objected to the approach taken by this patch here, and no one has
> answered his objections:
>
> http://archives.postgresql.org/pgsql-hackers/2010-01/msg00144.php
>

Ugh, i thought KaiGai's revised patch here

<http://archives.postgresql.org/message-id/4B41BB04.2070609(a)ak.jp.nec.com>

was in response to Tom's complaint, since it modifies the method to identify
column origins by recursivly scanning pg_inherits with its current
inhrelid.


> I think someone needs to figure out what the worst-case scenario for
> this is performance-wise and submit a reproducible test case with
> benchmark results. In the meantime, I'm going to set this to Waiting
> on Author.

Makes sense.

--
Thanks

Bernd

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Bernd Helmle on 24 Jan 2010 13:09


--On 24. Januar 2010 08:37:13 -0500 Robert Haas <robertmhaas(a)gmail.com>
wrote:

>>
>> I agree - the requirements here are much looser than for, say, SELECT
>> or UPDATE.  But it still has to not suck.
>>

Yeah, i think the meaning of "suck" can be much weakier than for a DML
command. However, if it would degrade the performance of a formerly well
running command in a way, that it would be unusable, that would be annoying.

>> I think the problem case here might be something like this...  create
>> ten tables A1 through A10.  Now create 10 more tables B1 through B10
>> each of which inherits from all of A1 through A10.  Now create 10 more
>> tables C1 through C10 that inherit from B1 through B10.  Now create
>> 1000 tables D1 through D1000 that inherit from C1 through C10.  Now
>> drop a column from A1.
>
> Er... rename a column from A1, not drop.
>

Did that with a crude pl/pgsql script, and got the following numbers:

Current -HEAD:

Phenom-II 2.6 GHz: Time: 282,471 ms
MacBook: Time: 499,866 ms

With KaiGais recent patch (which covers the TYPE case, too):

Phenom-II 2.6 GHz: Time: 476,800 ms
MacBook: Time: 753,161 ms


--
Thanks

Bernd

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: KaiGai Kohei on 27 Jan 2010 10:17
(2010/01/27 23:29), Robert Haas wrote:
> 2010/1/27 KaiGai Kohei<kaigai(a)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?

Thanks,
--
KaiGai Kohei <kaigai(a)kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3 4
Prev: [HACKERS] More frame options in window functions
Next: [HACKERS] psql tab completion for DO blocks