Re: TRUNCATE on foreign table

On Sun, Apr 4, 2021 at 10:23 PM Kazutaka Onishi <onishi(at)heterodb(dot)com> wrote:
> 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.

I still don't understand why we need sum(id), not count(*). Am I
missing something here?

Here are some more comments on the v12 patch:
1) Instead of switch case, a simple if else clause would reduce the code a bit:
if (behavior == DROP_RESTRICT)
appendStringInfoString(buf, " RESTRICT");
else if (behavior == DROP_CASCADE)
appendStringInfoString(buf, " CASCADE");

2) Some coding style comments:
It's better to have a new line after variable declarations,
assignments, function calls, before if blocks, after if blocks for
better readability of the code.
+ appendStringInfoString(buf, "TRUNCATE "); ---> new line after this
+ forboth (lc1, frels_list,

+ } ---> new line after this
+ appendStringInfo(buf, " %s IDENTITY",

+ /* ensure the target foreign table is truncatable */
+ truncatable = server_truncatable; ---> new line after this
+ foreach (cell, ft->options)

+ } ---> new line after this
+ if (!truncatable)

+ } ---> new line after this
+ /* set up remote query */
+ initStringInfo(&sql);
+ deparseTruncateSql(&sql, frels_list, frels_extra, behavior,
restart_seqs); ---> new line after this
+ /* run remote query */
+ if (!PQsendQuery(conn, sql.data))
+ pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
---> new line after this
+ res = pgfdw_get_result(conn, sql.data); ---> new line after this
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ pgfdw_report_error(ERROR, res, conn, true, sql.data); --->
new line after this
+ /* clean-up */
+ PQclear(res);
+ pfree(sql.data);
+}

and so on.

a space after "false," and before "NULL"
+ conn = GetConnection(user, false,NULL);

bring lc2, frels_extra to the same of lc1, frels_list
+ forboth (lc1, frels_list,
+ lc2, frels_extra)

3) I think we need truncatable behaviour that is consistent with updatable.
With your patch, seems like below is the behaviour for truncatable:
both server and foreign table are truncatable = truncated
server is not truncatable and foreign table is truncatable = not
truncated and error out
server is truncatable and foreign table is not truncatable = not
truncated and error out
server is not truncatable and foreign table is not truncatable = not
truncated and error out

Below is the behaviour for updatable:
both server and foreign table are updatable = updated
server is not updatable and foreign table is updatable = updated
server is updatable and foreign table is not updatable = not updated
server is not updatable and foreign table is not updatable = not updated

And also see comment in postgresIsForeignRelUpdatable
/*
* By default, all postgres_fdw foreign tables are assumed updatable. This
* can be overridden by a per-server setting, which in turn can be
* overridden by a per-table setting.
*/

IMO, you could do the same thing for truncatable, change is required
in your patch:
both server and foreign table are truncatable = truncated
server is not truncatable and foreign table is truncatable = truncated
server is truncatable and foreign table is not truncatable = not
truncated and error out
server is not truncatable and foreign table is not truncatable = not
truncated and error out

4) GetConnection needs to be done after all the error checking code
otherwise on error we would have opened a new connection without
actually using it. Just move below code outside of the for loop in
postgresExecForeignTruncate
+ user = GetUserMapping(GetUserId(), server_id);
+ conn = GetConnection(user, false,NULL);
to here:
+ Assert (OidIsValid(server_id)));
+
+ /* get a connection to the server */
+ user = GetUserMapping(GetUserId(), server_id);
+ conn = GetConnection(user, false, NULL);
+
+ /* set up remote query */
+ initStringInfo(&sql);
+ deparseTruncateSql(&sql, frels_list, frels_extra, behavior, restart_seqs);
+ /* run remote query */
+ if (!PQsendQuery(conn, sql.data))
+ pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
+ res = pgfdw_get_result(conn, sql.data);
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ pgfdw_report_error(ERROR, res, conn, true, sql.data);

5) This assertion is bogus, because GetForeignServerIdByRelId will
return valid server id and otherwise it fails with "cache lookup
error", so please remove it.
+ else
+ {
+ /* postgresExecForeignTruncate() is invoked for each server */
+ Assert(server_id == GetForeignServerIdByRelId(frel_oid));
+ }

6) You can add a comment here saying this if-clause gets executed only
once per server.
+
+ if (!OidIsValid(server_id))
+ {
+ server_id = GetForeignServerIdByRelId(frel_oid);

7) Did you try checking whether we reach hash_destroy code when a
failure happens on executing truncate on a remote table? Otherwise we
might want to do routine->ExecForeignTruncate inside PG_TRY block?
+ /* truncate_check_rel() has checked that already */
+ Assert(routine->ExecForeignTruncate != NULL);
+
+ routine->ExecForeignTruncate(ft_info->frels_list,
+ ft_info->frels_extra,
+ behavior,
+ restart_seqs);
+ }
+
+ hash_destroy(ft_htab);
+ }

8) This comment can be removed and have more descriptive comment
before the for loop in postgresExecForeignTruncate similar to the
comment what we have in postgresIsForeignRelUpdatable for updatable.
+ /* pick up remote connection, and sanity checks */

9) It will be good if you can divide up your patch into 3 separate
patches - 0001 code, 0002 tests, 0003 documentation

10) Why do we need many new tables for tests? Can't we do this -
insert, show count(*) as non-zero, truncate, show count(*) as 0, again
insert, another truncate test? And we can also have a very minimal
number of rows say 1 or 2 not 10? If possible, reduce the number of
new tables introduced. And do you have a specific reason to have a
text column in each of the tables? AFAICS, we don't care about the
column type, you could just have another int column and use
generate_series while inserting. We can remove md5 function calls.
Your tests will look clean.

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