Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Fri, Oct 18, 2019 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 3:09 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Oct 3, 2019 at 4:03 AM Tomas Vondra
> > <tomas.vondra@2ndquadrant.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