Re: Patch to implement pg_current_logfile() function

Hello Robert,

On Wed, 7 Dec 2016 18:52:24 -0500
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold
> <gilles(dot)darold(at)dalibo(dot)com> wrote:
> > It seems that all fixes have been included in this patch.

I read this and knew that I hadn't finished review, but didn't
immediately respond because I thought the patch had to be
marked "ready for committer" on commitfest.postgresql.org
before the committers would spend a lot of time on it.

I don't have the time to fully respond, or I'd be able to
attach new code, but want to comment before anybody else
spends a lot of time on this patch.

> Yikes. This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now,

Yes and no. I don't really know what the coding style is and
rather than have to make multiple corrections to code that might
get weeded out and discarded I've been focusing on getting the logic
right. Really, I've not put a thought to it except for trying to write
what I write like what's there, and sticking to < 80 chars per line.

There has been lots of review.
The only truly effective way I've found to communicate regards
the pg_current_logfiles patch has been to write code and provide
detailed test cases. I could be wrong, but unless I submit
code I don't feel like I've been understood.
I just haven't been interested in re-formatting
somebody else's code before I think the code really works.

> I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code. And not be unnecessarily
> complicated.

I've been introducing some complication, but I hope it's necessary.
To keep the review process simple my plan has been to submit
multiple patches. One with the basics and more on top of that
that introduce complication that can be considered and accepted or
rejected. So I send emails with multiple patches, some that I think
need to go into the core patch and others to be kept separate.
But, although I try, this does not seem to have been communicated
because I keep getting emails back that contain only a single patch.

Maybe something's wrong with my review process but I don't know
how to fix it.

If you think a single patch is the way to go I can open up
separate tickets at commitfest.postgresql.org. But this seems
like overkill because all the patches touch the same code.

The extreme case is the attached "cleanup_rotate" patch.
(Which applies to v14 of this patch.) It's nothing but
a little bit of tiding up of the master branch, but does
touch code that's already being modified so it seems
like the committers would want to look at it at the same
time as when reviewing the pg_current_logfile patch.
Once you've looked at the pg_current_logfile patch
you've already looked at and modified code in the function
the "cleanup_rotate" patch touches.

But the "cleanup_rotate" patch got included
in the v15 version of the patch and is also in v16.
I'm expecting to submit it as a separate patch
along with the pg_current_logfile patch and tried
to be very very clear about this. It
really has nothing to do with the pg_current_logfile
functionality. (And is the only "separate" patch
which really has nothing to do with the pg_current_logfile
functionality.)

More examples of separate patches are below. Any guidance
would be appreciated.

>
> Detailed comments below:

> rm_log_metainfo() could be merged into logfile_writename(), since
> they're always called in conjunction and in the same pattern.

This would be true, if it weren't for the attached
"retry_current_logfiles" patches that were applied in
v15 of the patch and removed from v16 due to some
mis-communication I don't understand.

(But the attached patches apply on top of the the v14 patch.
I don't have time to refactor them now.)

FYI. The point of the "retry_current_logfiles"
patch series is to handle the case
where logfile_writename gets a ENFILE or EMFILE.
When this happens the current_logfiles file can
"skip" and not contain the name(s) of the current
log file for an entire log rotation. To miss, say,
a month of logs because the logs only rotate monthly
and your log processing is based on reading the
current_logfiles file sounds like a problem.

What I was attempting to communicate in my email response
to the v15 patch is that the (attached, but applies to the
v14 patch again) "backoff" patch should be submitted
as a separate patch. It seems over-complicated, but
exists in case a committer is worried about re-trying
writes on a system that's busy enough to generate ENFILE
or EMFILE errors.

> + if (errno == ENFILE || errno == EMFILE)
> + ereport(LOG,
> + (errmsg("system is too busy to write logfile meta
> info, %s will be updated on next rotation (or use SIGHUP to retry)",
> LOG_METAINFO_DATAFILE)));
>
> This seems like a totally unprincipled reaction to a purely arbitrary
> subset of errors. EMFILE or ENFILE doesn't represent a general "too
> busy" condition, and logfile_open() has already logged the real error.

This is my doing. I wrote "too busy" because that's what the existing
code comments say that ENFILE and EMFILE are indicative of.

The point of the code is to report not just the real error, but what
effect the real error has -- that the current_logfiles file did not
get updated in a timely fashion. Maybe this isn't necessary, especially
if the write of current_logfiles gets retried on failure. Or maybe
the right thing to do is to give logfile_open() an argument that
let's it elaborate it's error message. Any guidance here would
be appreciated.

Thanks for the review and I'll try to read over and digest the details
before submitting another email with patches.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein