Обсуждение: The unused_oids script should have a reminder to use the 8000-8999OID range

I pushed a commit that required a new pg_proc entry today. Had I not
been involved with the work that became commit a6417078, I would
definitely not have used an OID from the range reserved for devel
system catalogs (8000 - 8999). As I understand it, this is now
standard practice.

Perhaps unsurprisingly, other committers didn't get the memo, and
haven't been using the special reserved range since its introduction
in March. I think that this could be avoided by simply making
unused_oids print a reminder about the new practice.

Is it within the discretion of committers to not use the reserved
range? It seems preferable for everybody to consistently use the
reserved OID range.
-- 
Peter Geoghegan

On Thu, Aug 1, 2019 at 10:37 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> I pushed a commit that required a new pg_proc entry today. Had I not
> been involved with the work that became commit a6417078, I would
> definitely not have used an OID from the range reserved for devel
> system catalogs (8000 - 8999). As I understand it, this is now
> standard practice.
>
> Perhaps unsurprisingly, other committers didn't get the memo, and
> haven't been using the special reserved range since its introduction
> in March. I think that this could be avoided by simply making
> unused_oids print a reminder about the new practice.


Huge +1.  Last time I had to pick a new oid it took me ages to find
the correct range for that.  The script could even suggest a random
free oid in the range, for extra laziness as you also suggested in the
almost exact same mail at
CAH2-WzmCzNMebiN4-8p=ON92m0Rz0ybxNEKrO_2J+9DqWfWP=A@mail.gmail.com :)

On Thu, Aug 1, 2019 at 1:57 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> Huge +1.  Last time I had to pick a new oid it took me ages to find
> the correct range for that.  The script could even suggest a random
> free oid in the range, for extra laziness as you also suggested in the
> almost exact same mail at
> CAH2-WzmCzNMebiN4-8p=ON92m0Rz0ybxNEKrO_2J+9DqWfWP=A@mail.gmail.com :)

Seems like I should propose a patch this time around. I don't do Perl,
but I suppose I could manage something as trivial as this.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> Is it within the discretion of committers to not use the reserved
> range? It seems preferable for everybody to consistently use the
> reserved OID range.

I think it's up to the committer in the end.  But if someone submits
a patch using high OIDs, I for one would not change that (unless it
had a collision through bad luck).

I agree that adjusting the unused_oids script would be an appropriate
thing to do now.

            regards, tom lane

On Thu, Aug 01, 2019 at 06:59:06PM -0700, Peter Geoghegan wrote:
> Seems like I should propose a patch this time around. I don't do Perl,
> but I suppose I could manage something as trivial as this.

Well, that new project policy is not that well-advertised then, see
for example the recent 5925e55, c085e1c and 313f87a.  So having some
kind of safety net would be nice.
--
Michael

On Fri, Aug 2, 2019 at 6:21 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Aug 01, 2019 at 06:59:06PM -0700, Peter Geoghegan wrote:
> > Seems like I should propose a patch this time around. I don't do Perl,
> > but I suppose I could manage something as trivial as this.
>
> Well, that new project policy is not that well-advertised then, see
> for example the recent 5925e55, c085e1c and 313f87a.  So having some
> kind of safety net would be nice.

Trivial patch for that attached.  The output is now like:

[...]
Using an oid in the 8000-9999 range is recommended.
For instance: 9427

(checking that the suggested random oid is not used yet.)

On Fri, Aug 2, 2019 at 1:42 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> Trivial patch for that attached.

Thanks!

> The output is now like:
>
> [...]
> Using an oid in the 8000-9999 range is recommended.
> For instance: 9427
>
> (checking that the suggested random oid is not used yet.)

I've taken your patch, and changed the wording a bit. I think that
it's worth being a bit more explicit. The attached revision produces
output that looks like this:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to make a random choice in the range 8000-9999.
Suggested random unused OID: 9099

I would like to push this patch shortly. How do people feel about this
wording? (It's based on the documentation added by commit a6417078.)

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> I've taken your patch, and changed the wording a bit. I think that
> it's worth being a bit more explicit. The attached revision produces
> output that looks like this:

> Patches should use a more-or-less consecutive range of OIDs.
> Best practice is to make a random choice in the range 8000-9999.
> Suggested random unused OID: 9099

Maybe s/make a/start with/ ?

Also, once people start doing this, it'd be unfriendly to suggest
9099 if 9100 is already committed.  There should be some attention
to *how many* consecutive free OIDs will be available if one starts
at the suggestion.  You could perhaps print "9099 (42 OIDs available
starting here)", and if the user doesn't like the amount of headroom
in that, they could just run it again for a different suggestion.

            regards, tom lane

On Fri, Aug 2, 2019 at 1:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Maybe s/make a/start with/ ?

> Also, once people start doing this, it'd be unfriendly to suggest
> 9099 if 9100 is already committed.  There should be some attention
> to *how many* consecutive free OIDs will be available if one starts
> at the suggestion.

How about this wording?:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to start with a random choice in the range 8000-9999.
Suggested random unused OID: 9591 (409 consecutive OID(s) available
starting here)

Attached is v3, which implements your suggestion, generating output
like the above. I haven't written a line of Perl in my life prior to
today, so basic code review would be helpful.

-- 
Peter Geoghegan

On Fri, Aug 2, 2019 at 2:52 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> Noob question here: why not start with the next unused OID in the range, and on the other hand reserve the range for
sequentially-assignedvalues?
 

The general idea is to avoid OID collisions while a patch is under
development. Choosing a value that aligns nicely with
already-allocated OIDs makes these collisions much more likely, which
commit a6417078 addressed back in March. We want a random choice among
patches, but OIDs used within a patch should be consecutive.

(There is still some chance of a collision, but you have to be fairly
unlucky to have that happen under the system introduced by commit
a6417078.)

It's probably the case that most patches that create a new pg_proc
entry only create one. The question of consecutive OIDs only comes up
with a fairly small number of patches.


--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> Attached is v3, which implements your suggestion, generating output
> like the above. I haven't written a line of Perl in my life prior to
> today, so basic code review would be helpful.

The "if ($oid > $prev_oid + 2)" test seems unnecessary.
It's certainly wrong to keep iterating beyond the first
oid that's > $suggestion.

Perhaps you meant to go back and try a different suggestion
if there's not at least 2 free OIDs?  But then there needs
to be an outer loop around both of these loops.

            regards, tom lane

On Fri, Aug 2, 2019 at 3:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The "if ($oid > $prev_oid + 2)" test seems unnecessary.
> It's certainly wrong to keep iterating beyond the first
> oid that's > $suggestion.

Sorry. That was just carelessness on my part. (Being the world's worst
Perl programmer is no excuse.)

How about the attached? I've simply removed the "if ($oid > $prev_oid
+ 2)" test.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> How about the attached? I've simply removed the "if ($oid > $prev_oid
> + 2)" test.

Better ... but I'm the world's second worst Perl programmer,
so I have little to say about whether it's idiomatic.

            regards, tom lane

On Fri, Aug 2, 2019 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Better ... but I'm the world's second worst Perl programmer,
> so I have little to say about whether it's idiomatic.

Perhaps Michael can weigh in here? I'd rather hear a second opinion on
v4 of the patch before proceeding.

-- 
Peter Geoghegan

On Sat, Aug 3, 2019 at 2:40 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Fri, Aug 2, 2019 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Better ... but I'm the world's second worst Perl programmer,
> > so I have little to say about whether it's idiomatic.
>
> Perhaps Michael can weigh in here? I'd rather hear a second opinion on
> v4 of the patch before proceeding.

I probably write less perl than Michael, but it looks just fine to me.

On Sat, Aug 03, 2019 at 11:40:24AM +0200, Julien Rouhaud wrote:
> I probably write less perl than Michael, but it looks just fine to me.

Indentation with pgperltidy complains with the attached diff (based on
top of v4).

+printf "Patches should use a more-or-less consecutive range of OIDs.\n";
"Patches should try to use a consecutive range of OIDs"?

Why choosing a random position within [8000,9999]?  This leads to the
following messages for example with multiple runs, which is confusing:
Suggested random unused OID: 9473 (527 consecutive OID(s) available
Suggested random unused OID: 8159 (31 consecutive OID(s) available
Suggested random unused OID: 9491 (509 consecutive OID(s) available

Wouldn't it be better to choose the lowest position in the development
range, and then adapt the suggestion based on that?  We could
recommend the range if there are at least 10 OIDs available in the
range from the lowest position, and there are few patches eating more
than 5-10 OIDs at once.
--
Michael

On Sat, Aug 3, 2019 at 7:48 PM Michael Paquier <michael@paquier.xyz> wrote:
> Why choosing a random position within [8000,9999]?  This leads to the
> following messages for example with multiple runs, which is confusing:
> Suggested random unused OID: 9473 (527 consecutive OID(s) available
> Suggested random unused OID: 8159 (31 consecutive OID(s) available
> Suggested random unused OID: 9491 (509 consecutive OID(s) available
>
> Wouldn't it be better to choose the lowest position in the development
> range, and then adapt the suggestion based on that?

No, it wouldn't. The entire point of suggesting a totally random OID
is that it minimizes the probability of a collision among concurrently
developed patches, per the policy established by commit a6417078 --
what you suggest would defeat the very purpose of this patch. In fact,
having everybody see the same suggestion from unused_oids would
*maximize* the number of OID collisions.

> We could
> recommend the range if there are at least 10 OIDs available in the
> range from the lowest position, and there are few patches eating more
> than 5-10 OIDs at once.

That sounds like an over-engineered solution to a problem that doesn't exist.

-- 
Peter Geoghegan

On Fri, Aug 2, 2019 at 1:28 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> I'm fine with it!

Pushed a version with similar wording just now.

Thanks!
-- 
Peter Geoghegan

On Mon, Aug 5, 2019 at 8:51 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> Pushed a version with similar wording just now.

Thanks!

On Mon, Aug 05, 2019 at 09:00:26PM +0200, Julien Rouhaud wrote:
> Thanks!

What you have committed does that:
+do
+{
+   $suggestion = int(8000 + rand(2000));
+} while (grep(/^$suggestion$/, @{$oids}));
So it would be possible to get 9998-9999 as suggestion.  In which
case, one can basically finish with this message:
Suggested random unused OID: 9999 (1 consecutive OID(s) available
starting here)

Wouldn't it be better to keep some room at the end of the allowed
array?  Or at least avoid suggesting ranges where there is less than
3-5 OIDs available consecutively.
--
Michael

On Mon, Aug 5, 2019 at 8:47 PM Michael Paquier <michael@paquier.xyz> wrote:
> So it would be possible to get 9998-9999 as suggestion.  In which
> case, one can basically finish with this message:
> Suggested random unused OID: 9999 (1 consecutive OID(s) available
> starting here)

I strongly doubt that this will ever be a real problem. Just try again.

> Wouldn't it be better to keep some room at the end of the allowed
> array?  Or at least avoid suggesting ranges where there is less than
> 3-5 OIDs available consecutively.

Not in my view. There is value in having simple, predictable behavior.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Aug 5, 2019 at 8:47 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Wouldn't it be better to keep some room at the end of the allowed
>> array?  Or at least avoid suggesting ranges where there is less than
>> 3-5 OIDs available consecutively.

> Not in my view. There is value in having simple, predictable behavior.

There was some discussion of that upthread, and Peter argued that many
patches only need one OID anyway so why try harder.  I'm not totally
sure I buy that --- my sense is that even simple patches tend to add
several related functions not just one.  But as long as the script
tells you how many OIDs are available, what's the problem?  Just run
it again if you want a different suggestion, or make your own choice.

            regards, tom lane

On Mon, Aug 5, 2019 at 10:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> There was some discussion of that upthread, and Peter argued that many
> patches only need one OID anyway so why try harder.  I'm not totally
> sure I buy that --- my sense is that even simple patches tend to add
> several related functions not just one.

That has been my experience, but it turns out that that was colored by
the areas that I work in. I reviewed the history of pg_proc.dat today,
and found that adding multiple entries at a time is more common that I
thought it was.

> But as long as the script
> tells you how many OIDs are available, what's the problem?  Just run
> it again if you want a different suggestion, or make your own choice.

Right. Besides, adding something along the lines Michael described
necessitates fixing the problems that it creates. We'll run out of
blocks of 5 contiguous OIDs (or whatever) far sooner than we'll run
out of single OIDs. Now we have to worry about doing a second
(actually a third) pass over the OIDs as a fallback when that happens.
And so on.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Aug 5, 2019 at 10:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> But as long as the script
>> tells you how many OIDs are available, what's the problem?  Just run
>> it again if you want a different suggestion, or make your own choice.

> Right. Besides, adding something along the lines Michael described
> necessitates fixing the problems that it creates. We'll run out of
> blocks of 5 contiguous OIDs (or whatever) far sooner than we'll run
> out of single OIDs.

Well, if we ever get even close to that situation, this whole approach
isn't really gonna work.  My estimate is that in any one development
cycle we'll commit order-of-a-couple-dozen patches that consume new OIDs.
In that context you'd be just unlucky to get an OID suggestion that
doesn't have dozens to hundreds of free OIDs after it.  (If the rate
of manually-assigned-OID consumption were any faster than that, we'd
have filled up the 1-10K space long since.)

            regards, tom lane

On Mon, Aug 5, 2019 at 11:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Right. Besides, adding something along the lines Michael described
> > necessitates fixing the problems that it creates. We'll run out of
> > blocks of 5 contiguous OIDs (or whatever) far sooner than we'll run
> > out of single OIDs.
>
> Well, if we ever get even close to that situation, this whole approach
> isn't really gonna work.

My point was that I don't see any reason to draw the line at or after
what Michael suggested, but before handling the exhaustion of
available blocks of 5 contiguous OIDs in the range 8000-9999. It's
just busy work.

-- 
Peter Geoghegan

On 2019-Aug-06, Tom Lane wrote:

> My estimate is that in any one development
> cycle we'll commit order-of-a-couple-dozen patches that consume new OIDs.
> In that context you'd be just unlucky to get an OID suggestion that
> doesn't have dozens to hundreds of free OIDs after it.  (If the rate
> of manually-assigned-OID consumption were any faster than that, we'd
> have filled up the 1-10K space long since.)

If we ever get to a point where this is a real problem in one cycle, we
can just run the renumber_oids script before the end of the cycle.

So IMO what we have now is more than sufficient for the time being.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services