Re: TRUNCATE on foreign table - Mailing list pgsql-hackers

Fujii-san,

Thank you for your review!
Now I prepare v5 patch and I'll answer to your each comment. please
check this again.
m(_ _)m

1. In postgres-fdw.sgml, "and truncatable" should be appended into the
above first description?
2. truncate.sgml should be updated because, for example, it contains
the above descriptions.

Yeah, you're right. I've fixed it.



3.  Don't we need to document the detail information about frels_extra?

I've written about frels_extra into fdwhander.sgml.



4. postgres_fdw determines whether to specify ONLY or not by checking
whether the passed extra value is zero or not.

Please refer this:
https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
> Negative value means that foreign-tables are not specified in the TRUNCATE
> command, but truncated due to dependency (like partition's child leaf).

I've added this information into fdwhandler.sgml.



5. For example, we can easily do that by truncate foreign tables
before local ones. Thought?

Umm... yeah, I feel it's better procedure, but not so required because
TRUNCATE is NOT called frequently.
Certainly, we already have postgresIsForeignUpdatable() to check
whether the foreign table is updatable or not.
Following this way, we have to add postgresIsForeignTruncatable() to check.
However, Unlike UPDATE, TRUNCATE is NOT called frequently. Current
procedure is inefficient but works correctly.
Thus, I feel postgresIsForeignTruncatable() is not needed.


6. XLOG_HEAP_TRUNCATE record is written even for the truncation of a
foreign table. Why is this necessary?

Please give us more time to investigate this.

2021年3月25日(木) 3:47 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
>
>
> On 2021/03/13 18:57, Kazutaka Onishi wrote:
> > I have fixed the patch to pass check-world test. :D
>
> Thanks for updating the patch! Here are some review comments from me.
>
>
>        By default all foreign tables using <filename>postgres_fdw</filename> are assumed
>        to be updatable.  This may be overridden using the following option:
>
> In postgres-fdw.sgml, "and truncatable" should be appended into
> the above first description? Also "option" in the second description
> should be a plural form "options"?
>
>
>       <command>TRUNCATE</command> is not currently supported for foreign tables.
>       This implies that if a specified table has any descendant tables that are
>       foreign, the command will fail.
>
> truncate.sgml should be updated because, for example, it contains
> the above descriptions.
>
>
> +     <literal>frels_extra</literal> is same length with
> +     <literal>frels_list</literal>, that delivers extra information of
> +     the context where the foreign-tables are truncated.
> +    </para>
>
> Don't we need to document the detail information about frels_extra?
> Otherwise the developers of FDW would fail to understand how to
> handle the frels_extra when trying to make their FDWs support TRUNCATE.
>
>
> +               relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
> +                               relids_extra = lappend_int(relids_extra, -1);
>
> postgres_fdw determines whether to specify ONLY or not by checking
> whether the passed extra value is zero or not. That is, for example,
> using only 0 and 1 for extra values is enough for the purpose. But
> ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
> these three values necessary?
>
>
> With the patch, if both local and foreign tables are specified as
> the target tables to truncate, TRUNCATE command tries to truncate
> foreign tables after truncating local ones. That is, if "truncatable"
> option is set to false or enough permission to truncate is not granted
> yet in the foreign server, an error will be thrown after the local tables
> are truncated. I don't think this is good order of processings. IMO,
> instead, we should check whether foreign tables can be truncated
> before any actual truncation operations. For example, we can easily
> do that by truncate foreign tables before local ones. Thought?
>
>
> XLOG_HEAP_TRUNCATE record is written even for the truncation of
> a foreign table. Why is this necessary?
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION