Re: [PATCH] Logical decoding of TRUNCATE

Hi,

On 2018-04-02 07:39:57 +0100, Simon Riggs wrote:
> On 1 April 2018 at 21:01, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >> ***************
> >> *** 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?
>
> TRUNCATE removes rows, just as DELETE does, so anybody that wants to
> publish the removal of rows will be interested in this.

I'm not convinced. I think it's perfectly reasonable to want to truncate
away data on the live node, but maintain it on an archival node. In
that case one commonly wants to continue to maintain OLTP changes (hence
DELETE), but not the bulk truncation. I think there's a reasonable
counter-argument in that this isn't fine grained enough.

> >> + * Write a WAL record to allow this set of actions to be logically decoded.
> >> + * We could optimize this away when !RelationIsLogicallyLogged(rel)
> >> + * but that doesn't save much space or time.
> >
> > What you're saying isn't that you're not logging anything, but that
> > you're allocating the header regardless? Because this certainly sounds
> > like you unconditionally log a WAL record.
>
> It says that, yes, my idea - as explained.

My complaint is that the comment and the actual implementation
differ. The above sounds like it's unconditionally WAL logging, but:

+ /*
+ * Write a WAL record to allow this set of actions to be logically decoded.
+ * We could optimize this away when !RelationIsLogicallyLogged(rel)
+ * but that doesn't save much space or time.
+ *
+ * Assemble an array of relids, then an array of seqrelids so we can write
+ * a single WAL record for the whole action.
+ */
+ logrelids = palloc(maxrelids * sizeof(Oid));
+ foreach (cell, relids_logged)
+ {
+ nrelids++;
+ if (nrelids > maxrelids)
+ {
+ maxrelids *= 2;
+ logrelids = repalloc(logrelids, maxrelids * sizeof(Oid));
+ }
+ logrelids[nrelids - 1] = lfirst_oid(cell);
+ }
+
+ foreach (cell, seq_relids_logged)
+ {
+ nseqrelids++;
+ if ((nrelids + nseqrelids) > maxrelids)
+ {
+ maxrelids *= 2;
+ logrelids = repalloc(logrelids, maxrelids * sizeof(Oid));
+ }
+ logrelids[nrelids + nseqrelids - 1] = lfirst_oid(cell);
+ }
+
+ if ((nrelids + nseqrelids) > 0)
+ {
+ xl_heap_truncate xlrec;
+
+ xlrec.dbId = MyDatabaseId;
+ xlrec.nrelids = nrelids;
+ xlrec.nseqrelids = nseqrelids;
+ xlrec.flags = 0;
+ if (behavior == DROP_CASCADE)
+ xlrec.flags |= XLH_TRUNCATE_CASCADE;
+ if (restart_seqs)
+ xlrec.flags |= XLH_TRUNCATE_RESTART_SEQS;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapTruncate);
+ XLogRegisterData((char *) logrelids,
+ (nrelids + nseqrelids) * sizeof(Oid));
+
+ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
+
+ (void) XLogInsert(RM_HEAP_ID, XLOG_HEAP_TRUNCATE);
+ }

Note that the XLogInsert() is in an if branch that's only executed if
there's either logged relids or sequence relids.

I think logrelids should be allocated at the max size immediately, and
the comment rewritten to explicitly only talk about the allocation,
rather than sounding like it's about WAL logging as well.

Greetings,

Andres Freund