Hi Mark,
I have tried to fix your review comment in the attached version,
please see my inline reply below.
On Fri, Sep 10, 2021 at 8:06 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Thu, Sep 9, 2021 at 11:12 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
> >
> >
>
> Thank you, for looking at the patch. Please see my reply inline below:
>
> >
> > > On Sep 8, 2021, at 6:44 AM, Amul Sul <sulamul@gmail.com> wrote:
> > >
> > > Here is the rebased version.
> >
> > v33-0004
> >
> > This patch moves the include of "catalog/pg_control.h" from transam/xlog.c into access/xlog.h, making pg_control.h
indirectlyincluded from a much larger set of files. Maybe that's ok. I don't know. But it seems you are doing this
merelyto get the symbol (not even the definition) for struct DBState. I'd recommend rearranging the code so this isn't
necessary,but otherwise you'd at least want to remove the now redundant includes of catalog/pg_control.h from
xlogdesc.c,xloginsert.c, auth-scram.c, postmaster.c, misc/pg_controldata.c, and pg_controldata/pg_controldata.c.
> >
>
> Yes, you are correct, xlog.h is included in more than 150 files. I was
> wondering if we can have a forward declaration instead of including
> pg_control.h (e.g. The same way struct XLogRecData was declared in
> xlog.h). Perhaps, DBState is enum & I don't see we have done the same
> for enum elsewhere as we are doing for structures, but that seems to
> be fine, IMO.
>
> Earlier, I was unsure before preparing this patch, but since that
> makes sense (I assumed) and minimizes duplications, can we go ahead
> and post separately with the same change in StartupXLOG() which I have
> skipped for the same reason mentioned in patch commit-msg.
>
FYI, I have posted this patch separately [1] & drop it from the current set.
> > v33-0005
> > The code comment change in autovacuum.c introduces a non-grammatical sentence: "First, the system is not read only
i.e.wal writes permitted".
> >
Fixed.
> > The function comment in checkpointer.c reads more like it toggles the system into allowing something, rather than
actuallydoing that same something: "SendSignalToCheckpointer allows a process to send a signal to the checkpoint
process".
> >
I am not sure I understood the concern, what comments should you
think? This function helps to signal the checkpointer, but doesn't
tell what it is supposed to do.
> > The new code comment in ipci.c contains a typo, but more importantly, it doesn't impart any knowledge beyond what a
readerof the function name could already surmise. Perhaps the comment can better clarify what is happening: "Set up
walprobibit shared state"
> >
Done.
> > The new code comment in sync.c copies and changes a nearby comment but drops part of the verb phrase: "As in
ProcessSyncRequests,we don't want to stop wal prohibit change requests". The nearby comment reads "stop absorbing". I
thinkthis one should read "stop processing". This same comment is used again below. Then a third comment reads "For
thesame reason mentioned previously for the wal prohibit state change request check." That third comment is too glib.
> >
Ok, "stop processing" is used. I think the third comment should be
fine instead of coping the same again, however, I change that comment
a bit for more clarity as "For the same reason mentioned previously
for the same function call".
> > tcop/utility.c needlessly includes "access/walprohibit.h"
> >
> > wait_event.h extends enum WaitEventIO with new values WAIT_EVENT_WALPROHIBIT_STATE and
WAIT_EVENT_WALPROHIBIT_STATE_CHANGE. I don't find the difference between these two names at all clear. Waiting for a
statechange is clear enough. But how is waiting on a state different?
> >
WAIT_EVENT_WALPROHIBIT_STATE_CHANGE gets set in pg_prohibit_wal()
while waiting for the system to prohibit state change.
WAIT_EVENT_WALPROHIBIT_STATE is set for the checkpointer process when
it sees the system is in a WAL PROHIBITED state & stops there. But I
think it makes sense to have only one, i.e.
WAIT_EVENT_WALPROHIBIT_STATE_CHANGE. The same can be used for
checkpointer since it won't do anything until wal prohibited state
change.
Remove WAIT_EVENT_WALPROHIBIT_STATE in the attached version.
> > xlog.h defines a new enum. I don't find any of it clear; not the comment, nor the name of the enum, nor the names
ofthe values:
> >
> > /* State of work that enables wal writes */
> > typedef enum XLogAcceptWritesState
> > {
> > XLOG_ACCEPT_WRITES_PENDING = 0, /* initial state, not started */
> > XLOG_ACCEPT_WRITES_SKIPPED, /* skipped wal writes */
> > XLOG_ACCEPT_WRITES_DONE /* wal writes are enabled */
> > } XLogAcceptWritesState;
> >
> > This enum seems to have been written from the point of view of someone who already knew what it was for. It needs
tobe written in a way that will be clear to people who have no idea what it is for.
> >
I tried to avoid the function name in the comment, since the enum name
pretty much resembles the XLogAcceptWrite() function name whose
execution state we are trying to track, but added now, that would be
much clearer.
> > v33-0006:
> >
> > The new code comments in brin.c and elsewhere should use the verb "require" rather than "have", otherwise "building
indexes"reads as a noun phrase rather than as a gerund: /* Building indexes will have an XID */
> >
Rephrased the comments but I think HAVE XID is much more appropriate
there because that assert function name ends with HaveXID.
Apart from this I have moved CheckWALPermitted() closer to
START_CRIT_SECTION which you have pointed out in your other post and
made a few other changes. Note that patch numbers are changed, I have
rebased my implementation on top of the under discussion refactoring
patches which I have posted previously [2] and reattached the same
here to make CFbot continue its testing.
Note that with the current version patch on the latest master head
getting one issue but can be seen sometimes only where one, the same
INSERT query gets stuck, waiting for WALWriteLock in exclusive mode. I
am not sure if it is due to my changes, but that is not occurring without
my patch. I am looking into that, just in case if anybody wants to
know more, I have attached the backtrace, pg_lock & ps output, see
ps-bt-pg_lock.out.text attached file.
Regards,
Amul
1] https://postgr.es/m/CAAJ_b97nd_ghRpyFV9Djf9RLXkoTbOUqnocq11WGq9TisX09Fw@mail.gmail.com
2] https://postgr.es/m/CAAJ_b96G-oBxDC3C7Y72ER09bsheGHOxBK1HXHVOyHNXjTDmcA@mail.gmail.com