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