Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Fri, Oct 18, 2019 at 5:32 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Oct 14, 2019 at 3:09 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, Oct 3, 2019 at 4:03 AM Tomas Vondra
> > <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> > >
> > >
> > > Sure, I wasn't really proposing to adding all stats from that patch,
> > > including those related to streaming. We need to extract just those
> > > related to spilling. And yes, it needs to be moved right after 0001.
> > >
> > I have extracted the spilling related code to a separate patch on top
> > of 0001. I have also fixed some bugs and review comments and attached
> > as a separate patch. Later I can merge it to the main patch if you
> > agree with the changes.
> >
>
> Few comments
> -------------------------
> 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer
> 1.
> + {
> + {"logical_decoding_work_mem", PGC_USERSET, RESOURCES_MEM,
> + gettext_noop("Sets the maximum memory to be used for logical decoding."),
> + gettext_noop("This much memory can be used by each internal "
> + "reorder buffer before spilling to disk or streaming."),
> + GUC_UNIT_KB
> + },
>
> I think we can remove 'or streaming' from above sentence for now. We
> can add it later with later patch where streaming will be allowed.
Done
>
> 2.
> @@ -206,6 +206,18 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
> </para>
> </listitem>
> </varlistentry>
> +
> + <varlistentry>
> + <term><literal>work_mem</literal> (<type>integer</type>)</term>
> + <listitem>
> + <para>
> + Limits the amount of memory used to decode changes on the
> + publisher. If not specified, the publisher will use the default
> + specified by <varname>logical_decoding_work_mem</varname>. When
> + needed, additional data are spilled to disk.
> + </para>
> + </listitem>
> + </varlistentry>
>
> It is not clear why we need this parameter at least with this patch?
> I have raised this multiple times [1][2].

I have moved it out as a separate patch (0003) so that if we need that
we need this for the streaming transaction then we can keep this.
>
> bugs_and_review_comments_fix
> 1.
> },
> &logical_decoding_work_mem,
> - -1, -1, MAX_KILOBYTES,
> - check_logical_decoding_work_mem, NULL, NULL
> + 65536, 64, MAX_KILOBYTES,
> + NULL, NULL, NULL
>
> I think the default value should be 1MB similar to
> maintenance_work_mem. The same was true before this change.
default value for maintenance_work_mem is also 64MB. Did you mean min value?
>
> 2. -#logical_decoding_work_mem = 64MB # min 1MB, or -1 to use
> maintenance_work_mem
> +i#logical_decoding_work_mem = 64MB # min 64kB
>
> It seems the 'i' is a leftover character in the above change. Also,
> change the default value considering the previous point.
oops, fixed.
>
> 3.
> @@ -2479,7 +2480,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
> ReorderBufferTXN *txn)
>
> /* update the statistics */
> rb->spillCount += 1;
> - rb->spillTxns += txn->serialized ? 1 : 0;
> + rb->spillTxns += txn->serialized ? 0 : 1;
> rb->spillBytes += size;
>
> Why is this change required? Shouldn't we increase the spillTxns
> count only when the txn is serialized?
Already agreed in previous mail so added comments
>
> 0002-Track-statistics-for-spilling
> 1.
> + <row>
> + <entry><structfield>spill_txns</structfield></entry>
> + <entry><type>integer</type></entry>
> + <entry>Number of transactions spilled to disk after the memory used by
> + logical decoding exceeds <literal>logical_work_mem</literal>. The
> + counter gets incremented both for toplevel transactions and
> + subtransactions.
> + </entry>
> + </row>
>
> The parameter name is wrong here. /logical_work_mem/logical_decoding_work_mem
done
>
> 2.
> + <row>
> + <entry><structfield>spill_txns</structfield></entry>
> + <entry><type>integer</type></entry>
> + <entry>Number of transactions spilled to disk after the memory used by
> + logical decoding exceeds <literal>logical_work_mem</literal>. The
> + counter gets incremented both for toplevel transactions and
> + subtransactions.
> + </entry>
> + </row>
> + <row>
> + <entry><structfield>spill_count</structfield></entry>
> + <entry><type>integer</type></entry>
> + <entry>Number of times transactions were spilled to disk. Transactions
> + may get spilled repeatedly, and this counter gets incremented on every
> + such invocation.
> + </entry>
> + </row>
> + <row>
> + <entry><structfield>spill_bytes</structfield></entry>
> + <entry><type>integer</type></entry>
> + <entry>Amount of decoded transaction data spilled to disk.
> + </entry>
> + </row>
>
> In all the above cases, the explanation text starts immediately after
> <entry> tag, but the general coding practice is to start from the next
> line, see the explanation of nearby parameters.
It seems it's mixed, for example, you can see
<entry>Timeline number of last write-ahead log location received and
flushed to disk, the initial value of this field being the timeline
number of the first log location used when WAL receiver is started
</entry>

or
<entry>Timeline number of last write-ahead log location received and
flushed to disk, the initial value of this field being the timeline
number of the first log location used when WAL receiver is started
</entry>

>
> It seems these parameters are added in pg-stat-wal-receiver-view in
> the docs, but in code, it is present as part of pg_stat_replication.
> It seems doc needs to be updated. Am, I missing something?
Fixed
>
> 3.
> ReorderBufferSerializeTXN()
> {
> ..
> /* update the statistics */
> rb->spillCount += 1;
> rb->spillTxns += txn->serialized ? 0 : 1;
> rb->spillBytes += size;
>
> Assert(spilled == txn->nentries_mem);
> Assert(dlist_is_empty(&txn->changes));
> txn->nentries_mem = 0;
> txn->serialized = true;
> ..
> }
>
> I am not able to understand the above code. We are setting the
> serialized parameter a few lines after we check it and increment the
> spillTxns count. Can you please explain it?
>
> Also, isn't spillTxns count bit confusing, because in some cases it
> will include subtransactions and other cases (where the largest picked
> transaction is a subtransaction) it won't include it?
>
Already discussed in the last mail.

I have merged bugs_and_review_comments_fix.patch changes to 0001 and 0002.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com