Re: TRUNCATE on foreign table

Thank you for checking v11!
I've updated it and attached v12.

> 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

thanks, I've removed these whitespaces and confirmed no warnings
occurred when I run "git apply <<patch_name>>.patch"

> 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.

Sure. I've added head_destroy().

> 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.

Sure. I've replaced with the test command "SELECT * FROM ..." to
"SELECT COUNT(*) FROM ..."
However, for example, the "id" column is used to check after running
TRUNCATE with ONLY clause to the inherited table.
Thus, I use "sum(id)" instead of "count(*)" to check the result when
the table has records.

2021年4月5日(月) 0:20 Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>:
>
> On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi <onishi(at)heterodb(dot)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