Robert Haas wrote:
> I think this one could be removed:
>
> + if (n > 0)
> + elog(DEBUG1,"Absorbing %d fsync requests",n);
>
Right; that one is just there to let you know the patch is working, and
how much the background writer does for you per pass, mainly for the
purpose of reviewing the patch.
> But if this is generating a lot of log data or adding a lot of
> overhead, then you have bigger problems anyway:
>
> + elog(DEBUG1, "Unable to forward fsync request, executing directly");
>
The argument against this log line even existing is that if the field is
added to pg_stat_bgwriter, that's probably how you want to monitor this
data anyway. I don't think there's actually much value to it, which is
one reason I didn't worry too much about matching style guidelines,
translation, etc. You've found the two things that I think both need to
go away before commit, but I left them in because I think they're
valuable for testing the patch itself does what it's supposed to.
> Also, how about referring to this as buffers_backend_fsync
> consistently throughout, instead of dropping the "f" in some places?
>
I started out touching code that called it just "sync", but then crossed
to other code that called it "fsync", and made the external UI use that
name. No objections to sorting that out within my patch so it's consistent.
--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us