Horton

Thread

Commits

  1. Document lack of validation when attaching foreign partitions.

  2. Add tests for foreign partitions.

  1. foreign partition DDL regression tests

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-02-22T01:40:07Z

    Ashutosh Bapat pointed out [0] that regression tests are missing for the
foreign partition DDL commands.  Attached patch takes care of that.

Thanks,
Amit

[0]
https://www.postgresql.org/message-id/CAFjFpRcrdzBRj0cZ%2BJAQmfSa2Tv8wSEcWAeYtDpV-YZnNna2sA%40mail.gmail.com

  2. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-02-22T04:26:08Z

    Please add this to the upcoming commitfest.

On Wed, Feb 22, 2017 at 7:10 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Ashutosh Bapat pointed out [0] that regression tests are missing for the
> foreign partition DDL commands.  Attached patch takes care of that.
>
> Thanks,
> Amit
>
> [0]
> https://www.postgresql.org/message-id/CAFjFpRcrdzBRj0cZ%2BJAQmfSa2Tv8wSEcWAeYtDpV-YZnNna2sA%40mail.gmail.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  3. Re: foreign partition DDL regression tests

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-02-22T04:56:23Z

    On 2017/02/22 13:26, Ashutosh Bapat wrote:
> Please add this to the upcoming commitfest.

Done.

Thanks,
Amit

  4. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-03-07T12:14:35Z

    Hi Amit,
Thanks for adding testcases. Overall the testcases look good.

The testcase is using ALTER TABLE to modify foreign table schema.
Though this works, I think a better option is to use ALTER FOREIGN
TABLE.

Something not related to this patch but
-- no attach partition validation occurs for foreign tables
ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1);

I am wondering whether we can improve this. For local tables, if a
valid constraint equivalent to the partitioning condition is not
present on the table being attached, it scans the data to verify
partition conditions. But for a foreign table, we don't want to do
that since the constraint is not guaranteed to be valid after the
initial check. For a normal foreign table a user can set a constraint
if s/he knows that that constraint will be honoured on the foreign
server. Thus instead of saying that we do not validate data, we can
refuse to attach a partition if corresponding check constraint is
absent on the foreign table being attached. A user will then be forced
to add that constraint if s/he is sure that the constraint will be
obeyed on the foreign server.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  5. Re: foreign partition DDL regression tests

    Robert Haas <robertmhaas@gmail.com> — 2017-03-08T02:06:04Z

    On Tue, Mar 7, 2017 at 7:14 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Hi Amit,
> Thanks for adding testcases. Overall the testcases look good.
>
> The testcase is using ALTER TABLE to modify foreign table schema.
> Though this works, I think a better option is to use ALTER FOREIGN
> TABLE.
>
> Something not related to this patch but
> -- no attach partition validation occurs for foreign tables
> ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1);
>
> I am wondering whether we can improve this. For local tables, if a
> valid constraint equivalent to the partitioning condition is not
> present on the table being attached, it scans the data to verify
> partition conditions. But for a foreign table, we don't want to do
> that since the constraint is not guaranteed to be valid after the
> initial check. For a normal foreign table a user can set a constraint
> if s/he knows that that constraint will be honoured on the foreign
> server. Thus instead of saying that we do not validate data, we can
> refuse to attach a partition if corresponding check constraint is
> absent on the foreign table being attached. A user will then be forced
> to add that constraint if s/he is sure that the constraint will be
> obeyed on the foreign server.

I agree that we could do that, but what value would it have?  It just
forces the user to spend two SQL commands doing what could otherwise
be done in one.  And the first command might not be that obvious.  I
think we should leave well enough alone.

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

  6. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-03-08T04:18:11Z

    On Wed, Mar 8, 2017 at 7:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Mar 7, 2017 at 7:14 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> Hi Amit,
>> Thanks for adding testcases. Overall the testcases look good.
>>
>> The testcase is using ALTER TABLE to modify foreign table schema.
>> Though this works, I think a better option is to use ALTER FOREIGN
>> TABLE.
>>
>> Something not related to this patch but
>> -- no attach partition validation occurs for foreign tables
>> ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1);
>>
>> I am wondering whether we can improve this. For local tables, if a
>> valid constraint equivalent to the partitioning condition is not
>> present on the table being attached, it scans the data to verify
>> partition conditions. But for a foreign table, we don't want to do
>> that since the constraint is not guaranteed to be valid after the
>> initial check. For a normal foreign table a user can set a constraint
>> if s/he knows that that constraint will be honoured on the foreign
>> server. Thus instead of saying that we do not validate data, we can
>> refuse to attach a partition if corresponding check constraint is
>> absent on the foreign table being attached. A user will then be forced
>> to add that constraint if s/he is sure that the constraint will be
>> obeyed on the foreign server.
>
> I agree that we could do that, but what value would it have?  It just
> forces the user to spend two SQL commands doing what could otherwise
> be done in one.

I don't think it's going to be two commands always. A user who wants
to attach a foreign table as a partition, "knows" that the data on the
foreign server honours the partitioning bounds. If s/he knows that
probably he added the constraint on the foreign table, so that planner
could make use of it. Remember this is an existing foreign table. If
s/he is not aware that the data on the foreign server doesn't honour
partition bounds, adding that as a partition would be a problem. I
think, this step gives the user a chance to make a conscious decision.

> And the first command might not be that obvious.

A hint with error reported would help. In fact, the hint might as well
say "add constraint if you are sure that the foreign data honours
partition bound specification" or something like that. I noticed that
the documentation at
https://www.postgresql.org/docs/devel/static/sql-altertable.html for
ATTACH PARTITION does not have anything about foreign tables. May be
we should add whatever is the current status.

>  I
> think we should leave well enough alone.

At least we need to update the documentation.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  7. Re: foreign partition DDL regression tests

    Robert Haas <robertmhaas@gmail.com> — 2017-03-08T15:04:26Z

    On Tue, Mar 7, 2017 at 11:18 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>> I agree that we could do that, but what value would it have?  It just
>> forces the user to spend two SQL commands doing what could otherwise
>> be done in one.
>
> I don't think it's going to be two commands always. A user who wants
> to attach a foreign table as a partition, "knows" that the data on the
> foreign server honours the partitioning bounds. If s/he knows that
> probably he added the constraint on the foreign table, so that planner
> could make use of it. Remember this is an existing foreign table. If
> s/he is not aware that the data on the foreign server doesn't honour
> partition bounds, adding that as a partition would be a problem. I
> think, this step gives the user a chance to make a conscious decision.

I think attaching the foreign table as a partition constitutes a
sufficiently-conscious decision.

> At least we need to update the documentation.

Got a proposal?

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

  8. Re: foreign partition DDL regression tests

    Robert Haas <robertmhaas@gmail.com> — 2017-03-08T16:28:53Z

    On Tue, Feb 21, 2017 at 8:40 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Ashutosh Bapat pointed out [0] that regression tests are missing for the
> foreign partition DDL commands.  Attached patch takes care of that.

Committed.

I didn't do anything about Ashutosh's comment that we could use ALTER
FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem
critical.

Also, the names of the objects in this test are kinda generic (pt2 et.
al.) but they match the existing names in the same file (pt1, foo).
If we're going to start differentiating those a little better, we
should probably change them all, and as a separate commit.

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

  9. Re: foreign partition DDL regression tests

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-09T00:52:53Z

    On 2017/03/09 1:28, Robert Haas wrote:
> On Tue, Feb 21, 2017 at 8:40 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Ashutosh Bapat pointed out [0] that regression tests are missing for the
>> foreign partition DDL commands.  Attached patch takes care of that.
> 
> Committed.

Thanks.

> I didn't do anything about Ashutosh's comment that we could use ALTER
> FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem
> critical.

Attached is a patch to fix that, just in case.

> Also, the names of the objects in this test are kinda generic (pt2 et.
> al.) but they match the existing names in the same file (pt1, foo).
> If we're going to start differentiating those a little better, we
> should probably change them all, and as a separate commit.

Agreed.

Thanks,
Amit

  10. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-03-09T06:19:21Z

    >
>> At least we need to update the documentation.
>
> Got a proposal?

How about something like attached?


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  11. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-03-09T06:23:17Z

    >
>> I didn't do anything about Ashutosh's comment that we could use ALTER
>> FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem
>> critical.
>
> Attached is a patch to fix that, just in case.

Thanks. Looks good to me.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  12. Re: foreign partition DDL regression tests

    Robert Haas <robertmhaas@gmail.com> — 2017-03-09T18:14:20Z

    On Thu, Mar 9, 2017 at 1:19 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>>> At least we need to update the documentation.
>>
>> Got a proposal?
>
> How about something like attached?

Committed with some revisions.

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

  13. Re: foreign partition DDL regression tests

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-03-10T04:04:21Z

    On Thu, Mar 9, 2017 at 11:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 9, 2017 at 1:19 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>>> At least we need to update the documentation.
>>>
>>> Got a proposal?
>>
>> How about something like attached?
>
> Committed with some revisions.

Thanks.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company