Обсуждение: postgres_fdw: IMPORT FOREIGN SCHEMA ... LIMIT TO (partition)

Hi,

Recently I was trying to copy some of the data of one database to
another through postgres_fdw, and found that it wouldn't import that
partition through IMPORT FOREIGN SCHEMA, even when I explicitly
specified the name of the table that contained the data in the LIMIT
TO clause.

I realised the reason is that currently, postgres_fdw explicitly
disallows importing foreign partitions. This is a reasonable default
when importing a whole schema, but if I wanted to explicitly import
one of a partitioned tables' partitions, that would now require me to
manually copy the foreign table's definitions through the use of
CREATE FOREIGN TABLE, which is a hassle and prone to mistakes.

As such, I propose the attached patch, in which the 'no
partitions'-restriction of postgres_fdw is lifted for the LIMIT TO
clause. This has several benefits, including not holding locks on the
foreign root partition during queries, and less suprising behaviour
for LIMIT TO ("table that happens to be a partition").

Regards,

Matthias van de Meent

Am Donnerstag, dem 21.01.2021 um 15:56 +0100 schrieb Matthias van de
Meent:
> Hi,
> 
> Recently I was trying to copy some of the data of one database to
> another through postgres_fdw, and found that it wouldn't import that
> partition through IMPORT FOREIGN SCHEMA, even when I explicitly
> specified the name of the table that contained the data in the LIMIT
> TO clause.
> 
> I realised the reason is that currently, postgres_fdw explicitly
> disallows importing foreign partitions. This is a reasonable default
> when importing a whole schema, but if I wanted to explicitly import
> one of a partitioned tables' partitions, that would now require me to
> manually copy the foreign table's definitions through the use of
> CREATE FOREIGN TABLE, which is a hassle and prone to mistakes.
> 

Hi,

I took a look at this patch.

Patch applies on current master.

Documentation and adjusted regression tests included.
Regression tests passes without errors.

The patch changes IMPORT FOREIGN SCHEMA to explicitely allow partition
child tables in the LIMIT TO clause of the IMPORT FOREIGN SCHEMA
command by relaxing the checks introduced with commit [1]. The reason
behind [1] are discussed in [2].

So the original behavior this patch wants to address was done
intentionally, so what needs to be discussed here is whether we want to
relax that a little. One argument for the original behavior since then
was that it is cleaner to just automatically import the parent, which
allows access to the childs through the foreign table anways and
exclude partition childs when querying pg_class.

I haven't seen demand for the implemented feature here myself, but i
could imagine use cases where just a single child or a set of child
tables are candidates. For example, i think it's possible that users
can query only specific childs and want them to have imported on
another foreign server.


[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f49bcd4ef3e9a75de210357a4d9bbe3e004db956

[2]
https://www.postgresql.org/message-id/20170309141531.GD9812@tamriel.snowman.net


-- 
Thanks,
    Bernd

On Mon, 22 Mar 2021 at 21:16, Bernd Helmle <mailings@oopsware.de> wrote:
>
> Hi,
>
> I took a look at this patch.

Thanks!

> Patch applies on current master.
>
> Documentation and adjusted regression tests included.
> Regression tests passes without errors.
>
> The patch changes IMPORT FOREIGN SCHEMA to explicitely allow partition
> child tables in the LIMIT TO clause of the IMPORT FOREIGN SCHEMA
> command by relaxing the checks introduced with commit [1]. The reason
> behind [1] are discussed in [2].

I should've included potentially interested parties earlier, but never
too late. Stephen, Michael, Amit, would you have an opinion on lifting
this restriction for the LIMIT TO clause, seeing your involvement in
the implementation of removing partitions from IFS?

> So the original behavior this patch wants to address was done
> intentionally, so what needs to be discussed here is whether we want to
> relax that a little. One argument for the original behavior since then
> was that it is cleaner to just automatically import the parent, which
> allows access to the childs through the foreign table anways and
> exclude partition childs when querying pg_class.

Yes, but it should be noted that the main reason that was mentioned as
a reason to exclude partitions is to not cause table catalog bloat,
and I argue that this argument is not as solid in the case of the
explicitly named tables of the LIMIT TO clause. Except if SQL standard
prescribes otherwise, I think allowing partitions in LIMIT TO clauses
is an improvement overall.

> I haven't seen demand for the implemented feature here myself, but i
> could imagine use cases where just a single child or a set of child
> tables are candidates. For example, i think it's possible that users
> can query only specific childs and want them to have imported on
> another foreign server.

I myself have had this need, in that I've had to import some
partitions manually as a result of this limitation. IMPORT FORAIGN
SCHEMA really is great when it works, but limitations like these are
crippling for some more specific use cases (e.g. allowing
long-duration read-only access to one partition in the partition tree
while also allowing the partition layout of the parents to be
modified).


With regards,

Matthias.

Am Mittwoch, dem 24.03.2021 um 13:23 +0100 schrieb Matthias van de
Meent:
> Yes, but it should be noted that the main reason that was mentioned
> as
> a reason to exclude partitions is to not cause table catalog bloat,
> and I argue that this argument is not as solid in the case of the
> explicitly named tables of the LIMIT TO clause. Except if SQL
> standard
> prescribes otherwise, I think allowing partitions in LIMIT TO clauses
> is an improvement overall.

Don't get me wrong, i find this useful, too. Especially because it's a
very minor change in the code. And i don't see negative aspects here
currently, either (which doesn't mean there aren't some).

> 
> I myself have had this need, in that I've had to import some
> partitions manually as a result of this limitation. IMPORT FORAIGN
> SCHEMA really is great when it works, but limitations like these are
> crippling for some more specific use cases (e.g. allowing
> long-duration read-only access to one partition in the partition tree
> while also allowing the partition layout of the parents to be
> modified).

Interesting use case. 


-- 
Thanks,
    Bernd

Am Mittwoch, dem 24.03.2021 um 17:32 +0100 schrieb Bernd Helmle:
> > Yes, but it should be noted that the main reason that was mentioned
> > as
> > a reason to exclude partitions is to not cause table catalog bloat,
> > and I argue that this argument is not as solid in the case of the
> > explicitly named tables of the LIMIT TO clause. Except if SQL
> > standard
> > prescribes otherwise, I think allowing partitions in LIMIT TO
> > clauses
> > is an improvement overall.
> 
> Don't get me wrong, i find this useful, too. Especially because it's
> a
> very minor change in the code. And i don't see negative aspects here
> currently, either (which doesn't mean there aren't some).

Since there are currently no obvious objections i've marked this "Read
for Committer".


-- 
Thanks,
    Bernd

On 2021/03/29 2:39, Bernd Helmle wrote:
> Am Mittwoch, dem 24.03.2021 um 17:32 +0100 schrieb Bernd Helmle:
>>> Yes, but it should be noted that the main reason that was mentioned
>>> as
>>> a reason to exclude partitions is to not cause table catalog bloat,
>>> and I argue that this argument is not as solid in the case of the
>>> explicitly named tables of the LIMIT TO clause. Except if SQL
>>> standard
>>> prescribes otherwise, I think allowing partitions in LIMIT TO
>>> clauses
>>> is an improvement overall.
>>
>> Don't get me wrong, i find this useful, too. Especially because it's
>> a
>> very minor change in the code. And i don't see negative aspects here
>> currently, either (which doesn't mean there aren't some).
> 
> Since there are currently no obvious objections i've marked this "Read
> for Committer".

For now I have no objection to this feature.

-IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch)
+IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch, t4_part)

Isn't it better to create also another partition like "t4_part2"?
If we do this, for example, the above test can confirm that both
partitions in EXCEPT and not in are excluded.

+    All tables or foreign tables which are partitions of some other table
+    are automatically excluded from <xref linkend="sql-importforeignschema"/>
+    unless they are explicitly included in the <literal>LIMIT TO</literal>

IMO it's better to document that partitions are imported when they are
included in LIMIT TO, instead. What about the following?

     Tables or foreign tables which are partitions of some other table are
     imported only when they are explicitly specified in
     <literal>LIMIT TO</literal> clause.  Otherwise they are automatically
     excluded from <xref linkend="sql-importforeignschema"/>.

+    clause.  Since all data can be accessed through the partitioned table
+    which is the root of the partitioning hierarchy, this approach should
+    allow access to all the data without creating extra objects.

Now "this approach" in the above is not clear? What about replacing it with
something like "importing only partitioned tables"?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi Matthias,

On Wed, Mar 24, 2021 at 9:23 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> On Mon, 22 Mar 2021 at 21:16, Bernd Helmle <mailings@oopsware.de> wrote:
> > The patch changes IMPORT FOREIGN SCHEMA to explicitely allow partition
> > child tables in the LIMIT TO clause of the IMPORT FOREIGN SCHEMA
> > command by relaxing the checks introduced with commit [1]. The reason
> > behind [1] are discussed in [2].
>
> I should've included potentially interested parties earlier, but never
> too late. Stephen, Michael, Amit, would you have an opinion on lifting
> this restriction for the LIMIT TO clause, seeing your involvement in
> the implementation of removing partitions from IFS?

Sorry that I'm replying to this a bit late.

> > So the original behavior this patch wants to address was done
> > intentionally, so what needs to be discussed here is whether we want to
> > relax that a little. One argument for the original behavior since then
> > was that it is cleaner to just automatically import the parent, which
> > allows access to the childs through the foreign table anways and
> > exclude partition childs when querying pg_class.
>
> Yes, but it should be noted that the main reason that was mentioned as
> a reason to exclude partitions is to not cause table catalog bloat,
> and I argue that this argument is not as solid in the case of the
> explicitly named tables of the LIMIT TO clause. Except if SQL standard
> prescribes otherwise, I think allowing partitions in LIMIT TO clauses
> is an improvement overall.
>
> > I haven't seen demand for the implemented feature here myself, but i
> > could imagine use cases where just a single child or a set of child
> > tables are candidates. For example, i think it's possible that users
> > can query only specific childs and want them to have imported on
> > another foreign server.
>
> I myself have had this need, in that I've had to import some
> partitions manually as a result of this limitation. IMPORT FORAIGN
> SCHEMA really is great when it works, but limitations like these are
> crippling for some more specific use cases (e.g. allowing
> long-duration read-only access to one partition in the partition tree
> while also allowing the partition layout of the parents to be
> modified).

FWIW, I agree that it would be nice to have this.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

On Tue, Apr 6, 2021 at 8:34 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> For now I have no objection to this feature.
>
> -IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch)
> +IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch, t4_part)
>
> Isn't it better to create also another partition like "t4_part2"?
> If we do this, for example, the above test can confirm that both
> partitions in EXCEPT and not in are excluded.
>
> +    All tables or foreign tables which are partitions of some other table
> +    are automatically excluded from <xref linkend="sql-importforeignschema"/>
> +    unless they are explicitly included in the <literal>LIMIT TO</literal>
>
> IMO it's better to document that partitions are imported when they are
> included in LIMIT TO, instead. What about the following?
>
>      Tables or foreign tables which are partitions of some other table are
>      imported only when they are explicitly specified in
>      <literal>LIMIT TO</literal> clause.  Otherwise they are automatically
>      excluded from <xref linkend="sql-importforeignschema"/>.
>
> +    clause.  Since all data can be accessed through the partitioned table
> +    which is the root of the partitioning hierarchy, this approach should
> +    allow access to all the data without creating extra objects.
>
> Now "this approach" in the above is not clear? What about replacing it with
> something like "importing only partitioned tables"?

+1, that wording is better.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

On 2021/04/06 16:05, Amit Langote wrote:
> On Tue, Apr 6, 2021 at 8:34 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> For now I have no objection to this feature.
>>
>> -IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch)
>> +IMPORT FOREIGN SCHEMA import_source EXCEPT (t1, "x 4", nonesuch, t4_part)
>>
>> Isn't it better to create also another partition like "t4_part2"?
>> If we do this, for example, the above test can confirm that both
>> partitions in EXCEPT and not in are excluded.
>>
>> +    All tables or foreign tables which are partitions of some other table
>> +    are automatically excluded from <xref linkend="sql-importforeignschema"/>
>> +    unless they are explicitly included in the <literal>LIMIT TO</literal>
>>
>> IMO it's better to document that partitions are imported when they are
>> included in LIMIT TO, instead. What about the following?
>>
>>       Tables or foreign tables which are partitions of some other table are
>>       imported only when they are explicitly specified in
>>       <literal>LIMIT TO</literal> clause.  Otherwise they are automatically
>>       excluded from <xref linkend="sql-importforeignschema"/>.
>>
>> +    clause.  Since all data can be accessed through the partitioned table
>> +    which is the root of the partitioning hierarchy, this approach should
>> +    allow access to all the data without creating extra objects.
>>
>> Now "this approach" in the above is not clear? What about replacing it with
>> something like "importing only partitioned tables"?
> 
> +1, that wording is better.

Thanks! So I applied all the changes that I suggested upthread to the patch.
I also updated the comment as follows.

           * Import table data for partitions only when they are explicitly
-         * specified in LIMIT TO clause. Otherwise ignore them and
-         * only include the definitions of the root partitioned tables to
-         * allow access to the complete remote data set locally in
-         * the schema imported.
+         * specified in LIMIT TO clause. Otherwise ignore them and only
+         * include the definitions of the root partitioned tables to allow
+         * access to the complete remote data set locally in the schema
+         * imported.

Attached is the updated version of the patch. Barring any objection,
I'm thinking to commit this.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, 6 Apr 2021 at 14:29, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Thanks! So I applied all the changes that I suggested upthread to the patch.
> I also updated the comment as follows.
>
>                  * Import table data for partitions only when they are explicitly
> -                * specified in LIMIT TO clause. Otherwise ignore them and
> -                * only include the definitions of the root partitioned tables to
> -                * allow access to the complete remote data set locally in
> -                * the schema imported.
> +                * specified in LIMIT TO clause. Otherwise ignore them and only
> +                * include the definitions of the root partitioned tables to allow
> +                * access to the complete remote data set locally in the schema
> +                * imported.
>
> Attached is the updated version of the patch. Barring any objection,
> I'm thinking to commit this.

Thanks, this was on my to-do list for today, but you were faster.

No objections on my part, and thanks for picking this up.

With regards,

Matthias van de Meent

On Tue, Apr 06, 2021 at 02:39:54PM +0200, Matthias van de Meent wrote:
> On Tue, 6 Apr 2021 at 14:29, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> Attached is the updated version of the patch. Barring any objection,
>> I'm thinking to commit this.

Sorry for the late reply.  The approach to use LIMIT TO for this
purpose looks sensible from here, and I agree that it can have its
uses.  So what you have here LGTM.
--
Michael

On 2021/04/06 22:02, Michael Paquier wrote:
> On Tue, Apr 06, 2021 at 02:39:54PM +0200, Matthias van de Meent wrote:
>> On Tue, 6 Apr 2021 at 14:29, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>> Attached is the updated version of the patch. Barring any objection,
>>> I'm thinking to commit this.
> 
> Sorry for the late reply.  The approach to use LIMIT TO for this
> purpose looks sensible from here, and I agree that it can have its
> uses.  So what you have here LGTM.

Pushed. Thanks all!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION