Re: TRUNCATE on foreign table - Mailing list pgsql-hackers

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.rupireddyforpostgres@gmail.com>:
>
> 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