Re: TRUNCATE on foreign table

Thank you for checking v13, and here is v14 patch.

> 1) Are we using all of these macros? I see that we are setting them
> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> them?

These may be needed for the foreign data handler other than postgres_fdw.

> 2) Why is this change for? The existing comment properly says the
> behaviour i.e. all foreign tables are updatable by default.

This is just a mistake. I've fixed it.

> 3) In the docs, let's not combine updatable and truncatable together.
> Have a separate section for <title>Truncatability Options</title>, all
> the documentation related to it be under this new section.

Sure. I've added new section.

> 4) I have a basic question: If I have a truncate statement with a mix
> of local and foreign tables, IIUC, the patch is dividing up a single
> truncate statement into two truncate local tables, truncate foreign
> tables. Is this transaction safe at all?

According to this discussion, we can revert both tables in the local
and the server.
https://www.postgresql.org/message-id/CAOP8fzbuJ5GdKa%2B%3DGtizbqFtO2xsQbn4mVjjzunmsNVJMChSMQ%40mail.gmail.com

> 6) Again v13 has white space errors, please ensure to run git diff
> --check on every patch.

Umm..  I'm sure I've checked it on v13.
I've confirmed it on v14.

2021年4月6日(火) 13:33 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> >
> > > Did you check that hash_destroy is not reachable when an error occurs
> > > on the remote server while executing truncate command?
> >
> > I've checked it and hash_destroy doesn't work on error.
> >
> > I just adding elog() to check this:
> > + elog(NOTICE,"destroyed");
> > + hash_destroy(ft_htab);
> >
> > Then I've checked by the test.
> >
> > + -- 'truncatable' option
> > + ALTER SERVER loopback OPTIONS (ADD truncatable 'false');
> > + TRUNCATE tru_ftable; -- error
> > + ERROR:  truncate on "tru_ftable" is prohibited
> > <-   hash_destroy doesn't work.
> > + ALTER FOREIGN TABLE tru_ftable OPTIONS (ADD truncatable 'true');
> > + TRUNCATE tru_ftable; -- accepted
> > + NOTICE:  destroyed     <-  hash_destroy works.
> >
> > Of course, the elog() is not included in v13 patch.
>
> Few more comments on v13:
>
> 1) Are we using all of these macros? I see that we are setting them
> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> them?
> +#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
> +#define TRUNCATE_REL_CONTEXT_ONLY         0x02
> +#define TRUNCATE_REL_CONTEXT_CASCADING     0x04
>
> 2) Why is this change for? The existing comment properly says the
> behaviour i.e. all foreign tables are updatable by default.
> @@ -2216,7 +2223,7 @@ postgresIsForeignRelUpdatable(Relation rel)
>      ListCell   *lc;
>
>      /*
> -     * By default, all postgres_fdw foreign tables are assumed updatable. This
> +     * By default, all postgres_fdw foreign tables are assumed NOT
> truncatable. This
>
> And the below comment is wrong, by default foreign tables are assumed
> truncatable.
> +     * By default, all postgres_fdw foreign tables are NOT assumed
> truncatable. This
> +     * can be overridden by a per-server setting, which in turn can be
> +     * overridden by a per-table setting.
> +     */
>
> 3) In the docs, let's not combine updatable and truncatable together.
> Have a separate section for <title>Truncatability Options</title>, all
> the documentation related to it be under this new section.
>     <para>
>      By default all foreign tables using
> <filename>postgres_fdw</filename> are assumed
> -    to be updatable.  This may be overridden using the following option:
> +    to be updatable and truncatable.  This may be overridden using
> the following options:
>     </para>
>
> 4) I have a basic question: If I have a truncate statement with a mix
> of local and foreign tables, IIUC, the patch is dividing up a single
> truncate statement into two truncate local tables, truncate foreign
> tables. Is this transaction safe at all?
> A better illustration: TRUNCATE local_rel1, local_rel2, local_rel3,
> foreign_rel1, foreign_rel2, foreign_rel3;
> Your patch executes TRUNCATE local_rel1, local_rel2, local_rel3; on
> local server and TRUNCATE foreign_rel1, foreign_rel2, foreign_rel3; on
> remote server. Am I right?
> Now the question is: if any failure occurs either in local server
> execution or in remote server execution, the other truncate command
> would succeed right? Isn't this non-transactional and we are breaking
> the transactional guarantee of the truncation.
> Looks like the order of execution is - first local rel truncation and
> then foreign rel truncation, so what happens if foreign rel truncation
> fails? Can we revert the local rel truncation?
>
> 6) Again v13 has white space errors, please ensure to run git diff
> --check on every patch.
> bharath@ubuntu:~/workspace/postgres$ git apply
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:41:
> trailing whitespace.
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:47:
> trailing whitespace.
>
> warning: 2 lines add whitespace errors.
> bharath@ubuntu:~/workspace/postgres$ git diff --check
> contrib/postgres_fdw/deparse.c:2200: trailing whitespace.
> +
> contrib/postgres_fdw/deparse.c:2206: trailing whitespace.
> +
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com