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

Thank you for your comments.
I've attached v13.

> Here are some more comments on the v12 patch:
> I still don't understand why we need sum(id), not count(*). Am I
> missing something here?

The value of "id" is used for checking whether correct records are
truncated or not.
For instance, on "truncate with ONLY clause",
At first, There are 16 values in "tru_ftable_parent", for instance,
[1,2,3,...,8,10,11,12,...,18].
By "TRUNCATE ONLY tru_ftable_parent", [1,2,3,...,8] will be truncated.
Thus, the "sum(id)" = 126.
If we use "count(*)" here, then the value will be 8.
Let's consider the case that there are 8 values [1,2,3,...,8] by some
kind of bug after running "TRUNCATE ONLY tru_ftable_parent".
Then, we miss this bug by "count(*)" because the value is the same as
the correct pattern.

> 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");

I've modified it.


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

I've fixed it.

> 3) I think we need truncatable behaviour that is consistent with updatable.

It's not correct. "truncatable" option works the same as "updatable".
I've confirmed that the table can be truncated with the combination:
truncatable on the server setting is false & truncatable on the table
setting is true.
I've also added to the test.

> 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

Sure, I've moved it.


> 5) This assertion is bogus, because GetForeignServerIdByRelId will
> return valid server id and otherwise it fails with "cache lookup
> error", so please remove it.

I've removed it.

> 6) You can add a comment here saying this if-clause gets executed only
> once per server.

I've added it.


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

I've added PG_TRY block.


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

I've removed the comment and copied the comment from
postgresIsForeignRelUpdatable,
and modified it.

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

I'll do it when I send a patch in the future, please forgive me on this patch.

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

I've removed the text field but the number of records are kept.
As I say at the top, the value of id is checked so I want to keep the
number of rows.

2021年4月5日(月) 14:59 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> 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