9.2 branch and 9.1beta2 timing (was Re: InitProcGlobal cleanup)

While working on my patch to reduce the overhead of frequent table
locks, I had cause to monkey with InitProcGlobal() and noticed that
it's sort of a mess. For reasons that are not clear to me, it
allocates one of the three PGPROC arrays using ShemInitStruct() and
the other two using ShmemAlloc(). I'm not clear on why we should use
different functions for different allocations, and it also seems like
it would make sense to do the whole allocation at once instead of
doing three separate ones. Also, the setup of AuxiliaryProcs is
strangely split into two parts, one at the top of the function (where
we allocate the memory) and the other at the bottom (where we
initialize it), but there's no clear reason to break it up like that.

Any reason not to instead do something like the attached?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> While working on my patch to reduce the overhead of frequent table
> locks, I had cause to monkey with InitProcGlobal() and noticed that
> it's sort of a mess. For reasons that are not clear to me, it
> allocates one of the three PGPROC arrays using ShemInitStruct() and
> the other two using ShmemAlloc(). I'm not clear on why we should use
> different functions for different allocations, and it also seems like
> it would make sense to do the whole allocation at once instead of
> doing three separate ones. Also, the setup of AuxiliaryProcs is
> strangely split into two parts, one at the top of the function (where
> we allocate the memory) and the other at the bottom (where we
> initialize it), but there's no clear reason to break it up like that.

> Any reason not to instead do something like the attached?

I find this a whole lot less readable, because you've largely obscured
the fact that there are three or four different groups of PGPROC
structures being built here and then linked into several different
lists/arrays. The code might be okay but it desperately needs more
comments.

regards, tom lane

On Thu, Jun 2, 2011 at 1:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> While working on my patch to reduce the overhead of frequent table
>> locks, I had cause to monkey with InitProcGlobal() and noticed that
>> it's sort of a mess.  For reasons that are not clear to me, it
>> allocates one of the three PGPROC arrays using ShemInitStruct() and
>> the other two using ShmemAlloc().  I'm not clear on why we should use
>> different functions for different allocations, and it also seems like
>> it would make sense to do the whole allocation at once instead of
>> doing three separate ones.  Also, the setup of AuxiliaryProcs is
>> strangely split into two parts, one at the top of the function (where
>> we allocate the memory) and the other at the bottom (where we
>> initialize it), but there's no clear reason to break it up like that.
>
>> Any reason not to instead do something like the attached?
>
> I find this a whole lot less readable, because you've largely obscured
> the fact that there are three or four different groups of PGPROC
> structures being built here and then linked into several different
> lists/arrays.  The code might be okay but it desperately needs more
> comments.

OK, here's a version with more comments.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> OK, here's a version with more comments.

Looks OK to me, assuming you've checked that the right number of PGPROCs
are getting created (in particular the AV launcher is no longer
accounted for explicitly).

regards, tom lane

On Thu, Jun 2, 2011 at 3:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> OK, here's a version with more comments.
>
> Looks OK to me, assuming you've checked that the right number of PGPROCs
> are getting created (in particular the AV launcher is no longer
> accounted for explicitly).

That should be fine, due to the way MaxBackends is initialized. See
related comment around guc.c:103.

I'll commit this to 9.2 after we branch. (When are we doing that, BTW?)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'll commit this to 9.2 after we branch. (When are we doing that, BTW?)

Sometime in the next two weeks I guess ;-). At the PGCon meeting we
said 1 June, but seeing that we still have a couple of open beta2 issues
I'm not in a hurry.

I think a reasonable plan would be to fix the currently known open
issues, push out a beta2, and then branch. That would avoid
double-patching. We'd want to get this done before the commitfest
starts on the 15th, of course, so if we stick to usual release
scheduling that would mean wrap next Thursday (June 9), beta2 announce
on Monday the 13th, and make the branch somewhere around that date as
well.

Comments?

regards, tom lane

On Thu, Jun 2, 2011 at 4:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'll commit this to 9.2 after we branch.  (When are we doing that, BTW?)
>
> Sometime in the next two weeks I guess ;-).  At the PGCon meeting we
> said 1 June, but seeing that we still have a couple of open beta2 issues
> I'm not in a hurry.
>
> I think a reasonable plan would be to fix the currently known open
> issues, push out a beta2, and then branch.  That would avoid
> double-patching.  We'd want to get this done before the commitfest
> starts on the 15th, of course, so if we stick to usual release
> scheduling that would mean wrap next Thursday (June 9), beta2 announce
> on Monday the 13th, and make the branch somewhere around that date as
> well.
>
> Comments?

OK by me. It appears that the open items list is a bit stale:

http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Open_Items

The first item listed there is, I believe, fixed. I'm not sure about
the second. You just volunteered to fix the third, and the fourth is
awaiting comments on -bugs. The larger problem is that there are
likely some other things that should be listed there, but aren't. If
anyone is aware of stuff we need to get done, please add it there...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> It appears that the open items list is a bit stale:
>
> http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Open_Items
>
> The first item listed there is, I believe, fixed.

That was "SSI HOT chain traversal issue" -- which was fixed. I just
moved it to "Resolved Issues".

> I'm not sure about the second.

That's "Make DDL commands SSI-aware". That is two to four lines of
code away from complete, but I got stuck last time I tried to figure
out where to put those lines. The code for DROP TABLE and TRUNCATE
TABLE is not quite as easy to follow as most of the PostgreSQL code
base -- or so it seemed to me. I'll take another run at it. A
patch will be forthcoming on Sunday at the latest. All other DDL
commands are done and have been through some testing.

> If anyone is aware of stuff we need to get done, please add it
> there...

I've submitted a comment-only patch and will be submitting a patch
by Sunday to add a few more lines to README-SSI. I don't know that
those are beta2 blockers, though. Should I add them to "Not
Blockers for Beta2"?

-Kevin

On Thu, Jun 2, 2011 at 5:35 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> It appears that the open items list is a bit stale:
>>
>> http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Open_Items
>>
>> The first item listed there is, I believe, fixed.
>
> That was "SSI HOT chain traversal issue" -- which was fixed.  I just
> moved it to "Resolved Issues".
>
>> I'm not sure about the second.
>
> That's "Make DDL commands SSI-aware".  That is two to four lines of
> code away from complete, but I got stuck last time I tried to figure
> out where to put those lines.  The code for DROP TABLE and TRUNCATE
> TABLE is not quite as easy to follow as most of the PostgreSQL code
> base -- or so it seemed to me.  I'll take another run at it.  A
> patch will be forthcoming on Sunday at the latest.  All other DDL
> commands are done and have been through some testing.
>
>> If anyone is aware of stuff we need to get done, please add it
>> there...
>
> I've submitted a comment-only patch and will be submitting a patch
> by Sunday to add a few more lines to README-SSI.  I don't know that
> those are beta2 blockers, though.  Should I add them to "Not
> Blockers for Beta2"?

Either is fine. Comment patches are easy.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company