Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two
From: Yeb Havinga on 2 Aug 2010 10:47
Robert Haas wrote:
> On Mon, Aug 2, 2010 at 9:20 AM, Yeb Havinga <yebhavinga(a)gmail.com> wrote:
>> The attached patch uses the globally defined list.
> I can't speak for any other committer, but personally I'm prepared to
> reject out of hand any solution involving a global variable. This
> code is none to easy to follow as it is and adding global variables to
> the mix is not going to make it better, even if we can convince
> ourselves that it's safe and correct. This is something of a corner
> case, so I won't be crushed if a cleaner fix is too invasive to
> back-patch.
Hello Robert,

Thanks for looking at the patch. I've attached a bit more wordy version,
that adds a boolean to AlteredTableInfo and a function to wipe that
boolean between processing of subcommands.
> Incidentally, we need to shift this discussion to a new
> subject line; we have a working patch for the original subject of this
> thread, and are now discussing the related problem I found with
> attributes.
>
Both solutions have changes in callers of 'find_inheritance_children'. I
was working in the hope a unifiying solution would pop up naturally, but
so far it has not. Checking of the new AlteredTableInfo->relVisited
boolean could perhaps be pushed down into find_inheritance_children, if
multiple-'doing things' for the childs under a multiple-inheriting
relation is unwanted for every kind of action. It seems to me that the
question whether that is the case must be answered, before the current
working patch for coninhcount is 'ready for committer'.

regards,
Yeb Havinga

From: Robert Haas on 2 Aug 2010 09:39
On Mon, Aug 2, 2010 at 9:20 AM, Yeb Havinga <yebhavinga(a)gmail.com> wrote:
> Robert Haas wrote:
>>
>> I agree that's the crux of the problem, but I can't see solving it
>> with a global variable. �I realize you were just testing...
>
> Yes it was just a test. However, somewhere information must be kept or
> altered so it can be detected that a relation has already been visited, i.e.
> it is a multiple inheriting child. The other solutions I could think of are
> more intrusive (i.e. definitionin ATController and passing as parameter).
>
> The attached patch uses the globally defined list. After ATPrepCmd the list
> pointer is reset to NIL, the list not freed since the allocs are done in a
> memory context soon to be deleted (PortalHeapMemory). It passes regression
> as well as the script below.

I can't speak for any other committer, but personally I'm prepared to
reject out of hand any solution involving a global variable. This
code is none to easy to follow as it is and adding global variables to
the mix is not going to make it better, even if we can convince
ourselves that it's safe and correct. This is something of a corner
case, so I won't be crushed if a cleaner fix is too invasive to
back-patch. Incidentally, we need to shift this discussion to a new
subject line; we have a working patch for the original subject of this
thread, and are now discussing the related problem I found with
attributes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

--
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: Robert Haas on 2 Aug 2010 12:08
On Mon, Aug 2, 2010 at 10:47 AM, Yeb Havinga <yebhavinga(a)gmail.com> wrote:
>>> The attached patch uses the globally defined list.
>>
>> I can't speak for any other committer, but personally I'm prepared to
>> reject out of hand any solution involving a global variable. �This
>> code is none to easy to follow as it is and adding global variables to
>> the mix is not going to make it better, even if we can convince
>> ourselves that it's safe and correct. �This is something of a corner
>> case, so I won't be crushed if a cleaner fix is too invasive to
>> back-patch.
>
> Thanks for looking at the patch. I've attached a bit more wordy version,
> that adds a boolean to AlteredTableInfo and a function to wipe that boolean
> between processing of subcommands.

I don't think that this is much cleaner than the global variable
solution; you haven't really localized that need to know about the new
flag in any meaningful way, the hacks in ATOneLevelRecusion()
basically destroy any pretense of that code possibly being reusable
for some other caller. However, there's a more serious problem, which
is that it doesn't in general fix the bug: try it with the
top1/top2/bottom/basement example I posted upthread. If you add the
same column to both top1 and top2 and then drop it in both top1 and
top2, basement ends up with a leftover copy. The problem is that
"only visit each child once" is not the right algorithm; what you need
to do is "only visit the descendents of each child if no merge
happened at the parent". I believe that the only way to do this
correct is to merge the prep stage into the execution stage, as the
code for adding constraints already does. At the prep stage, you
don't have enough information to determine which relations you'll
ultimately need to update, since you don't know where the merges will
happen.

>> �Incidentally, we need to shift this discussion to a new
>> subject line; we have a working patch for the original subject of this
>> thread, and are now discussing the related problem I found with
>> attributes.
>>
>
> Both solutions have changes in callers of 'find_inheritance_children'. I was
> working in the hope a unifiying solution would pop up naturally, but so far
> it has not. Checking of the new AlteredTableInfo->relVisited boolean could
> perhaps be pushed down into find_inheritance_children, if multiple-'doing
> things' for the childs under a multiple-inheriting relation is unwanted for
> every kind of action. It seems to me that the question whether that is the
> case must be answered, before the current working patch for coninhcount is
> 'ready for committer'.

I am of the opinion that the chances of a unifying solution popping up
are pretty much zero. :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

--
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: Yeb Havinga on 2 Aug 2010 14:56
Robert Haas wrote:
> I don't think that this is much cleaner than the global variable
> solution; you haven't really localized that need to know about the new
> flag in any meaningful way, the hacks in ATOneLevelRecusion()
> basically destroy any pretense of that code possibly being reusable
> for some other caller. However, there's a more serious problem, which
> is that it doesn't in general fix the bug: try it with the
> top1/top2/bottom/basement example I posted upthread. If you add the
> same column to both top1 and top2 and then drop it in both top1 and
> top2, basement ends up with a leftover copy. The problem is that
> "only visit each child once" is not the right algorithm; what you need
> to do is "only visit the descendents of each child if no merge
> happened at the parent". I believe that the only way to do this
> correct is to merge the prep stage into the execution stage, as the
> code for adding constraints already does. At the prep stage, you
> don't have enough information to determine which relations you'll
> ultimately need to update, since you don't know where the merges will
> happen.
>
Hello Robert,

Again thanks for looking at the patch. Unfortunately I missed the
top1/top2 example earlier, but now I've seen it I understand that it is
impossible to fix this problem during the prep stage, without looking at
actual existing columns, i.e. without the merge code. Running the
top1/top2 example I'd also have expected an error while adding the
column to the second top, since the columns added to top1 and top2 are
from a different origin. It is apparently considered good behaviour,
however troubles emerge when e.g. trying to rename a_table_column in the
top1/top2 example, where that is no problem in the 'lollipop' structure,
that has a single origin.

ALTER TABLE top RENAME COLUMN a_table_column TO another_table_column;

SELECT t.oid, t.relname, a.attname, a.attinhcount
FROM pg_class t
JOIN pg_attribute a ON (a.attrelid = t.oid)
JOIN pg_namespace n ON (t.relnamespace = n.oid)
WHERE n.nspname = 'test_inheritance' AND a.attname LIKE '%table_column%'
ORDER BY oid;

I do not completely understand what you mean with the destruction of
reusability of ATOneLevelRecursion, currently only called by
ATPrepAddColumn - in the patch it is documented in the definition of
relVisited that is it visit info for each subcommand. The loop over
subcommands is in ATController, where the value is properly reset for
each all current and future subcommands. Hence the ATOneLevelRecursion
routing is usable in its current form for all callers during the prep
stage, and not only ATPrepAddColumn.
> I am of the opinion that the chances of a unifying solution popping up
> are pretty much zero. :-)
>
Me too, now I understand the fixes must be in the execution stages.

regards,
Yeb Havinga


--
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: Robert Haas on 2 Aug 2010 16:21
On Mon, Aug 2, 2010 at 2:56 PM, Yeb Havinga <yebhavinga(a)gmail.com> wrote:
> I do not completely understand what you mean with the destruction of
> reusability of ATOneLevelRecursion, currently only called by ATPrepAddColumn
> - in the patch it is documented in the definition of relVisited that is it
> visit info for each subcommand. The loop over subcommands is in
> ATController, where the value is properly reset for each all current and
> future subcommands. Hence the ATOneLevelRecursion routing is usable in its
> current form for all callers during the prep stage, and not only
> ATPrepAddColumn.

Well, only if they happen to want the "visit each table only once"
behavior, which might not be true for every command type.

>> I am of the opinion that the chances of a unifying solution popping up
>> are pretty much zero. �:-)
>
> Me too, now I understand the fixes must be in the execution stages.

OK. I'll go ahead and commit the patch upthread, so that the
constraint bug is fixed, and then we can keep arguing about what do
about the column bug, perhaps on a new thread.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

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

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two