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

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