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.rupireddyforpostg...@gmail.com>: > > On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi <oni...@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
pgsql14-truncate-on-foreign-table.v14.patch
Description: Binary data
