Re: [PATCH] Logical decoding of TRUNCATE

Hi,

On 2018-04-02 14:30:50 -0400, Peter Eisentraut wrote:
> 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.

I primarily the way the code is written. For me code differing like this
between USE_ASSERT_CHECKING and not seems like a recipe for disaster.
And yea, I'm not sure why the session_replication_role bit is here either.

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

I actually think that's a fairly sane behaviour because it allows you to
have different schemas on both sides and still deal in a reasonably sane
manner. What I'm concerned about is more that we're developing a one-of
DDL replication feature w/ corresponding bespoke WAL-logging.

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

And entirely fails if there's schema differences.

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

Well, you could also queue a re-check at the end of the processed
message, doing a bulk re-check at the end. But that's obviously more
work.

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

I think it's e.g. perfectly reasonable to want OLTP changes to be
replicated, but not to truncate historical data. So for me those actions
should be separate...

Greetings,

Andres Freund