Re: [PATCH] Logical decoding of TRUNCATE

On 4/1/18 16:01, Andres Freund wrote:
> On 2018-01-25 14:21:15 +0100, Marco Nenciarini wrote:
>> + if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
>> + {
>> +
>> + /*
>> + * Check foreign key references. In CASCADE mode, this should be
>> + * unnecessary since we just pulled in all the references; but as a
>> + * cross-check, do it anyway if in an Assert-enabled build.
>> + */
>> #ifdef USE_ASSERT_CHECKING
>> heap_truncate_check_FKs(rels, false);
>> + #else
>> + if (stmt->behavior == DROP_RESTRICT)
>> + heap_truncate_check_FKs(rels, false);
>> #endif
>> + }
>
> That *can't* be right.

You mean the part that ignores this in session_replication_role =
replica? I don't like that either. And it's also not clear why it's
needed for this feature.

>> + case REORDER_BUFFER_CHANGE_TRUNCATE:
>> + appendStringInfoString(ctx->out, " TRUNCATE:");
>> +
>> + if (change->data.truncate_msg.restart_seqs
>> + || change->data.truncate_msg.cascade)
>> + {
>> + if (change->data.truncate_msg.restart_seqs)
>> + appendStringInfo(ctx->out, " restart_seqs");
>> + if (change->data.truncate_msg.cascade)
>> + appendStringInfo(ctx->out, " cascade");
>> + }
>> + else
>> + appendStringInfoString(ctx->out, " (no-flags)");
>> + break;
>> default:
>> Assert(false);
>> }
>
> I know this has been discussed in the thread already, but it really
> strikes me as wrong to basically do some mini DDL replication feature
> via per-command WAL records.

The problem is that the interaction of TRUNCATE and foreign keys is a mess.

Let's say I have a provider database with table1, table2, and table3,
all connected by foreign keys, and a replica database with table1,
table2, and table4, all connected by foreign keys. I run TRUNCATE
table1 CASCADE on the provider. What should replication do?

The proposed patch applies the TRUNCATE table1 CASCADE on the replica,
which ends up truncating table1, table2, and table4, which is different
from what happened on the origin.

An alternative might be to make the replication protocol message say "I
truncated table1, table2" (let's say table3 is not replicated). (This
sounds more like logical replication rather than DDL replication.) That
will then fail to apply on the replica, because it requires that you
include all tables connected by foreign keys.

We could then do what we do in the DELETE case and just ignore foreign
keys altogether, which is obviously bad and not a forward-looking behavior.

Or we could do what we *should* be doing in the DELETE case and apply
cascading actions on the replica to table4, but that kind of row-by-row
processing is what we want to avoid by using TRUNCATE in the first place.

None of these solutions are good. I don't have any other ideas, though.

>> ***************
>> *** 111,116 **** CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
>> --- 111,121 ----
>> and so the default value for this option is
>> <literal>'insert, update, delete'</literal>.
>> </para>
>> + <para>
>> + <command>TRUNCATE</command> is treated as a form of
>> + <command>DELETE</command> for the purpose of deciding whether
>> + to publish, or not.
>> + </para>
>> </listitem>
>> </varlistentry>
>> </variablelist>
>
> Why is this a good idea?

I think it seemed like a good idea at the time, so to speak, but several
people have argued against it, so we should probably change this in the
final version.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services