On Mon, Mar 30, 2020 at 03:52:38PM +0530, Amit Kapila wrote:
> On Sun, Mar 29, 2020 at 5:49 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
>
> @@ -1249,6 +1250,16 @@ XLogInsertRecord(XLogRecData *rdata,
> ProcLastRecPtr = StartPos;
> XactLastRecEnd = EndPos;
>
> + /* Provide WAL update data to the instrumentation */
> + if (inserted)
> + {
> + pgWalUsage.wal_bytes += rechdr->xl_tot_len;
> + if (doPageWrites && fpw_lsn <= RedoRecPtr)
> + pgWalUsage.wal_fpw_records++;
> + else
> + pgWalUsage.wal_records++;
> + }
> +
>
> I think the above code has multiple problems. (a) fpw_lsn can be
> InvalidXLogRecPtr and still there could be full-page image (for ex.
> when REGBUF_FORCE_IMAGE flag for buffer is set). (b) There could be
> multiple FPW records while inserting a record; consider when there are
> multiple registered buffers. I think the right place to figure this
> out is XLogRecordAssemble. (c) There are cases when we also attach the
> record data even when we decide to write FPW (cf. REGBUF_KEEP_DATA),
> so we might want to increment wal_fpw_records and wal_records for such
> cases.
>
> I think the right place to compute this information is
> XLogRecordAssemble even though we update it at the place where you
> have it in the patch. You can probably compute that in local
> variables and then transfer to pgWalUsage in XLogInsertRecord. I am
> fine if you can think of some other way but the current patch doesn't
> seem correct to me.
My previous approach was indeed totally broken. v8 attached which hopefully
will be ok.