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
