On 2017/02/21 20:17, Ashutosh Bapat wrote:
> On Tue, Feb 21, 2017 at 12:05 PM, Amit Langote wrote:
>> On 2017/02/20 21:49, Ashutosh Bapat wrote:
>>> Here are some comments
>>>
>>> For the sake of readability you may want reverse the if and else order.
>>> - recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>>> + if (!child_is_partition)
>>> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>>> + else
>>> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
>>> like
>>> + if (child_is_partition)
>>> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
>>> + else
>>> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
>>
>> Sure, done.
>
> I still see
> - recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
> + if (!child_is_partition)
> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
> + else
> + recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
>
> Are you sure you have attached the right patch?
Oops, really fixed this time.
>>> --- cleanup: avoid using CASCADE
>>> -DROP TABLE list_parted, part_1;
>>> -DROP TABLE list_parted2, part_2, part_5, part_5_a;
>>> -DROP TABLE range_parted, part1, part2;
>>> +-- cleanup
>>> +DROP TABLE list_parted, list_parted2, range_parted;
>>> Testcases usually drop one table at a time, I guess, to reduce the differences
>>> when we add or remove tables from testcases. All such blocks should probably
>>> follow same policy.
>>
>> Hmm, I see this in src/test/regress/sql/inherit.sql:141
>>
>> DROP TABLE firstparent, secondparent, jointchild, thirdparent, otherchild;
>
> Hmm, I can spot some more such usages. Let's keep this for the
> committer to decide.
Sure.
>>> BTW, I noticed that although we are allowing foreign tables to be
>>> partitions, there are no tests in foreign_data.sql for testing it. If
>>> there would have been we would tests DROP TABLE on a partitioned table
>>> with foreign partitions as well. That file has testcases for testing
>>> foreign table inheritance, and should have tests for foreign table
>>> partitions.
>>
>> That makes sense. Patch 0002 is for that (I'm afraid this should be
>> posted separately though). I didn't add/repeat all the tests that were
>> added by the foreign table inheritance patch again for foreign partitions
>> (common inheritance rules apply to both cases), only added those for the
>> new partitioning commands and certain new rules.
>
> Thanks. Yes, a separate thread would do. I will review it there. May
> be you want to add it to the commitfest too.
Posted in a new thread titled "foreign partition DDL regression tests".
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers