Re: TRUNCATE on foreign table

On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > 5) Can't we use do_sql_command function after making it non static? We
> > could go extra mile, that is we could make do_sql_command little more
> > generic by passing some enum for each of PQsendQuery,
> > PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
> > the respective code chunks with do_sql_command in postgres_fdw.c.
>
> I've skipped this for now.
> I feel it sounds cool, but not easy.
> It should be added by another patch because it's not only related to TRUNCATE.

Fair enough! I will give it a thought and provide a patch separately.

> > 6) A white space error when the patch is applied.
> > contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.
>
> I've checked the patch and clean spaces.
> But I can't confirmed this message by attaching(patch -p1 < ...) my v8 patch.
> If this still occurs, please tell me how you attach the patch.

I usually follow these steps:
1) write code 2) git diff --check (will give if there are any white
space or indentation errors) 3) git add -u 4) git commit (will enter a
commit message) 5) git format-patch -1 <<sha of the commit>> -v
<<version number>> 6) to apply patch, git apply <<patch_name>>.patch

> > 7) I may be missing something here. Why do we need a hash table at
> > all? We could just do it with a linked list right? Is there a specific
> > reason to use a hash table? IIUC, the hash table entries will be lying
> > around until the local session exists since we are not doing
> > hash_destroy.
>
> I've skipped this for now.

If you don't destroy the hash, you are going to cause a memory leak.
Because, the pointer to hash tableft_htab is local to
ExecuteTruncateGuts (note that it's not a static variable) and you are
creating a memory for the hash table and leaving the function without
cleaning it up. IMHO, we should destroy the hash memory at the end of
ExecuteTruncateGuts.

Another comment for tests, why do we need to do full outer join of two
tables to just show up there are some rows in the table? I would
suggest that all the tests introduced in the patch can be something
like this: 1) before truncate, just show the count(*) from the table
2) truncate the foreign tables 3) after truncate, just show the
count(*) which should be 0. Because we don't care what the data is in
the tables, we only care about whether truncate is happened or not.

+SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
b.id ORDER BY a.id;
+ id |                x                 | id |                z
+----+----------------------------------+----+----------------------------------
+  1 | eccbc87e4b5ce2fe28308fd9f2a7baf3 |    |
+  2 | a87ff679a2f3e71d9181a67b7542122c |    |
+  3 | e4da3b7fbbce2345d7772b0674a318d5 |  3 | 1679091c5a880faf6fb5e6087eb1b2dc
+  4 | 1679091c5a880faf6fb5e6087eb1b2dc |  4 | 8f14e45fceea167a5a36dedd4bea2543
+  5 | 8f14e45fceea167a5a36dedd4bea2543 |  5 | c9f0f895fb98ab9159f51fd0297e236d
+  6 | c9f0f895fb98ab9159f51fd0297e236d |  6 | 45c48cce2e2d7fbdea1afc51c7c6ad26
+  7 | 45c48cce2e2d7fbdea1afc51c7c6ad26 |  7 | d3d9446802a44259755d38e6d163e820
+  8 | d3d9446802a44259755d38e6d163e820 |  8 | 6512bd43d9caa6e02c990b0a82652dca
+    |                                  |  9 | c20ad4d76fe97759aa27a0c99bff6710
+    |                                  | 10 | c51ce410c124a10e0db5e4b97fc2af39
+(10 rows)
+
+TRUNCATE tru_ftable, tru_pk_ftable CASCADE;
+SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
b.id ORDER BY a.id;  -- empty


With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com